Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10536014:
Implement ISO BMFF support in Media Source. (Closed)
|
Created:
8 years, 6 months ago by strobe_ Modified:
8 years, 6 months ago Reviewers:
acolwell GONE FROM CHROMIUM CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement ISO BMFF support in Media Source.
BUG=129072
TEST=MP4StreamParserTest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142268
Patch Set 1 #
Total comments: 148
Patch Set 2 : Add box_reader_unittest.cc #Patch Set 3 : Code review changes; add offset_byte_queue_unittest.cc #
Total comments: 16
Patch Set 4 : Monomorphized and object-orientated. #Patch Set 5 : A thorough sign-ification #
Total comments: 63
Patch Set 6 : Lint and compile fixes #Patch Set 7 : Inject parameter sets #
Total comments: 2
Patch Set 8 : Add break, only use EXPECT in avc_unittest #Patch Set 9 : Use vector in Annex B conversion instead of raw buffer #
Total comments: 1
Patch Set 10 : Add MEDIA_EXPORT for win, linux_chromeos #Patch Set 11 : Add NewMediaSegment callback #Patch Set 12 : Don't use std::abs() (breaks on ARM) #Messages
Total messages: 19 (0 generated)
Here is an initial pass on everything. General themes are: - Use int everywhere you can. Only use unsigned values when you absolutely must. int is assumed to be at least 32 bits. - Use descriptive variable/method names to enhance readability. - Use the correct #include order. - Take advantage of early returns to avoid indentation and readability problems. https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... media/filters/chunk_demuxer.cc:15: #include "media/mp4/mp4_stream_parser.h" includes should be alphabetical https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... media/filters/chunk_demuxer.cc:47: &kVP8CodecInfo, remove. VP8 should not be allowed in an audio only mimetype https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/source_b... File media/filters/source_buffer.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/source_b... media/filters/source_buffer.cc:10: #include "media/mp4/mp4_stream_parser.h" Remove. Object not used in this file. Please remove the webm_stream_parser.h too. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc File media/mp4/avc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newcode8 media/mp4/avc.cc:8: #include <algorithm> nit: system header files come right after avc.h https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:14: const static size_t kAnnexBStartCodeSize = 4; nit: use int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:19: bool Parse(BoxReader* r, AVCDecoderConfigurationRecord* avcc) { nit: rename r -> reader & avcc to something more descriptive like avc_config https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:19: bool Parse(BoxReader* r, AVCDecoderConfigurationRecord* avcc) { Forward declared in avc.h like the other methods. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:21: RCHECK(r->Read(&avcc->version) && avcc->version == 1 && Change read signatures to include size. ie Read1Byte(), Read4Bytes() https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:26: uint8 length_size_minus_one; nit: use int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:31: uint8 num_sps; nit: use int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:37: uint16 sps_length; nit: use int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:42: uint8 num_pps; nit:use int here and various places below. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:55: bool ConvertAVCCToAnnexBInPlaceForLengthSize4(size_t buf_size, uint8* buf) { Add static if only used in this file. Otherwise add forward decl to .h. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:59: uint32 nal_size = (((uint32) buf[pos]) << 24) use static_cast<> https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:69: bool ConvertAVCCToAnnexB(uint8 length_size, std::vector<uint8>* buffer) { use int for length_size https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:82: uint32 nal_size = temp[pos]; use int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h File media/mp4/avc.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode16 media/mp4/avc.h:16: uint8 version; nit: use int here and below. In general Chrome prefers use of regular int instead of unsigned types and assumes int is at least 32 bits. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode32 media/mp4/avc.h:32: std::vector<SPS> spses; nit: How about sps_list & pps_list? or xxx_vector? https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode41 media/mp4/avc.h:41: ChannelLayout ConvertAACChannelCountToChannelLayout(uint16 count); nit: change count to int https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest.cc File media/mp4/avc_unittest.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:5: #include "base/basictypes.h" Group w/ other base includes https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:7: #include "string.h" Use <> & place before all other includes. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:12: using media::mp4::ConvertAVCCToAnnexB; Remove and put tests in media::mp4 namespace https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:20: class AVCCConversionTest : public testing::TestWithParam<uint32> { Use int here and various other places below. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:24: for (uint32 i = 1; i < length_size; i++) { nit: remove {} https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:30: for (uint32 i = 1; i < length_size; i++) { nit: remove {} https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:51: // This actually still works for one-byte sizes. I'm not sure what this comment is trying to tell me. Please reword. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:15: bool Parse(BoxReader* r, FileType* type) { nit: r -> reader here and elsewhere https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:56: : default_sample_info_size(0), sample_count(0) {} nit: } on next line https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:83: : is_encrypted(false), default_iv_size(0) {} nit: } on next line. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:128: r->Skip(10) && // reserved WDYT about renaming this to SkipBytes so the units are clearer? https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:455: + sample_flags_present + sample_composition_time_offsets_present; nit: + goes on the end of the previous line and this indentation is wrong. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:538: RCHECK(!(sidx->sizes[i] & 0x8000000) != 0 || What?!? A comment indicating what this is checking would be nice. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... File media/mp4/box_definitions.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:7: #pragma once remove https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:12: #include <string> goes before non-system includes https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:14: #define ARRAY_LENGTH(x) (sizeof(x) / sizeof(*(x))) remove. Use arraysize() from basictypes.h instead https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:316: bool Parse(BoxReader* r, T* box); \ WDYT about bool T::Parse(BoxReader* reader) so this code can be a little more object oriented. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.cc File media/mp4/box_reader.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:8: #include "media/mp4/rcheck.h" alphabetical order https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:10: #include <string.h> system headers after box_reader.h https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:91: } else { drop else to avoid a level of indent https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:141: DCHECK(Read4(&size) && Read(&type_)); I believe you want CHECK here and below. Otherwise these won't get called in release builds. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:143: if (size == 0) { Break this if/else chain up by early returning more. This should allow you to reduce the indentation level by 1 and make the code easier to follow. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:149: if (size < 16) { remove {} https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:153: if (size < 8) { remove {} https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h File media/mp4/box_reader.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:13: #include <map> System headers go at the top https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:28: bool Read(int8* v) WARN_UNUSED_RESULT; Add size to signatures like Read4 does below. It makes it easier to see at call sites how much data is being read. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:55: template<typename T> bool Read_(T* t) WARN_UNUSED_RESULT; Remove _. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:63: // if an intact, complete box was not available in the buffer. If '*err' is '*err' -> |*err| You should also add information about the expected lifetime of |buf| relative to the lifetime of the BoxReader object. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:73: // true. The semantics of '*err' are the same as above. '*err' -> |*err| https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:93: template<typename T> bool ReadChildren(std::vector<T>* children) nit: I believe the line break should be after the ( and then you can put the WARN_UNUSED_RESULT on the same line as the ). Apply this here and below. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:144: auto itr = children_.find(child_type); You might want to verify that this construct builds on all our current platforms. You may have to be explicit about the iterator type here. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:147: RCHECK(Parse(&itr->second, child)); Ideally I'd like to see this become RCHECK(child->Parse(&itr->second)); https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc File media/mp4/cenc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc#newc... media/mp4/cenc.cc:17: bool Parse(size_t iv_size, FrameCENCInfo* cenc, BufferReader* r) { r -> reader https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc#newc... media/mp4/cenc.cc:38: size_t sz = 0; sz -> size https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.h File media/mp4/cenc.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.h#newcode9 media/mp4/cenc.h:9: #include <vector> include order https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/fourccs.h File media/mp4/fourccs.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/fourccs.h#ne... media/mp4/fourccs.h:10: typedef uint32 FourCC; How about making this an enum and putting it within the mp4 namespace? https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:53: } else { early return and drop else https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:80: has_video_(false) {} } on next line. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:115: if (state_ != kParsingBoxes || kEmittingSamples) why is this here? It looks wrong and isn't doing anything. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:158: if (!audio_buffers.empty()) DCHECK(audio_cb_.Run(audio_buffers)); You don't want DCHECK here. If the callback returns false you want to return false from this function. if (!audio_buffers.empty() && (audio_cb_.is_null() || !audio_cb_.Run(audio_buffers))) return false; This also makes sure you don't accidentally return audio data when you aren't expected to. This applies to video below as well. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:169: scoped_ptr<BoxReader> r(BoxReader::ReadTopLevelBox(buf, size, err)); r -> reader https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:205: AudioDecoderConfig ac; ac -> audio_config vc -> video_config https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:301: } else if (runs_.SampleValid()) { remove else https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:336: runs_.AdvanceSample(); looks like you can remove the else below and then just have a single runs_.AdvanceSample() https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:341: ChangeState(kParsingBoxes); reverse condition in if above, change state and return early. Then you can de-indent the large block above. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:360: if (type != FOURCC_MDAT) Add {} since the body spans multiple lines. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... File media/mp4/mp4_stream_parser.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:22: } Remove }. Lets keep the MP4StreamParser in the mp4 namespace. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:24: using mp4::BoxReader; remove since everything is in the mp4 namespace now. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:46: // '*buf==NULL' and '*size==0'. Use || when referring to parameters. ie |buf| == NULL https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:99: bool ParseMoov(BoxReader* r); r -> reader here and elsewhere https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:122: // 'moof_head_' is the offset of the start of the most recently parsed moof Use || https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:126: // 'mdat_tail_' is the offset of the end of the Use || https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:14: return base::TimeDelta::FromMicroseconds( nit: Add a DCHECK_LT(numer, xxx) to alert us if we encounter a value that causes an overflow. I'm assuming numer is not likely to be very large normally. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:27: const std::vector<T>& ts, nit: more descriptive name please. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:159: if (RunValid()) { nit: reverse check and return early. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:170: // XXX frame_cenc_info_ Change this to a TODO? https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.h:78: uint32 track_id() const { return run_itr_->track_id; } Move all these impls into cc file and add DCHECKS for RunValid(), SampleValid(), or is_encrypted() as appropriate.
Is there a reason that the directory name is "mp4" instead of "iso" or "iso_bmff"?
On 2012/06/06 20:43:36, ddorwin wrote: > Is there a reason that the directory name is "mp4" instead of "iso" or > "iso_bmff"? A frustrated author might hit up SVNweb to get some insight about why their file might play. (Generally optimistic, I know, but I've done it myself from time to time.) In this case, the file URL will probably be ".mp4", and the MIME type is required to be "video/mp4", so I felt that having the folder also be called "mp4" would be the least surprising choice. Also ISOBMFFStreamParser is top-heavy and IsoBmffStreamParser just makes me sad. :)
Here are all the minor updates. The two larger patches - to make Read() unpolymorphic and to make Parse() a virtual method - are forthcoming. https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... media/filters/chunk_demuxer.cc:15: #include "media/mp4/mp4_stream_parser.h" On 2012/06/06 17:32:39, acolwell wrote: > includes should be alphabetical Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/chunk_de... media/filters/chunk_demuxer.cc:47: &kVP8CodecInfo, On 2012/06/06 17:32:39, acolwell wrote: > remove. VP8 should not be allowed in an audio only mimetype Done. Sorry, copy and paste error. https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/source_b... File media/filters/source_buffer.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/filters/source_b... media/filters/source_buffer.cc:10: #include "media/mp4/mp4_stream_parser.h" On 2012/06/06 17:32:39, acolwell wrote: > Remove. Object not used in this file. Please remove the webm_stream_parser.h > too. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc File media/mp4/avc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newcode8 media/mp4/avc.cc:8: #include <algorithm> On 2012/06/06 17:32:39, acolwell wrote: > nit: system header files come right after avc.h Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:19: bool Parse(BoxReader* r, AVCDecoderConfigurationRecord* avcc) { On 2012/06/06 17:32:39, acolwell wrote: > nit: rename r -> reader & avcc to something more descriptive like avc_config Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:19: bool Parse(BoxReader* r, AVCDecoderConfigurationRecord* avcc) { On 2012/06/06 17:32:39, acolwell wrote: > Forward declared in avc.h like the other methods. Doing this creates an include loop with box_definitions.h (where it's currently declared). This problem will go away in the forthcoming patch to make Parse() a virtual method. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:21: RCHECK(r->Read(&avcc->version) && avcc->version == 1 && On 2012/06/06 17:32:39, acolwell wrote: > Change read signatures to include size. ie Read1Byte(), Read4Bytes() In progress. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:59: uint32 nal_size = (((uint32) buf[pos]) << 24) On 2012/06/06 17:32:39, acolwell wrote: > use static_cast<> Done. (Changed to match style used in BufferReader::Read.) https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:69: bool ConvertAVCCToAnnexB(uint8 length_size, std::vector<uint8>* buffer) { On 2012/06/06 17:32:39, acolwell wrote: > use int for length_size Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.cc#newco... media/mp4/avc.cc:82: uint32 nal_size = temp[pos]; On 2012/06/06 17:32:39, acolwell wrote: > use int Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h File media/mp4/avc.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode16 media/mp4/avc.h:16: uint8 version; On 2012/06/06 17:32:39, acolwell wrote: > nit: use int here and below. In general Chrome prefers use of regular int > instead of unsigned types and assumes int is at least 32 bits. I'm working on converting the polymorphic Read() functions into separate calls for each site to make the structure of the box more visible within the parsing functions, but I'm not in favor of arbitrarily destroying the type symmetry between the spec and the header by making everything an int. Here's why: Currently, the process for adding or verifying a box is: - Check to make sure that the type names in the struct match those in the spec - Check to make sure the call to Read() is in the right spot - Debugging? Just print the bytes of the structure and find out where you misregistered. There are a few cases where the logic gets more complex, but for most fields, the two-step verification is all that's required. It seems to work well, too; we had relatively few bugs in the parsing code after writing it. Using ints wherever possible would alter this process: - Look at the source type, and mentally translate it to the destination type. - Is it a uint32? Decide whether it's reasonable to assume that the value will always fit in the positive half of an int32. If it doesn't, decide whether to stick it in an int64 or a uint32. If it does, maybe write a test or add an RCHECK/DCHECK at the read site to ensure that it does, unless we decide the parameter is one we don't actually care that much about. Unless we might ever do a right shift on it, in which case we have to be careful to cast to a uint before shifting or we'll get sign-extended goop in the higher bits. - Is it smaller than an int32? Then use one. Be sure to do sign extension, unless the value is a flag in which case be sure to avoid it. - Is it a uint64? Should we try to pack it into an int64 for consistency and to avoid unsignedness? Can we actually reasonably count on it fitting on a 32-bit int, and just fail files that don't? - Decide whether there was semantic value in the original type, and maybe add some comments around usage sites. Maybe record the rationale of your decision. - Write the parsing line for that, choosing from among the 20-odd necessary permutations of the read function [source size, source signedness, dest size, dest signedness], followed by any verification checks needed to ensure that you made the right packing decision. - After you're finished writing all the parsing functions, go back through and examine them to make sure that all implicitly cross-referenced values (such as track IDs on separate boxes) were chosen to have the same type on the host, and that they are always parsed and used in the same way. Add more comments whenever you get burned by a mismatch. - Debugging? Be sure to have the spec, header, and implementation files open to reverse-engineer the layout changes and determine which bytes made it into what values in which order. Now of course this mental loop isn't *that* hard, but it's still much more than is needed when just copying types from the spec. The point of consistency in coding style is to make code easier to understand and maintain, and I think that in this case, keeping consistency with the spec does that better than keeping consistency with code in other modules. Thoughts? https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode32 media/mp4/avc.h:32: std::vector<SPS> spses; On 2012/06/06 17:32:39, acolwell wrote: > nit: How about sps_list & pps_list? or xxx_vector? Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc.h#newcode41 media/mp4/avc.h:41: ChannelLayout ConvertAACChannelCountToChannelLayout(uint16 count); On 2012/06/06 17:32:39, acolwell wrote: > nit: change count to int Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest.cc File media/mp4/avc_unittest.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:5: #include "base/basictypes.h" On 2012/06/06 17:32:39, acolwell wrote: > Group w/ other base includes Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/avc_unittest... media/mp4/avc_unittest.cc:7: #include "string.h" On 2012/06/06 17:32:39, acolwell wrote: > Use <> & place before all other includes. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:15: bool Parse(BoxReader* r, FileType* type) { On 2012/06/06 17:32:39, acolwell wrote: > nit: r -> reader here and elsewhere I'd argue that in this file, the reduced visual density from the shorter variable name improves readability, and since 'r' is used exactly the same way throughout the entire file, there is no loss of clarity. I'll change it if you're unconvinced by that ;) https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:56: : default_sample_info_size(0), sample_count(0) {} On 2012/06/06 17:32:39, acolwell wrote: > nit: } on next line Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:83: : is_encrypted(false), default_iv_size(0) {} On 2012/06/06 17:32:39, acolwell wrote: > nit: } on next line. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:128: r->Skip(10) && // reserved On 2012/06/06 17:32:39, acolwell wrote: > WDYT about renaming this to SkipBytes so the units are clearer? Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.cc:455: + sample_flags_present + sample_composition_time_offsets_present; On 2012/06/06 17:32:39, acolwell wrote: > nit: + goes on the end of the previous line and this indentation is wrong. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... File media/mp4/box_definitions.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:7: #pragma once On 2012/06/06 17:32:39, acolwell wrote: > remove Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:12: #include <string> On 2012/06/06 17:32:39, acolwell wrote: > goes before non-system includes Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:14: #define ARRAY_LENGTH(x) (sizeof(x) / sizeof(*(x))) On 2012/06/06 17:32:39, acolwell wrote: > remove. Use arraysize() from basictypes.h instead Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_definiti... media/mp4/box_definitions.h:316: bool Parse(BoxReader* r, T* box); \ On 2012/06/06 17:32:39, acolwell wrote: > WDYT about bool T::Parse(BoxReader* reader) so this code can be a little more > object oriented. I'm provisionally cool with that, and will give it a shot in the next review bundle. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.cc File media/mp4/box_reader.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:8: #include "media/mp4/rcheck.h" On 2012/06/06 17:32:39, acolwell wrote: > alphabetical order Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:10: #include <string.h> On 2012/06/06 17:32:39, acolwell wrote: > system headers after box_reader.h Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:91: } else { On 2012/06/06 17:32:39, acolwell wrote: > drop else to avoid a level of indent Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:141: DCHECK(Read4(&size) && Read(&type_)); On 2012/06/06 17:32:39, acolwell wrote: > I believe you want CHECK here and below. Otherwise these won't get called in > release builds. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:143: if (size == 0) { On 2012/06/06 17:32:39, acolwell wrote: > Break this if/else chain up by early returning more. This should allow you to > reduce the indentation level by 1 and make the code easier to follow. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:149: if (size < 16) { On 2012/06/06 17:32:39, acolwell wrote: > remove {} Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.c... media/mp4/box_reader.cc:153: if (size < 8) { On 2012/06/06 17:32:39, acolwell wrote: > remove {} Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h File media/mp4/box_reader.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:13: #include <map> On 2012/06/06 17:32:39, acolwell wrote: > System headers go at the top Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:63: // if an intact, complete box was not available in the buffer. If '*err' is On 2012/06/06 17:32:39, acolwell wrote: > '*err' -> |*err| > > You should also add information about the expected lifetime of |buf| relative to > the lifetime of the BoxReader object. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/box_reader.h... media/mp4/box_reader.h:73: // true. The semantics of '*err' are the same as above. On 2012/06/06 17:32:39, acolwell wrote: > '*err' -> |*err| Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc File media/mp4/cenc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc#newc... media/mp4/cenc.cc:17: bool Parse(size_t iv_size, FrameCENCInfo* cenc, BufferReader* r) { On 2012/06/06 17:32:39, acolwell wrote: > r -> reader Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.cc#newc... media/mp4/cenc.cc:38: size_t sz = 0; On 2012/06/06 17:32:39, acolwell wrote: > sz -> size Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.h File media/mp4/cenc.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/cenc.h#newcode9 media/mp4/cenc.h:9: #include <vector> On 2012/06/06 17:32:39, acolwell wrote: > include order Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/fourccs.h File media/mp4/fourccs.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/fourccs.h#ne... media/mp4/fourccs.h:10: typedef uint32 FourCC; On 2012/06/06 17:32:39, acolwell wrote: > How about making this an enum and putting it within the mp4 namespace? Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:53: } else { On 2012/06/06 17:32:39, acolwell wrote: > early return and drop else Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:80: has_video_(false) {} On 2012/06/06 17:32:39, acolwell wrote: > } on next line. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:115: if (state_ != kParsingBoxes || kEmittingSamples) On 2012/06/06 17:32:39, acolwell wrote: > why is this here? It looks wrong and isn't doing anything. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:158: if (!audio_buffers.empty()) DCHECK(audio_cb_.Run(audio_buffers)); On 2012/06/06 17:32:39, acolwell wrote: > You don't want DCHECK here. If the callback returns false you want to return > false from this function. > > if (!audio_buffers.empty() && > (audio_cb_.is_null() || !audio_cb_.Run(audio_buffers))) > return false; > > This also makes sure you don't accidentally return audio data when you aren't > expected to. This applies to video below as well. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:169: scoped_ptr<BoxReader> r(BoxReader::ReadTopLevelBox(buf, size, err)); On 2012/06/06 17:32:39, acolwell wrote: > r -> reader Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:205: AudioDecoderConfig ac; On 2012/06/06 17:32:39, acolwell wrote: > ac -> audio_config > vc -> video_config Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:301: } else if (runs_.SampleValid()) { On 2012/06/06 17:32:39, acolwell wrote: > remove else Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:336: runs_.AdvanceSample(); On 2012/06/06 17:32:39, acolwell wrote: > looks like you can remove the else below and then just have a single > runs_.AdvanceSample() Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:341: ChangeState(kParsingBoxes); On 2012/06/06 17:32:39, acolwell wrote: > reverse condition in if above, change state and return early. Then you can > de-indent the large block above. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.cc:360: if (type != FOURCC_MDAT) On 2012/06/06 17:32:39, acolwell wrote: > Add {} since the body spans multiple lines. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... File media/mp4/mp4_stream_parser.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:22: } On 2012/06/06 17:32:39, acolwell wrote: > Remove }. Lets keep the MP4StreamParser in the mp4 namespace. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:24: using mp4::BoxReader; On 2012/06/06 17:32:39, acolwell wrote: > remove since everything is in the mp4 namespace now. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:46: // '*buf==NULL' and '*size==0'. On 2012/06/06 17:32:39, acolwell wrote: > Use || when referring to parameters. ie |buf| == NULL Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:99: bool ParseMoov(BoxReader* r); On 2012/06/06 17:32:39, acolwell wrote: > r -> reader here and elsewhere Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:122: // 'moof_head_' is the offset of the start of the most recently parsed moof On 2012/06/06 17:32:39, acolwell wrote: > Use || Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/mp4_stream_p... media/mp4/mp4_stream_parser.h:126: // 'mdat_tail_' is the offset of the end of the On 2012/06/06 17:32:39, acolwell wrote: > Use || Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:14: return base::TimeDelta::FromMicroseconds( On 2012/06/06 17:32:39, acolwell wrote: > nit: Add a DCHECK_LT(numer, xxx) to alert us if we encounter a value that causes > an overflow. I'm assuming numer is not likely to be very large normally. Overflow happens around 300,000 years. But since it can help catch programming errors, done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:27: const std::vector<T>& ts, On 2012/06/06 17:32:39, acolwell wrote: > nit: more descriptive name please. Oops, that was dead code. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:159: if (RunValid()) { On 2012/06/06 17:32:39, acolwell wrote: > nit: reverse check and return early. Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.cc:170: // XXX frame_cenc_info_ On 2012/06/06 17:32:39, acolwell wrote: > Change this to a TODO? Done. https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10536014/diff/1/media/mp4/track_run_it... media/mp4/track_run_iterator.h:78: uint32 track_id() const { return run_itr_->track_id; } On 2012/06/06 17:32:39, acolwell wrote: > Move all these impls into cc file and add DCHECKS for RunValid(), SampleValid(), > or is_encrypted() as appropriate. Done.
Just a few more comments on your updates. Please do another pass on these files and replace the one and two letter abbreviated variable names with more descriptive names. I'm fine with the trex & moof style ones since those actually describe the box it represents, but sz, si, r, etc are not descriptive enough. i for iterating & array indexing are obviously ok. Looking forward to seeing the Read() & Parse() changes. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.h File media/mp4/avc.h (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.h#newcode16 media/mp4/avc.h:16: uint8 version; On 2012/06/07 16:33:54, strobe wrote: > Thoughts? Take it easy. This is the first file I reviewed and didn't realize that this was a format specific structure. I agree that it makes sense to keep these matching the spec which is why you don't see as many of these "use int" comments later. In cases like simple iteration on quantities known to be smaller than 32 bits int is preferred. http://codereview.chromium.org/10536014/diff/1/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/box_definitions.cc#n... media/mp4/box_definitions.cc:15: bool Parse(BoxReader* r, FileType* type) { On 2012/06/07 16:33:54, strobe wrote: > On 2012/06/06 17:32:39, acolwell wrote: > > nit: r -> reader here and elsewhere > > I'd argue that in this file, the reduced visual density from the shorter > variable name improves readability, and since 'r' is used exactly the same way > throughout the entire file, there is no loss of clarity. > > I'll change it if you're unconvinced by that ;) I'm unconvinced. :) Please change it here and in all other files. http://codereview.chromium.org/10536014/diff/41/media/mp4/box_reader_unittest.cc File media/mp4/box_reader_unittest.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/box_reader_unittest... media/mp4/box_reader_unittest.cc:87: TEST_F(BoxReaderTest, BoringTest) { Please use a more descriptive name http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.cc File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.c... media/mp4/mp4_stream_parser.cc:211: moov_->header.timescale); nit:indent http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.c... media/mp4/mp4_stream_parser.cc:214: moov_->header.timescale); nit: indent http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.cc File media/mp4/offset_byte_queue.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.c... media/mp4/offset_byte_queue.cc:40: if (offset >= head() && offset < tail()) { nit: Flip condition so the early return is the error path? http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.h File media/mp4/offset_byte_queue.h (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.h... media/mp4/offset_byte_queue.h:48: size_t head() { return head_; } Is there any reason these and other values in here need to be unsigned? I believe int or int64 will do? I'm asking because it is causing you to have to append a u after every constant that is compared with it. http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue_u... File media/mp4/offset_byte_queue_unittest.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue_u... media/mp4/offset_byte_queue_unittest.cc:18: for (uint32 i = 0; i < 256; i++) { use int http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.cc File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.... media/mp4/track_run_iterator.cc:16: DCHECK_LT(numer, numer * base::Time::kMicrosecondsPerSecond); Hmm.. I was hoping this could be a single line check. Shouldn't something like DCHECK_LT(std::abs(numer), kint64max / base::Time::kMicrosecondsPerSecond) be sufficient? http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.... media/mp4/track_run_iterator.cc:38: SampleInfo* si) { si -> sample_info
A few more tests still needed, and the common encryption stuff is still pending, but this should be better. The object-oriented approach works well, although I couldn't quite eliminate all the template functions. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.cc File media/mp4/avc.cc (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.cc#newcode14 media/mp4/avc.cc:14: const static size_t kAnnexBStartCodeSize = 4; On 2012/06/06 17:32:39, acolwell wrote: > nit: use int Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.cc#newcode55 media/mp4/avc.cc:55: bool ConvertAVCCToAnnexBInPlaceForLengthSize4(size_t buf_size, uint8* buf) { On 2012/06/06 17:32:39, acolwell wrote: > Add static if only used in this file. Otherwise add forward decl to .h. Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.h File media/mp4/avc.h (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/avc.h#newcode16 media/mp4/avc.h:16: uint8 version; On 2012/06/07 19:44:02, acolwell wrote: > On 2012/06/07 16:33:54, strobe wrote: > > Thoughts? > Take it easy. This is the first file I reviewed and didn't realize that this was > a format specific structure. I agree that it makes sense to keep these matching > the spec which is why you don't see as many of these "use int" comments later. Contritely: I apologize. I see that that message may have come off as more hostile than it was intended to be. With wry humor: I think the Elcor might have had the right idea. For email, at least. Suddenly concerned: Oh, I hope you've played Mass Effect, because if you haven't, this joke won't land at all. > In cases like simple iteration on quantities known to be smaller than 32 bits > int is preferred. Got it, I'll give everything another pass to see where I can apply this without issue. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc File media/mp4/avc_unittest.cc (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc#newc... media/mp4/avc_unittest.cc:12: using media::mp4::ConvertAVCCToAnnexB; On 2012/06/06 17:32:39, acolwell wrote: > Remove and put tests in media::mp4 namespace Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc#newc... media/mp4/avc_unittest.cc:20: class AVCCConversionTest : public testing::TestWithParam<uint32> { On 2012/06/06 17:32:39, acolwell wrote: > Use int here and various other places below. Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc#newc... media/mp4/avc_unittest.cc:24: for (uint32 i = 1; i < length_size; i++) { On 2012/06/06 17:32:39, acolwell wrote: > nit: remove {} Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc#newc... media/mp4/avc_unittest.cc:30: for (uint32 i = 1; i < length_size; i++) { On 2012/06/06 17:32:39, acolwell wrote: > nit: remove {} Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/avc_unittest.cc#newc... media/mp4/avc_unittest.cc:51: // This actually still works for one-byte sizes. On 2012/06/06 17:32:39, acolwell wrote: > I'm not sure what this comment is trying to tell me. Please reword. Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/box_reader.h File media/mp4/box_reader.h (right): http://codereview.chromium.org/10536014/diff/1/media/mp4/box_reader.h#newcode28 media/mp4/box_reader.h:28: bool Read(int8* v) WARN_UNUSED_RESULT; On 2012/06/06 17:32:39, acolwell wrote: > Add size to signatures like Read4 does below. It makes it easier to see at call > sites how much data is being read. Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/box_reader.h#newcode55 media/mp4/box_reader.h:55: template<typename T> bool Read_(T* t) WARN_UNUSED_RESULT; On 2012/06/06 17:32:39, acolwell wrote: > Remove _. Done. http://codereview.chromium.org/10536014/diff/1/media/mp4/box_reader.h#newcode147 media/mp4/box_reader.h:147: RCHECK(Parse(&itr->second, child)); On 2012/06/06 17:32:39, acolwell wrote: > Ideally I'd like to see this become > RCHECK(child->Parse(&itr->second)); Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/box_reader_unittest.cc File media/mp4/box_reader_unittest.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/box_reader_unittest... media/mp4/box_reader_unittest.cc:87: TEST_F(BoxReaderTest, BoringTest) { On 2012/06/07 19:44:02, acolwell wrote: > Please use a more descriptive name Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.cc File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.c... media/mp4/mp4_stream_parser.cc:211: moov_->header.timescale); On 2012/06/07 19:44:02, acolwell wrote: > nit:indent Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/mp4_stream_parser.c... media/mp4/mp4_stream_parser.cc:214: moov_->header.timescale); On 2012/06/07 19:44:02, acolwell wrote: > nit: indent Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.cc File media/mp4/offset_byte_queue.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.c... media/mp4/offset_byte_queue.cc:40: if (offset >= head() && offset < tail()) { On 2012/06/07 19:44:02, acolwell wrote: > nit: Flip condition so the early return is the error path? Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.h File media/mp4/offset_byte_queue.h (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue.h... media/mp4/offset_byte_queue.h:48: size_t head() { return head_; } On 2012/06/07 19:44:02, acolwell wrote: > Is there any reason these and other values in here need to be unsigned? I > believe int or int64 will do? I'm asking because it is causing you to have to > append a u after every constant that is compared with it. Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue_u... File media/mp4/offset_byte_queue_unittest.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/offset_byte_queue_u... media/mp4/offset_byte_queue_unittest.cc:18: for (uint32 i = 0; i < 256; i++) { On 2012/06/07 19:44:02, acolwell wrote: > use int Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.cc File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.... media/mp4/track_run_iterator.cc:16: DCHECK_LT(numer, numer * base::Time::kMicrosecondsPerSecond); On 2012/06/07 19:44:02, acolwell wrote: > Hmm.. I was hoping this could be a single line check. Shouldn't something like > DCHECK_LT(std::abs(numer), kint64max / base::Time::kMicrosecondsPerSecond) be > sufficient? Done. http://codereview.chromium.org/10536014/diff/41/media/mp4/track_run_iterator.... media/mp4/track_run_iterator.cc:38: SampleInfo* si) { On 2012/06/07 19:44:02, acolwell wrote: > si -> sample_info Done.
This is looking really good and I think this is almost ready. 1. Everything marked as "lint nit" was detected by using the links in the Lint column in patch file list. Please fix these so we can make the tool happy. The no newline at eof are bogus and always happen w/ new files. 2. File subbugs under http://crbug.com/129072 for any significant TODOS you have and include the crbug URL in the comments. 3. Submit this patch to the try bots (git try) so we can smoke out any compiler errors on the various platforms. Once all this is done I think you'll be in good shape to land this change. Thanks for your patience and hard work. http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc.cc File media/mp4/avc.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc.cc#newcode14 media/mp4/avc.cc:14: const static uint8 kAnnexBStartCode[] = {0, 0, 0, 1}; lint nit: static comes first here and below http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc.cc#newcode38 media/mp4/avc.cc:38: } else { drop else http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc.cc#newcode80 media/mp4/avc.cc:80: lint nit: remove blank line. http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc_unittest.cc File media/mp4/avc_unittest.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/avc_unittest.cc#n... media/mp4/avc_unittest.cc:15: const static uint8 kNALU1[] = { 0x01, 0x02, 0x03 }; lint nit: static comes first http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:281: AVCDecoderConfigurationRecord::AVCDecoderConfigurationRecord() {}; lint nit: remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:282: AVCDecoderConfigurationRecord::~AVCDecoderConfigurationRecord() {}; lint nit: remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:286: RCHECK(readereader->Read1(&version) && version == 1 && Oops :) readereader -> reader http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:342: // TODO(strobe): Widevine test files do not follow spec here; check disabled lint nit: 2 spaces after // here and below http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:369: samplerate >>= 16; How does mp4 handle 96kHz audio? http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:477: Movie::Movie() {}; lint nit: remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:478: Movie::~Movie() {}; lint nit: remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.h File media/mp4/box_definitions.h (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.h... media/mp4/box_definitions.h:8: #include <string> lint nit: #include <vector> http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.h... media/mp4/box_definitions.h:26: #define DECLARE_BOX(T) \ WDYT about DECLARE_BOX_METHODS instead? http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.h... media/mp4/box_definitions.h:36: DECLARE_BOX(FileType); WDYT about putting these at the top of the decls instead of always at the bottom? I don't have a strong feeling about this, but it feels nicer to have the common stuff done first followed by the box differentiating stuff. http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.cc File media/mp4/box_reader.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.cc#new... media/mp4/box_reader.cc:10: #include <string.h> lint nit: Apparently C system headers must come before C++ system headers. string.h must go before <algorithm> http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.cc#new... media/mp4/box_reader.cc:14: #include "media/mp4/box_definitions.h" lint nit: alphabetical order. http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.cc#new... media/mp4/box_reader.cc:80: : BufferReader(buf, size), scanned_(false) {}; lint nit: on next line and remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.cc#new... media/mp4/box_reader.cc:93: BoxReader* r = new BoxReader(buf, buf_size); r-> reader. http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.h File media/mp4/box_reader.h (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.h#newc... media/mp4/box_reader.h:21: struct Box { lint nit: needs virtual destructor. http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.h#newc... media/mp4/box_reader.h:125: private: lint nit: add a blank before private: http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader.h#newc... media/mp4/box_reader.h:130: // stream, and that the box must not be used further. If (result && !err), Does result mean the return value? Add || around err http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader_unitte... File media/mp4/box_reader_unittest.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader_unitte... media/mp4/box_reader_unittest.cc:17: const static uint8 kTestBox[] = { lint nit: static comes first. http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_reader_unitte... media/mp4/box_reader_unittest.cc:100: EXPECT_EQ((int32) 0xf90a0b0c, box.d); lint nit: use static_cast<> here and next line. http://codereview.chromium.org/10536014/diff/5005/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/cenc.cc#newcode13 media/mp4/cenc.cc:13: FrameCENCInfo::FrameCENCInfo() {}; lint nit: remove ; here and next line. http://codereview.chromium.org/10536014/diff/5005/media/mp4/fourccs.h File media/mp4/fourccs.h (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/fourccs.h#newcode89 media/mp4/fourccs.h:89: buf[2] = (fourcc >> 8 ) & 0xff; lint nit: Remove spaces before ). http://codereview.chromium.org/10536014/diff/5005/media/mp4/fourccs.h#newcode94 media/mp4/fourccs.h:94: lint nit: remove blank line. http://codereview.chromium.org/10536014/diff/5005/media/mp4/mp4_stream_parser... File media/mp4/mp4_stream_parser_unittest.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/mp4_stream_parser... media/mp4/mp4_stream_parser_unittest.cc:13: #include "base/logging.h" lint nit: alphabetical order. http://codereview.chromium.org/10536014/diff/5005/media/mp4/mp4_stream_parser... media/mp4/mp4_stream_parser_unittest.cc:18: #include "media/base/test_data_util.h" lint nit: alphabetical order. http://codereview.chromium.org/10536014/diff/5005/media/mp4/offset_byte_queue.cc File media/mp4/offset_byte_queue.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/offset_byte_queue... media/mp4/offset_byte_queue.cc:13: OffsetByteQueue::~OffsetByteQueue() {}; lint nit: remove ; http://codereview.chromium.org/10536014/diff/5005/media/mp4/track_run_iterato... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/track_run_iterato... media/mp4/track_run_iterator.cc:20: const static uint32 kSampleIsDifferenceSampleFlagMask = 0x10000; lint nit: static comes first. http://codereview.chromium.org/10536014/diff/5005/media/mp4/track_run_iterator.h File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/track_run_iterato... media/mp4/track_run_iterator.h:7: lint nit: add #include<vector>
OK, code now builds, compiles, and passes tests (except for the ones that require test data files which aren't yet in the patch) on all arches. A complication: fnk@, who created the video samples used in mp4_stream_parser_unittest.cc, noted that the header format isn't finalized and suggested that we wait to check them in. Do you recommend that we proceed without that test, and re-add it along with encryption support; try to generate some unencrypted test samples by hand; or wait until the samples are ready before committing this patch? (Bug numbering pending this response.) https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc.cc File media/mp4/avc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc.cc#ne... media/mp4/avc.cc:14: const static uint8 kAnnexBStartCode[] = {0, 0, 0, 1}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: static comes first here and below Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc.cc#ne... media/mp4/avc.cc:38: } else { On 2012/06/08 16:10:38, acolwell wrote: > drop else Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc.cc#ne... media/mp4/avc.cc:80: On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove blank line. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc_unitt... File media/mp4/avc_unittest.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/avc_unitt... media/mp4/avc_unittest.cc:15: const static uint8 kNALU1[] = { 0x01, 0x02, 0x03 }; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: static comes first Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:281: AVCDecoderConfigurationRecord::AVCDecoderConfigurationRecord() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:282: AVCDecoderConfigurationRecord::~AVCDecoderConfigurationRecord() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:286: RCHECK(readereader->Read1(&version) && version == 1 && On 2012/06/08 16:10:38, acolwell wrote: > Oops :) readereader -> reader Heh. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:342: // TODO(strobe): Widevine test files do not follow spec here; check disabled On 2012/06/08 16:10:38, acolwell wrote: > lint nit: 2 spaces after // here and below Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:369: samplerate >>= 16; On 2012/06/08 16:10:38, acolwell wrote: > How does mp4 handle 96kHz audio? Poorly. ;) BMFF supports a lot of crazy things, but this doesn't seem to be one of them (at least from the specs I have seen). It's certainly conceivable that some audio formats (e.g. AAC, DTS) might store alternate configs in either a format-specific box (ESDS) or simply plop them inline in a stream, and expect that the decoder will override the values in the container somehow. As for real-world examples, FFmpeg at least refuses to create an MP4 with 96k audio. If we need to, we can pull apart the first few bytes of the codec or the ESDS (if present) to grab the "real" information, but I'd prefer to avoid bending over backwards to support non-spec files. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:477: Movie::Movie() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.cc:478: Movie::~Movie() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... File media/mp4/box_definitions.h (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.h:8: #include <string> On 2012/06/08 16:10:38, acolwell wrote: > lint nit: #include <vector> Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.h:26: #define DECLARE_BOX(T) \ On 2012/06/08 16:10:38, acolwell wrote: > WDYT about DECLARE_BOX_METHODS instead? Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_defin... media/mp4/box_definitions.h:36: DECLARE_BOX(FileType); On 2012/06/08 16:10:38, acolwell wrote: > WDYT about putting these at the top of the decls instead of always at the > bottom? I don't have a strong feeling about this, but it feels nicer to have the > common stuff done first followed by the box differentiating stuff. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... File media/mp4/box_reader.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.cc:10: #include <string.h> On 2012/06/08 16:10:38, acolwell wrote: > lint nit: Apparently C system headers must come before C++ system headers. > string.h must go before <algorithm> Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.cc:14: #include "media/mp4/box_definitions.h" On 2012/06/08 16:10:38, acolwell wrote: > lint nit: alphabetical order. Apparently I never learned my ABCs. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.cc:80: : BufferReader(buf, size), scanned_(false) {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: on next line and remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.cc:93: BoxReader* r = new BoxReader(buf, buf_size); On 2012/06/08 16:10:38, acolwell wrote: > r-> reader. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reader.h File media/mp4/box_reader.h (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.h:21: struct Box { On 2012/06/08 16:10:38, acolwell wrote: > lint nit: needs virtual destructor. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.h:125: private: On 2012/06/08 16:10:38, acolwell wrote: > lint nit: add a blank before private: Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader.h:130: // stream, and that the box must not be used further. If (result && !err), On 2012/06/08 16:10:38, acolwell wrote: > Does result mean the return value? > Add || around err Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... File media/mp4/box_reader_unittest.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader_unittest.cc:17: const static uint8 kTestBox[] = { On 2012/06/08 16:10:38, acolwell wrote: > lint nit: static comes first. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/box_reade... media/mp4/box_reader_unittest.cc:100: EXPECT_EQ((int32) 0xf90a0b0c, box.d); On 2012/06/08 16:10:38, acolwell wrote: > lint nit: use static_cast<> here and next line. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/cenc.cc File media/mp4/cenc.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/cenc.cc#n... media/mp4/cenc.cc:13: FrameCENCInfo::FrameCENCInfo() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; here and next line. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/fourccs.h File media/mp4/fourccs.h (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/fourccs.h... media/mp4/fourccs.h:89: buf[2] = (fourcc >> 8 ) & 0xff; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: Remove spaces before ). Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/fourccs.h... media/mp4/fourccs.h:94: On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove blank line. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/mp4_strea... File media/mp4/mp4_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/mp4_strea... media/mp4/mp4_stream_parser_unittest.cc:13: #include "base/logging.h" On 2012/06/08 16:10:38, acolwell wrote: > lint nit: alphabetical order. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/mp4_strea... media/mp4/mp4_stream_parser_unittest.cc:18: #include "media/base/test_data_util.h" On 2012/06/08 16:10:38, acolwell wrote: > lint nit: alphabetical order. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/offset_by... File media/mp4/offset_byte_queue.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/offset_by... media/mp4/offset_byte_queue.cc:13: OffsetByteQueue::~OffsetByteQueue() {}; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: remove ; Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/track_run... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/track_run... media/mp4/track_run_iterator.cc:20: const static uint32 kSampleIsDifferenceSampleFlagMask = 0x10000; On 2012/06/08 16:10:38, acolwell wrote: > lint nit: static comes first. Done. https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/track_run... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10536014/diff/5005/media/mp4/track_run... media/mp4/track_run_iterator.h:7: On 2012/06/08 16:10:38, acolwell wrote: > lint nit: add #include<vector> Done.
LGTM http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10536014/diff/5005/media/mp4/box_definitions.c... media/mp4/box_definitions.cc:369: samplerate >>= 16; On 2012/06/11 18:44:21, strobe wrote: > On 2012/06/08 16:10:38, acolwell wrote: > > How does mp4 handle 96kHz audio? > > Poorly. ;) > > BMFF supports a lot of crazy things, but this doesn't seem to be one of them (at > least from the specs I have seen). > ... Ok. I'm fine with leaving this code as is for now. I agree it isn't worth jumping through hoops for this right now. http://codereview.chromium.org/10536014/diff/1032/media/mp4/mp4_stream_parser.cc File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10536014/diff/1032/media/mp4/mp4_stream_parser... media/mp4/mp4_stream_parser.cc:272: sample_table.description.video_entries[0].avcc; wow. that is quite the dereference chain. Should we break after avc_config is set?
Sorry to make a change after LGTM, but Windows was broken. This is final, media_unittests passes on all bots. (Unless there are any review comments.) http://codereview.chromium.org/10536014/diff/1032/media/mp4/mp4_stream_parser.cc File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10536014/diff/1032/media/mp4/mp4_stream_parser... media/mp4/mp4_stream_parser.cc:272: sample_table.description.video_entries[0].avcc; On 2012/06/13 14:16:06, acolwell wrote: > wow. that is quite the dereference chain. > Should we break after avc_config is set? Done. (It shouldn't make a difference, but it makes the intent clearer.) http://codereview.chromium.org/10536014/diff/8008/media/mp4/avc.cc File media/mp4/avc.cc (right): http://codereview.chromium.org/10536014/diff/8008/media/mp4/avc.cc#newcode19 media/mp4/avc.cc:19: static bool ConvertAVCToAnnexBInPlaceForLengthSize4(std::vector<uint8>* buf) { This change fixes behavior on on Windows.
On 2012/06/13 21:18:12, strobe wrote: > Sorry to make a change after LGTM, but Windows was broken. This is final, > media_unittests passes on all bots. (Unless there are any review comments.) No problem. My LGTM still stands so I'll send this to the commit queue.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/strobe@google.com/10536014/8008
Try job failure for 10536014-8008 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
strobe@ - Looks like you need to add MEDIA_EXPORT to a few classes(Box, BoxReader, BufferReader, OffsetByteQueue, AVC,AVCDecoderConfigurationRecord) to make ChromeOS happy. You can test your changes against a ChromeOS trybot by using 'git try --bot=linux_chromeos
On 2012/06/14 16:56:21, acolwell wrote: > strobe@ - Looks like you need to add MEDIA_EXPORT to a few classes(Box, > BoxReader, BufferReader, OffsetByteQueue, AVC,AVCDecoderConfigurationRecord) to > make ChromeOS happy. > > You can test your changes against a ChromeOS trybot by using 'git try > --bot=linux_chromeos The ChromeOS trybot is overloaded (some builds have been waiting 12+ hours), but the 'win' bot had the same errors on patch set 9, and it now compiles fine. Happy to wait for the results of the linux_chromeos try if needed.
On 2012/06/14 20:23:25, strobe wrote: > The ChromeOS trybot is overloaded (some builds have been waiting 12+ hours), but > the 'win' bot had the same errors on patch set 9, and it now compiles fine. > Happy to wait for the results of the linux_chromeos try if needed. wow! no need to wait. I'd say just click the Commit checkbox and let the commit-queue bots verify.
Actually I see that vrk just landed http://crrev.com/142221 which means you are going to have to rebase and stub out code for the NewMediaSegmentCB.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/strobe@google.com/10536014/3046
Change committed as 142268 |
