Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10651006:
Add Common Encryption support to BMFF, including subsample decryption. (Closed)
|
Created:
8 years, 6 months ago by strobe_ Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, acolwell GONE FROM CHROMIUM Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Common Encryption support to BMFF, including subsample decryption.
BUG=132351
TEST=AesDecryptorTest, plus manual playback in browser
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148453
Patch Set 1 #Patch Set 2 : Remove pipeline integration test (private file) #Patch Set 3 : Remove references to non-public encrypted files in tests #
Total comments: 126
Patch Set 4 : Address review comments. #Patch Set 5 : Fix another case issue #
Total comments: 53
Patch Set 6 : Add track_run_iterator_unittest #Patch Set 7 : Add comment regarding initData #Patch Set 8 : Add comment regarding initData #Patch Set 9 : Fix lint errors #Patch Set 10 : Add wrong subsample size test #
Total comments: 106
Patch Set 11 : Rebase #Patch Set 12 : Address review comments #Patch Set 13 : Address phantom comments #
Total comments: 2
Patch Set 14 : Fix rebase woes #Patch Set 15 : Rebase #Patch Set 16 : Convert key_id to string #
Total comments: 18
Patch Set 17 : More review comments #Patch Set 18 : Use reinterpret_cast for string creation #
Total comments: 11
Patch Set 19 : Hopefully-final review comments #Patch Set 20 : Rebase #Patch Set 21 : Satisfy mac_rel buildbot #
Messages
Total messages: 22 (0 generated)
xhwang and fgalligan, please review. I'll have some comments later today.
I'm not really qualified to review the ISO/MP4 changes and that code is pretty complex. Did you have an ISO expert review the original CL? Comments would be welcome in areas where, for example, there are non-obvious additions of certain values to others to create another. http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:22: DecryptConfig(const uint8* key_id, int key_id_size, Do we really need two constructors - there is a lot of duplication in the .cc file. Could WebM just specify 0,0 for the other values? http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:40: const SubsampleEntry* subsamples() const { return subsamples_.get(); } Any reason not to return and store these as a vector? http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:44: SubsampleEntry* mutable_subsamples() { return subsamples_.get(); } I'm wondering why this is necessary. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:42: if (!(encryptor.Init(key, crypto::Encryptor::CTR, base::StringPiece()) && Suggest: !foo || !bar http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:49: if (input.GetDecryptConfig()->subsample_count()) { Extract function for this case please. Then test it well! http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:50: int size = 0; maybe total_subsample_size http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:51: for (int i = 0; i < input.GetDecryptConfig()->subsample_count(); i++) This would be easier to read if you had aliases for subsamples and subsample_count. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:53: if (size > input.GetDataSize()) { The input only contains encrypted bytes? Shouldn't the unencrypted bytes be counted too? Should it ever be anything other than == once we've counted all? http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:58: scoped_array<uint8> encrypted_bytes(new uint8[size]); Please explain what this code does. Copies encrypted bytes to be back-to-back then decrypts them and copies them back? http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:80: encrypted_bytes.reset(); Why explicitly here? And while encrypted_text is still valid and could still be referenced. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:82: scoped_refptr<DecoderBuffer> output = DecoderBuffer::CopyFrom( We've copied at least twice now. Are there any other ways we could do this? http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:77: static const uint8 kInitialCtr[] = { Why not call this IV? http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:90: InitCTR(); I think this is unexpected. Please call it in each test. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:158: ASSERT_TRUE(decrypted); Does this pass because you fixed up the padding or whatever such that we can no longer detect the decrypt failure? Or maybe because CTR does not fail in the same way? http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:190: TEST_F(AesDecryptorTest, WrongCBCKey) { add InitCTRSubsample(); I think you also need a WrongKey (for CTR). OR just change the function name and don't have a CBC test. http://codereview.chromium.org/10651006/diff/15001/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/box_definitions.... media/mp4/box_definitions.cc:88: RCHECK(reader->ReadFullBoxHeader() && Why all the RCHECKs? Macros are discouraged, and this one actually has a return in it, which is really unexpected. I assume there was a specific reason it was allowed in the previous CL? http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode7 media/mp4/cenc.cc:7: #include <string.h> Why is this needed? http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode25 media/mp4/cenc.cc:25: RCHECK(reader->Read1(&iv[i])); Reading one byte at a time isn't the fastest solution, especially when the spec only allows multiples of 8. Why not read in muliples of 8 (or 4 if 8 is not supported)? http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode37 media/mp4/cenc.cc:37: RCHECK(reader->Read2(&clear_bytes) && Why not eliminate 4 lines by reading into the vector element directly? http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode45 media/mp4/cenc.cc:45: size_t FrameCENCInfo::GetTotalSize() const { "Total" appears to not include the iv. Maybe the function name can be more specific. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:257: int size; buf_size would make this function easier to read. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:295: size_t expected_size = runs_.size() + expected size of what? what is runs_.size()? It seems to be the frame size less subsample headers or something. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:299: decrypt_config->mutable_subsamples()[i].clear_bytes += nalu_size_diff; Rather than mutable_subsamples, you could have an AddFooToSubsamples() function. Not sure what Foo is or if it could be made generic. Seems better than exposing a member to be modified. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:323: decrypt_config->mutable_subsamples()[0].clear_bytes += Hmm, another mutable use. I guess it's not as simple as I suggested above. What is this doing? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:80: class CompareOffset { This seems to compare specific types of offsets. Is there a better name? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:84: if (a.aux_info_total_size > 0 && a.aux_info_start_offset < a_min) The .h comment seems to say is_encrypted and aux info required are orthogonal. Here, you have replaced the former with aux_info presence, which appears to contradict that. Reading below, I learn that CENC is aux info. (Continued below.) http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:132: if (traf.auxiliary_offset.offsets.size() > j) { What? How is an iterator related to some other collection? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:133: RCHECK(traf.auxiliary_size.sample_count == trun.sample_count); I have no idea what the rest of this code is doing. It's definitely worth a comment. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:190: bool TrackRunIterator::AuxInfoRequired() { What does this function really mean? "Need to read aux info"? The comment for GetDecryptConfig() and logic below seem to imply something more than the simple name. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:192: // The only kind of aux info that we handle is CENC info, so we only cache it Seems like a function-level comment. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:197: bool TrackRunIterator::CacheAuxInfo(const uint8* buf, int size) { buf_size would make this easier to read. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:197: bool TrackRunIterator::CacheAuxInfo(const uint8* buf, int size) { Add a function-level comment that this only caches CENC. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:198: RCHECK(AuxInfoRequired() && size >= aux_info_size()); Nothing in the loop below verifies that we don't go beyond these values. There should be some type of check or we might allow a buffer overrun. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:208: RCHECK(cenc_info_[i].Parse(run_itr_->track_encryption.default_iv_size, does something verify we don't exceed info_size (parameter to reader's construction) in this Parse? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:297: scoped_ptr<DecryptConfig> TrackRunIterator::GetDecryptConfig() { What tests do we have for this? :) http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:299: cenc_info_[sample_itr_ - run_itr_->samples.begin()]; What is this math doing? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:303: (cenc_info.GetTotalSize() != static_cast<size_t>(size()))) { It's unclear how these are related. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:309: &run_itr_->track_encryption.default_kid[0], Does this code not support sample groups? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:311: cenc_info.iv, sizeof(cenc_info.iv), iv is a fixed size now, so you'l always send 16 but the whole value was not initialized. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:29: struct TrackRunInfo { I think you can define this in the .cc file since the TRI's destructor is there. Just fwd declare here. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:57: bool RunValid() const; These should be Is* http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:66: // been cached. Requires |RunValid()|. Did you see || around function names somewhere? I thought we only did that for parameters/variables. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:67: bool AuxInfoRequired(); Is* http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:69: // Cache the CENC data from the given buffer. |buf| must be a buffer starting Is AuxInfo CENC data? If so, the function name should specify this, as should the code that was calling it. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:90: // Properties of the current sample. Requires |SampleValid()|. What does requires mean? Maybe "Only valid if SampleValid()." http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:98: // Requires |is_encrypted()| and |!AuxInfoRequired()|. Same comment as above. Also, why is this only valid if auxinfo not required?
https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:238: return need_key_cb_.Run(kid.Pass(), track_encryption.default_kid.size()); For ISO, the initData is supposed to be the PSSH. Most likely, it will be specified to be all PSSHes.
https://chromiumcodereview.appspot.com/10651006/diff/15001/media/base/decrypt... File media/base/decrypt_config.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/base/decrypt... media/base/decrypt_config.cc:12: static const char kDefaultIV[] = "0000000000000000"; s/kDefaultIV/kDefaultIv/ https://chromiumcodereview.appspot.com/10651006/diff/15001/media/base/decrypt... media/base/decrypt_config.cc:13: static const int kDefaultIVSize = 16; s/kDefaultIVSize/kDefaultIvSize/ https://chromiumcodereview.appspot.com/10651006/diff/15001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/base/decrypt... media/base/decrypt_config.h:22: DecryptConfig(const uint8* key_id, int key_id_size, On 2012/06/26 06:09:19, ddorwin wrote: > Do we really need two constructors - there is a lot of duplication in the .cc > file. Could WebM just specify 0,0 for the other values? For WebM we will also be adding the HMAC parameters. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:28: Maybe add a TODO to support a key change? https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:36: static const uint8 kCBCEncryptedData[] = { s/kCBCEncryptedData/kCbcEncryptedData https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:45: static const uint8 kCTREncryptedData[] = { s/kCTREncryptedData/kCtrEncryptedData/ https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:52: // |kCTREncryptedData| is |kOriginalData|, subsampled according to kCtrSubsampleEncryptedData https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:55: static const uint8 kCTRSubsampleEncryptedData[] = { s/kCTRSubsampleEncryptedData/kCtrSubsampleEncryptedData/
http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.cc File media/base/decrypt_config.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.cc:12: static const char kDefaultIV[] = "0000000000000000"; On 2012/06/27 00:28:36, fgalligan1 wrote: > s/kDefaultIV/kDefaultIv/ Done. http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.cc:13: static const int kDefaultIVSize = 16; On 2012/06/27 00:28:36, fgalligan1 wrote: > s/kDefaultIVSize/kDefaultIvSize/ Done. http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:22: DecryptConfig(const uint8* key_id, int key_id_size, On 2012/06/26 06:09:19, ddorwin wrote: > Do we really need two constructors - there is a lot of duplication in the .cc > file. Could WebM just specify 0,0 for the other values? Done. http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:40: const SubsampleEntry* subsamples() const { return subsamples_.get(); } On 2012/06/26 06:09:19, ddorwin wrote: > Any reason not to return and store these as a vector? Done. http://codereview.chromium.org/10651006/diff/15001/media/base/decrypt_config.... media/base/decrypt_config.h:44: SubsampleEntry* mutable_subsamples() { return subsamples_.get(); } On 2012/06/26 06:09:19, ddorwin wrote: > I'm wondering why this is necessary. Addressed in later comments. Can either: - Allow updating via SetSubsamples() - Allow mutation of contents through interface - Create a new DecryptConfig() with modified parameters - Retain current mutable_subsamples() method http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:28: On 2012/06/27 00:28:36, fgalligan1 wrote: > Maybe add a TODO to support a key change? Not sure what a key change means in this context. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:42: if (!(encryptor.Init(key, crypto::Encryptor::CTR, base::StringPiece()) && On 2012/06/26 06:09:19, ddorwin wrote: > Suggest: !foo || !bar Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:49: if (input.GetDecryptConfig()->subsample_count()) { On 2012/06/26 06:09:19, ddorwin wrote: > Extract function for this case please. Then test it well! Will do in next patch set. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:50: int size = 0; On 2012/06/26 06:09:19, ddorwin wrote: > maybe total_subsample_size Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:51: for (int i = 0; i < input.GetDecryptConfig()->subsample_count(); i++) On 2012/06/26 06:09:19, ddorwin wrote: > This would be easier to read if you had aliases for subsamples and > subsample_count. Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:53: if (size > input.GetDataSize()) { On 2012/06/26 06:09:19, ddorwin wrote: > The input only contains encrypted bytes? Shouldn't the unencrypted bytes be > counted too? > Should it ever be anything other than == once we've counted all? Input contains both encrypted and unencrypted bytes. Did change to equality test. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:58: scoped_array<uint8> encrypted_bytes(new uint8[size]); On 2012/06/26 06:09:19, ddorwin wrote: > Please explain what this code does. Copies encrypted bytes to be back-to-back > then decrypts them and copies them back? Precisely. Comment added. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:80: encrypted_bytes.reset(); On 2012/06/26 06:09:19, ddorwin wrote: > Why explicitly here? And while encrypted_text is still valid and could still be > referenced. Removed. (Original reason was to minimize number of live objects on memory-constrained devices, but prefer safety over optimization.) http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:82: scoped_refptr<DecoderBuffer> output = DecoderBuffer::CopyFrom( On 2012/06/26 06:09:19, ddorwin wrote: > We've copied at least twice now. Are there any other ways we could do this? I didn't find any good ways without modifying Encryptor, but I'll take another pass. One desperate strategy would be (if I understand CTR correctly) to decrypt a buffer of zeros of equal size to the encrypted bytes, and then manually XOR the bytes in place, but this seems unwise. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:36: static const uint8 kCBCEncryptedData[] = { On 2012/06/27 00:28:36, fgalligan1 wrote: > s/kCBCEncryptedData/kCbcEncryptedData Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:45: static const uint8 kCTREncryptedData[] = { On 2012/06/27 00:28:36, fgalligan1 wrote: > s/kCTREncryptedData/kCtrEncryptedData/ Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:52: // |kCTREncryptedData| is |kOriginalData|, subsampled according to On 2012/06/27 00:28:36, fgalligan1 wrote: > kCtrSubsampleEncryptedData Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:55: static const uint8 kCTRSubsampleEncryptedData[] = { On 2012/06/27 00:28:36, fgalligan1 wrote: > s/kCTRSubsampleEncryptedData/kCtrSubsampleEncryptedData/ Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:77: static const uint8 kInitialCtr[] = { On 2012/06/26 06:09:19, ddorwin wrote: > Why not call this IV? Done. (The underlying libraries tend to treat initial IV (for CBC) and initial counter (for CTR) as separate entities, hence the original naming, but in media:: we always think of it as an IV, so.) http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:90: InitCTR(); On 2012/06/26 06:09:19, ddorwin wrote: > I think this is unexpected. Please call it in each test. Done. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:158: ASSERT_TRUE(decrypted); On 2012/06/26 06:09:19, ddorwin wrote: > Does this pass because you fixed up the padding or whatever such that we can no > longer detect the decrypt failure? Or maybe because CTR does not fail in the > same way? The latter. Our CBC decoder expects PKCS#5 padding, which can be verified to a) trim the decoded string of padding, and b) as a (very weak) integrity check. It seems like CTR doesn't have this expectation. I've put DecryptAndExpectToFail() back and added comments to make this clearer. http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:190: TEST_F(AesDecryptorTest, WrongCBCKey) { On 2012/06/26 06:09:19, ddorwin wrote: > add InitCTRSubsample(); Not sure where this should go. > I think you also need a WrongKey (for CTR). > OR just change the function name and don't have a CBC test. Changed name. (I think that's what you meant, right?) http://codereview.chromium.org/10651006/diff/15001/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/box_definitions.... media/mp4/box_definitions.cc:88: RCHECK(reader->ReadFullBoxHeader() && On 2012/06/26 06:09:19, ddorwin wrote: > Why all the RCHECKs? Macros are discouraged, and this one actually has a return > in it, which is really unexpected. > I assume there was a specific reason it was allowed in the previous CL? The alternatives were worse. ;) There are some tricky cases during parsing where we might be exposed to memory corruption or otherwise produce invalid data. We catch those by using WARN_UNUSED_RESULT on common functions that might return an error indicating that parsing should immediately halt with an error state. The hierarchical nature of BMFF means that even in this very simple parser, RCHECK gets used quite a bit. Replacing each of those instances with the corresponding four-line definition turned out to be really messy, and we also had a couple places where the messiness of the code hid logic bugs in the original effort. http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode7 media/mp4/cenc.cc:7: #include <string.h> On 2012/06/26 06:09:19, ddorwin wrote: > Why is this needed? memset(). http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode25 media/mp4/cenc.cc:25: RCHECK(reader->Read1(&iv[i])); On 2012/06/26 06:09:19, ddorwin wrote: > Reading one byte at a time isn't the fastest solution, especially when the spec > only allows multiples of 8. Why not read in muliples of 8 (or 4 if 8 is not > supported)? Given the buffer's representation as a uint8 string elsewhere, I felt it was simpler to avoid issues such as endianness conversion that would come with a more complicated read scheme. If you'd like, we could store IVs as one or two uint64s in the DecryptConfig, perform host-to-network conversion on reads, and then perform network-to-host conversion when handling the data in AesDecryptor, but this seems like it wouldn't result in much of a performance benefit overall. http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode37 media/mp4/cenc.cc:37: RCHECK(reader->Read2(&clear_bytes) && On 2012/06/26 06:09:19, ddorwin wrote: > Why not eliminate 4 lines by reading into the vector element directly? I modified the vector element to use the definition of SubsampleEntry in decrypt_config.h, which has different types from those used in the BMFF file. http://codereview.chromium.org/10651006/diff/15001/media/mp4/cenc.cc#newcode45 media/mp4/cenc.cc:45: size_t FrameCENCInfo::GetTotalSize() const { On 2012/06/26 06:09:19, ddorwin wrote: > "Total" appears to not include the iv. Maybe the function name can be more > specific. Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:238: return need_key_cb_.Run(kid.Pass(), track_encryption.default_kid.size()); On 2012/06/26 23:16:20, ddorwin wrote: > For ISO, the initData is supposed to be the PSSH. Most likely, it will be > specified to be all PSSHes. Oh, duh, that makes a lot more sense! Where's that specified? http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:295: size_t expected_size = runs_.size() + On 2012/06/26 06:09:19, ddorwin wrote: > expected size of what? > what is runs_.size()? It seems to be the frame size less subsample headers or > something. size() is the size of the current sample; IV and subsamples (the auxiliary info) is stored independently from the notional sample in CENC. I've renamed to sample_size() and sample_offset() to make this clearer. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:299: decrypt_config->mutable_subsamples()[i].clear_bytes += nalu_size_diff; On 2012/06/26 06:09:19, ddorwin wrote: > Rather than mutable_subsamples, you could have an AddFooToSubsamples() function. > Not sure what Foo is or if it could be made generic. Seems better than exposing > a member to be modified. Two reasons we need to mutate: 1. To convert each NALU from AVC format (with a length prefix) to Annex B (with an 0x00000001 start code before each NALU). This amounts to adding a constant factor to each subsample's clear byte count. 2. To inject SPS/PPS. This is a variable-sized addition to the first subsample's clear bytes count. Do you prefer adding two appropriate methods on the DecryptConfig, or addding a DecryptConfig::SetSubsamples() method and mutate a copy locally, or just making a new DecryptConfig with values pulled from the old one? http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:323: decrypt_config->mutable_subsamples()[0].clear_bytes += On 2012/06/26 06:09:19, ddorwin wrote: > Hmm, another mutable use. I guess it's not as simple as I suggested above. What > is this doing? see above http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:84: if (a.aux_info_total_size > 0 && a.aux_info_start_offset < a_min) On 2012/06/26 06:09:19, ddorwin wrote: > The .h comment seems to say is_encrypted and aux info required are orthogonal. > Here, you have replaced the former with aux_info presence, which appears to > contradict that. > > Reading below, I learn that CENC is aux info. (Continued below.) Hopefully addressed by renames. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:132: if (traf.auxiliary_offset.offsets.size() > j) { On 2012/06/26 06:09:19, ddorwin wrote: > What? How is an iterator related to some other collection? Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:133: RCHECK(traf.auxiliary_size.sample_count == trun.sample_count); On 2012/06/26 06:09:19, ddorwin wrote: > I have no idea what the rest of this code is doing. It's definitely worth a > comment. Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:190: bool TrackRunIterator::AuxInfoRequired() { On 2012/06/26 06:09:19, ddorwin wrote: > What does this function really mean? "Need to read aux info"? > The comment for GetDecryptConfig() and logic below seem to imply something more > than the simple name. Hopefully addressed by rename. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:192: // The only kind of aux info that we handle is CENC info, so we only cache it On 2012/06/26 06:09:19, ddorwin wrote: > Seems like a function-level comment. Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:197: bool TrackRunIterator::CacheAuxInfo(const uint8* buf, int size) { On 2012/06/26 06:09:19, ddorwin wrote: > Add a function-level comment that this only caches CENC. Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:197: bool TrackRunIterator::CacheAuxInfo(const uint8* buf, int size) { On 2012/06/26 06:09:19, ddorwin wrote: > buf_size would make this easier to read. Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:198: RCHECK(AuxInfoRequired() && size >= aux_info_size()); On 2012/06/26 06:09:19, ddorwin wrote: > Nothing in the loop below verifies that we don't go beyond these values. There > should be some type of check or we might allow a buffer overrun. As noted below, the RCHECK() does this. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:208: RCHECK(cenc_info_[i].Parse(run_itr_->track_encryption.default_iv_size, On 2012/06/26 06:09:19, ddorwin wrote: > does something verify we don't exceed info_size (parameter to reader's > construction) in this Parse? Yes, if a request would read past the given size, the parse function immediately returns false, which is caught by the RCHECK() (which immediately returns false to the caller, etc, etc). http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:297: scoped_ptr<DecryptConfig> TrackRunIterator::GetDecryptConfig() { On 2012/06/26 06:09:19, ddorwin wrote: > What tests do we have for this? :) Yeah, this whole class needs a big ol' test. On sky-is-falling Thursday, I was hoping to defer that until we could get more time, but since our external deadlines are slipped I'm content waiting until we have a moment to finish that up before landing this patch. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:299: cenc_info_[sample_itr_ - run_itr_->samples.begin()]; On 2012/06/26 06:09:19, ddorwin wrote: > What is this math doing? Retrieves the index of the sample to which sample_itr_ is currently pointing. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:303: (cenc_info.GetTotalSize() != static_cast<size_t>(size()))) { On 2012/06/26 06:09:19, ddorwin wrote: > It's unclear how these are related. Hopefully renames make this clearer. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:309: &run_itr_->track_encryption.default_kid[0], On 2012/06/26 06:09:19, ddorwin wrote: > Does this code not support sample groups? Correct, it does not. The restricted BMFF format used by DASH/Media Source requires that only track runs be used. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:311: cenc_info.iv, sizeof(cenc_info.iv), On 2012/06/26 06:09:19, ddorwin wrote: > iv is a fixed size now, so you'l always send 16 but the whole value was not > initialized. According to the CENC spec, an 8-byte IV shall be expanded to be 16 bytes with the less-significant 8 bytes filled with zeros, which is performed in FrameCENCInfo::Parse(). http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:66: // been cached. Requires |RunValid()|. On 2012/06/26 06:09:19, ddorwin wrote: > Did you see || around function names somewhere? I thought we only did that for > parameters/variables. I thought so, but I could be mistaken, and if it looks weird to you then it probably does to other people too, so removed. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:67: bool AuxInfoRequired(); On 2012/06/26 06:09:19, ddorwin wrote: > Is* Okay, I renamed this to AuxInfoNeedsToBeCached() based on this and the comment below, which might possibly be the clunkiest name of the year, but should resolve the confusion about the meaning of AuxInfoRequired(). WDYT? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:69: // Cache the CENC data from the given buffer. |buf| must be a buffer starting On 2012/06/26 06:09:19, ddorwin wrote: > Is AuxInfo CENC data? If so, the function name should specify this, as should > the code that was calling it. "Auxiliary information" is a generic mechanism in the amended ISO spec. Here we only use it when CENC is signaled elsewhere in the spec (or, rather, we will, once the flux between CENC, PIFF, and WideVine settles down and I can provide some reasonable assurance of correct behavior across the spectrum of files which users are likely to see), but there's no guarantee that any aux info we read is in fact CENC info, so I felt it best to separate the two names. If you feel otherwise, I'm happy to switch. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:90: // Properties of the current sample. Requires |SampleValid()|. On 2012/06/26 06:09:19, ddorwin wrote: > What does requires mean? Maybe "Only valid if SampleValid()." Done. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:98: // Requires |is_encrypted()| and |!AuxInfoRequired()|. On 2012/06/26 06:09:19, ddorwin wrote: > Same comment as above. > Also, why is this only valid if auxinfo not required? Done.
http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:8: #include <vector> nit: usually we have empty line after including c++ system files. http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:19: Can we have more docs on this? Ideally, people should be able to implement the decryptor only by looking at this file, not any specific container spec. http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:25: const SubsampleEntry* subsamples, int subsample_count); Can we just pass const std::vector<SubsampleEntry>& in? http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:38: int iv_size() const { return iv_size_; } We have a plan to replace scoped_array with std::string (or vector) for key_id and key since they are relatively small (see http://code.google.com/p/chromium/issues/detail?id=130689). Similarly, I would be fine to use std::string for the IV if it's always small. ddorwin@, what do you think? http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:31: input.GetDecryptConfig()->iv_size()); Life will be easier here if we use std::string for IV. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:71: int in_pos = 0, out_pos = 0; Prefer to declaring variables on separate lines. I am surprised that this isn't specified explicitly in the style guide, but I seldom see this pattern in chromium code. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:105: out_pos += subsample.cypher_bytes; This part is so similar to the block starting from line 72. Can we have a help function to do that? http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:108: } else { nit: we don't need this "else" since "if" will always "return". e.g. if (...) { ... return; } do_something_else http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc#newcode7 media/mp4/cenc.cc:7: #include <string.h> Use C++ headers? #include <cstring> http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:134: // container, if it is present Add period "." at the end. http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:11: #include "media/base/decrypt_config.h" Forward declaration of DecryptConfig instead of including the header file. http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:69: // Cache the CENC data from the given buffer. |buf| must be a buffer starting Cache->Caches
http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:92: void InitCBC() { Change to InitCbc(), to be consistent with kCbcEncryptedData, and the coding style: Prefer to capitalize well-known abbreviations as if they were words: HttpUrl instead of HTTPURL. (http://dev.chromium.org/developers/coding-style#Naming) Please also change similar names in this CL.
media/crypto/aes_decryptor.cc:28: On 2012/06/27 00:28:36, fgalligan1 wrote: > Maybe add a TODO to support a key change? Not sure what a key change means in this context. Never mind. I was thinking the decryptor would signal a key change, but the mp4 parser should probably call generateKeyRequest.
http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:53: if (size > input.GetDataSize()) { On 2012/06/27 02:01:21, strobe wrote: > Did change to equality test. It's still >, but I'm not sure you meant to/can change it. http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:238: return need_key_cb_.Run(kid.Pass(), track_encryption.default_kid.size()); On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 23:16:20, ddorwin wrote: > > For ISO, the initData is supposed to be the PSSH. Most likely, it will be > > specified to be all PSSHes. > > Oh, duh, that makes a lot more sense! Where's that specified? This is based on discussions with others, though there are no implementations or examples yet. :) https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673 http://codereview.chromium.org/10651006/diff/15001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:299: decrypt_config->mutable_subsamples()[i].clear_bytes += nalu_size_diff; On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > Rather than mutable_subsamples, you could have an AddFooToSubsamples() > function. > > Not sure what Foo is or if it could be made generic. Seems better than > exposing > > a member to be modified. > > Two reasons we need to mutate: > > 1. To convert each NALU from AVC format (with a length prefix) to Annex B (with > an 0x00000001 start code before each NALU). This amounts to adding a constant > factor to each subsample's clear byte count. > > 2. To inject SPS/PPS. This is a variable-sized addition to the first subsample's > clear bytes count. > > Do you prefer adding two appropriate methods on the DecryptConfig, or addding a > DecryptConfig::SetSubsamples() method and mutate a copy locally, or just making > a new DecryptConfig with values pulled from the old one? Since we are just passing the DecryptConfig to someone else and don't need to store the modifications, a new one might be best. I'd be interested to know what others prefer. Can you add 1 and 2 above as comments in the appropriate places? http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:80: class CompareOffset { On 2012/06/26 06:09:19, ddorwin wrote: > This seems to compare specific types of offsets. Is there a better name? Ping. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:208: RCHECK(cenc_info_[i].Parse(run_itr_->track_encryption.default_iv_size, On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > does something verify we don't exceed info_size (parameter to reader's > > construction) in this Parse? > > Yes, if a request would read past the given size, the parse function immediately > returns false, which is caught by the RCHECK() (which immediately returns false > to the caller, etc, etc). We should have tests for this. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:299: cenc_info_[sample_itr_ - run_itr_->samples.begin()]; On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > What is this math doing? > > Retrieves the index of the sample to which sample_itr_ is currently pointing. Since it's already two lines, an index temp variable might be helpful in explaining this with code. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:309: &run_itr_->track_encryption.default_kid[0], On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > Does this code not support sample groups? > > Correct, it does not. The restricted BMFF format used by DASH/Media Source > requires that only track runs be used. We should document this somewhere. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:29: struct TrackRunInfo { On 2012/06/26 06:09:19, ddorwin wrote: > I think you can define this in the .cc file since the TRI's destructor is there. > Just fwd declare here. Ping. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:57: bool RunValid() const; On 2012/06/26 06:09:19, ddorwin wrote: > These should be Is* Ping. http://codereview.chromium.org/10651006/diff/15001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:69: // Cache the CENC data from the given buffer. |buf| must be a buffer starting On 2012/06/27 02:01:21, strobe wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > Is AuxInfo CENC data? If so, the function name should specify this, as should > > the code that was calling it. > > "Auxiliary information" is a generic mechanism in the amended ISO spec. Here we > only use it when CENC is signaled elsewhere in the spec (or, rather, we will, > once the flux between CENC, PIFF, and WideVine settles down and I can provide > some reasonable assurance of correct behavior across the spectrum of files which > users are likely to see), but there's no guarantee that any aux info we read is > in fact CENC info, so I felt it best to separate the two names. If you feel > otherwise, I'm happy to switch. As long as the code is not making assumptions about it being CENC data. There is other code assuming AuxInfo == CENC, and I seem to remember it being related to this (or similar) function. http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:19: On 2012/06/27 19:37:43, xhwang wrote: > Can we have more docs on this? Ideally, people should be able to implement the > decryptor only by looking at this file, not any specific container spec. Docs on what? DecryptConfig, the parameters, or CENC? http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:25: const SubsampleEntry* subsamples, int subsample_count); On 2012/06/27 19:37:43, xhwang wrote: > Can we just pass const std::vector<SubsampleEntry>& in? Agreed, especially since we store and return it as a vector now. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:50: // XXX(strobe): factor out and test before final commit XXX? TODO? http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:51: const std::vector<SubsampleEntry> subsamples = const ref instead of copying http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:58: if (total_encrypted_subsample_size > input.GetDataSize()) { Should we also check that enc plus unenc are == data size? I guess that requires a second total_ value, but it prevents generation/use of corrupted data, which is usually a good thing. If this is checked below, maybe a comment about this is sufficient. If so, is this check really necessary other than optimizing to return quickly in an error case. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:63: // The encrypted portion of each subsample must form a contiguous block, This line seems to discuss a single subsample rather than all subsamples. s/each subsample/all subsamples/ ? I'm not sure that's the best way to say it but at least it references all subsamples. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:155: // CBC. Remove along with CBC. \nTODO: Remove.. http://codereview.chromium.org/10651006/diff/28001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:199: // not expect or verify. It should be removed along with support for CBC. If you want to remove this one, there should be a CTR WrongKey to take its place. (KeyReplacement does cover this, but it's not obvious that it tests this case. Assuming these tests are quick, i think duplication is fine.) http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc#newcode40 media/mp4/cenc.cc:40: subsamples[i].cypher_bytes = cypher_bytes; This doesn't cause a possible overflow warning? (uint to int) http://codereview.chromium.org/10651006/diff/28001/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:257: int size; Is there a reason you didn't change the name to buf_size? With all the sizes, it wasn't immediately clear what "size" meant when reading the code below. http://codereview.chromium.org/10651006/diff/28001/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:294: // XXX(strobe): decide how best to make these modifications TODO http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:206: // This implementation currently only caches CENC auxiliary info. "CENC-style" to be consistent with 199? http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:100: scoped_ptr<DecryptConfig> GetDecryptConfig(); I don't think we usually return (or pass) scoped_ptrs like this. The comment should indicate the caller owns the result.
http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/28001/media/base/decrypt_config.... media/base/decrypt_config.h:19: On 2012/07/03 21:03:47, ddorwin wrote: > On 2012/06/27 19:37:43, xhwang wrote: > > Can we have more docs on this? Ideally, people should be able to implement the > > decryptor only by looking at this file, not any specific container spec. > > Docs on what? DecryptConfig, the parameters, or CENC? About SubsampleEntry.
https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:53: if (size > input.GetDataSize()) { On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/27 02:01:21, strobe wrote: > > Did change to equality test. > > It's still >, but I'm not sure you meant to/can change it. Yes, you're correct, the comment got out of sync with the code. Hopefully variable names make that clearer. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:238: return need_key_cb_.Run(kid.Pass(), track_encryption.default_kid.size()); On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/27 02:01:21, strobe wrote: > > On 2012/06/26 23:16:20, ddorwin wrote: > > > For ISO, the initData is supposed to be the PSSH. Most likely, it will be > > > specified to be all PSSHes. > > > > Oh, duh, that makes a lot more sense! Where's that specified? > > This is based on discussions with others, though there are no implementations or > examples yet. :) > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673 OK, comment added. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:257: int size; On 2012/06/26 06:09:19, ddorwin wrote: > buf_size would make this function easier to read. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:257: int size; On 2012/06/26 06:09:19, ddorwin wrote: > buf_size would make this function easier to read. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:299: decrypt_config->mutable_subsamples()[i].clear_bytes += nalu_size_diff; On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/27 02:01:21, strobe wrote: > > On 2012/06/26 06:09:19, ddorwin wrote: > > > Rather than mutable_subsamples, you could have an AddFooToSubsamples() > > function. > > > Not sure what Foo is or if it could be made generic. Seems better than > > exposing > > > a member to be modified. > > > > Two reasons we need to mutate: > > > > 1. To convert each NALU from AVC format (with a length prefix) to Annex B > (with > > an 0x00000001 start code before each NALU). This amounts to adding a constant > > factor to each subsample's clear byte count. > > > > 2. To inject SPS/PPS. This is a variable-sized addition to the first > subsample's > > clear bytes count. > > > > Do you prefer adding two appropriate methods on the DecryptConfig, or addding > a > > DecryptConfig::SetSubsamples() method and mutate a copy locally, or just > making > > a new DecryptConfig with values pulled from the old one? > > Since we are just passing the DecryptConfig to someone else and don't need to > store the modifications, a new one might be best. > I'd be interested to know what others prefer. > > Can you add 1 and 2 above as comments in the appropriate places? Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:80: class CompareOffset { On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > This seems to compare specific types of offsets. Is there a better name? > > Ping. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:299: cenc_info_[sample_itr_ - run_itr_->samples.begin()]; On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/27 02:01:21, strobe wrote: > > On 2012/06/26 06:09:19, ddorwin wrote: > > > What is this math doing? > > > > Retrieves the index of the sample to which sample_itr_ is currently pointing. > > Since it's already two lines, an index temp variable might be helpful in > explaining this with code. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:309: &run_itr_->track_encryption.default_kid[0], On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/27 02:01:21, strobe wrote: > > On 2012/06/26 06:09:19, ddorwin wrote: > > > Does this code not support sample groups? > > > > Correct, it does not. The restricted BMFF format used by DASH/Media Source > > requires that only track runs be used. > > We should document this somewhere. Expanded comment in mp4/box_definitions.h. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.h:29: struct TrackRunInfo { On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > I think you can define this in the .cc file since the TRI's destructor is > there. > > Just fwd declare here. > > Ping. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.h:57: bool RunValid() const; On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > These should be Is* > > Ping. Done. https://chromiumcodereview.appspot.com/10651006/diff/15001/media/mp4/track_ru... media/mp4/track_run_iterator.h:57: bool RunValid() const; On 2012/07/03 21:03:46, ddorwin wrote: > On 2012/06/26 06:09:19, ddorwin wrote: > > These should be Is* > > Ping. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/base/decrypt... media/base/decrypt_config.h:8: #include <vector> On 2012/06/27 19:37:43, xhwang wrote: > nit: usually we have empty line after including c++ system files. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/base/decrypt... media/base/decrypt_config.h:8: #include <vector> On 2012/06/27 19:37:43, xhwang wrote: > nit: usually we have empty line after including c++ system files. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/base/decrypt... media/base/decrypt_config.h:19: On 2012/07/03 21:17:12, xhwang wrote: > On 2012/07/03 21:03:47, ddorwin wrote: > > On 2012/06/27 19:37:43, xhwang wrote: > > > Can we have more docs on this? Ideally, people should be able to implement > the > > > decryptor only by looking at this file, not any specific container spec. > > > > Docs on what? DecryptConfig, the parameters, or CENC? > > About SubsampleEntry. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/base/decrypt... media/base/decrypt_config.h:25: const SubsampleEntry* subsamples, int subsample_count); On 2012/07/03 21:03:47, ddorwin wrote: > On 2012/06/27 19:37:43, xhwang wrote: > > Can we just pass const std::vector<SubsampleEntry>& in? > > Agreed, especially since we store and return it as a vector now. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:31: input.GetDecryptConfig()->iv_size()); On 2012/06/27 19:37:43, xhwang wrote: > Life will be easier here if we use std::string for IV. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:50: // XXX(strobe): factor out and test before final commit On 2012/07/03 21:03:47, ddorwin wrote: > XXX? TODO? Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:51: const std::vector<SubsampleEntry> subsamples = On 2012/07/03 21:03:47, ddorwin wrote: > const ref instead of copying Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:58: if (total_encrypted_subsample_size > input.GetDataSize()) { On 2012/07/03 21:03:47, ddorwin wrote: > Should we also check that enc plus unenc are == data size? > I guess that requires a second total_ value, but it prevents generation/use of > corrupted data, which is usually a good thing. > If this is checked below, maybe a comment about this is sufficient. > If so, is this check really necessary other than optimizing to return quickly in > an error case. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:63: // The encrypted portion of each subsample must form a contiguous block, On 2012/07/03 21:03:47, ddorwin wrote: > This line seems to discuss a single subsample rather than all subsamples. > > s/each subsample/all subsamples/ ? > I'm not sure that's the best way to say it but at least it references all > subsamples. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:71: int in_pos = 0, out_pos = 0; On 2012/06/27 19:37:43, xhwang wrote: > Prefer to declaring variables on separate lines. I am surprised that this isn't > specified explicitly in the style guide, but I seldom see this pattern in > chromium code. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:105: out_pos += subsample.cypher_bytes; On 2012/06/27 19:37:43, xhwang wrote: > This part is so similar to the block starting from line 72. Can we have a help > function to do that? Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:108: } else { On 2012/06/27 19:37:43, xhwang wrote: > nit: we don't need this "else" since "if" will always "return". > > e.g. > > if (...) { > ... > return; > } > > do_something_else Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:92: void InitCBC() { On 2012/06/27 22:38:35, xhwang wrote: > Change to InitCbc(), to be consistent with kCbcEncryptedData, and the coding > style: > Prefer to capitalize well-known abbreviations as if they were words: HttpUrl > instead of HTTPURL. > (http://dev.chromium.org/developers/coding-style#Naming) > > Please also change similar names in this CL. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:155: // CBC. Remove along with CBC. On 2012/07/03 21:03:47, ddorwin wrote: > \nTODO: Remove.. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:199: // not expect or verify. It should be removed along with support for CBC. On 2012/07/03 21:03:47, ddorwin wrote: > If you want to remove this one, there should be a CTR WrongKey to take its > place. (KeyReplacement does cover this, but it's not obvious that it tests this > case. Assuming these tests are quick, i think duplication is fine.) Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/cenc.cc#... media/mp4/cenc.cc:7: #include <string.h> On 2012/06/27 19:37:43, xhwang wrote: > Use C++ headers? #include <cstring> Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/cenc.cc#... media/mp4/cenc.cc:40: subsamples[i].cypher_bytes = cypher_bytes; On 2012/07/03 21:03:47, ddorwin wrote: > This doesn't cause a possible overflow warning? (uint to int) Surprisingly, no. I'm more than happy to use uint32/int64/uint64; I used int here to comply with the style guide. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:294: // XXX(strobe): decide how best to make these modifications On 2012/07/03 21:03:47, ddorwin wrote: > TODO Sorry, "XXX" is personal shorthand for "resolve before checking this in" (easy to grep for). https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:134: // container, if it is present On 2012/06/27 19:37:43, xhwang wrote: > Add period "." at the end. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:206: // This implementation currently only caches CENC auxiliary info. On 2012/07/03 21:03:47, ddorwin wrote: > "CENC-style" to be consistent with 199? Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... media/mp4/track_run_iterator.h:11: #include "media/base/decrypt_config.h" On 2012/06/27 19:37:43, xhwang wrote: > Forward declaration of DecryptConfig instead of including the header file. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... media/mp4/track_run_iterator.h:69: // Cache the CENC data from the given buffer. |buf| must be a buffer starting On 2012/06/27 19:37:43, xhwang wrote: > Cache->Caches Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/track_ru... media/mp4/track_run_iterator.h:100: scoped_ptr<DecryptConfig> GetDecryptConfig(); On 2012/07/03 21:03:47, ddorwin wrote: > I don't think we usually return (or pass) scoped_ptrs like this. > The comment should indicate the caller owns the result. Done.
Please upload changes before rebasing on test or WebM CL. http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): http://codereview.chromium.org/10651006/diff/28001/media/mp4/cenc.cc#newcode40 media/mp4/cenc.cc:40: subsamples[i].cypher_bytes = cypher_bytes; On 2012/07/13 00:47:07, strobe wrote: > On 2012/07/03 21:03:47, ddorwin wrote: > > This doesn't cause a possible overflow warning? (uint to int) > > Surprisingly, no. I'm more than happy to use uint32/int64/uint64; I used int > here to comply with the style guide. I think if you're dealing with data and it can really be unsigned 32 bits, it's okay to use uint32. They just don't want you adding more unsigned values. http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:22: const bool src_contains_clear_bytes, Consider an enum since it's not clear that false means dst_contains... See http://www.chromium.org/developers/coding-style#Code_Formatting. http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:23: const uint8* src, uint8* dst) { same line only for call sites, not defn/decl. http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:31: memcpy(dst, src, subsample.cypher_bytes); Should we verify we don't exceed either buffer size before copying? http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:62: if (!input.GetDecryptConfig()->subsamples().empty()) { Prefer positive logic (unless there's a good reason not to). In this case, it is essentially if-else and neither is an error or early return. This also makes the if block smaller. http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), Why not just size as the second parameter? http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:181: InitCtr(); Skipping. Will review after rebase. http://codereview.chromium.org/10651006/diff/31007/media/mp4/box_definitions.cc File media/mp4/box_definitions.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/box_definitions.... media/mp4/box_definitions.cc:402: RCHECK(reader->ReadVec(&data, reader->size() - reader->pos())); To verify, pos() is an index? index() might be a better name since pos() _could_ be a pointer. http://codereview.chromium.org/10651006/diff/31007/media/mp4/box_definitions.... media/mp4/box_definitions.cc:519: if (reader->version() == 1) { Comment. V1 was... All other versions ... http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:160: RCHECK(entry.format == FOURCC_MP4A || Is this worth logging since it means the file format is incompatible (i.e. problem with user input)? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:166: RCHECK(entry.esds.object_type == kISO_14496_3); same http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:179: RCHECK(entry.format == FOURCC_AVC1 || same http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:190: 0, 1, what is the 1? if it's not paired with the 0 (or even if), probably better to put on separate line since it's the only one that's doubled up. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:207: if (!init_cb_.is_null()) checking wrong callback? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:253: RCHECK(AVC::ConvertToAnnexB(size_of_nalu_length_, frame_buf)); Is there a [D]CHECK that size_of_nalu_length_ <= 4 somewhere? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:265: // a frame. If subsample info is present, we also update the clear byte update to what? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:270: avc_config = &moov_->tracks[t].media.information. strange line break. I think everything after = should be on a new line(s). http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:271: sample_table.description.video_entries[0].avcc; is video_entries size guaranteed to be >= 1? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:277: RCHECK(AVC::ConvertParameterSets(*avc_config, ¶m_sets)); ConvertConfigToParameterSets? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:289: if (!runs_.RunIsValid()) { Why isn't this IsRunValid? Unless you really are running "is valid". http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:298: if (!runs_.SampleIsValid()) { same http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:342: RCHECK(PrepareAVCBuffer(&frame_buf, &subsamples)); If else above, we want to run this for the key frame logic? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:371: << ", cts=" << runs_.cts().InMilliseconds() set cts before dts above. should it appear that way here too? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); Don't clear the buffers if there is no cb or it fails? When do they get cleared? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parser.h File media/mp4/mp4_stream_parser.h (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.h:95: // We keep them around to avoid having to go digging through the moov with s/them/these/ http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser_unittest.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:37: bool got_configs_; s/got/have/ seems better http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:44: return AppendDataInPieces(data, length, 7); Why 7? That's an odd size. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:85: EXPECT_LE(segment_start_, (*buf)->GetTimestamp()); You're really testing the timestamp, which means this should be GE(TS, start_), right? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:110: bool ParseMP4File(const std::string& filename, int append_size) { size=bytes? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:121: TEST_F(MP4StreamParserTest, TestParseBearDASH) { Is "bear" really relevant to the test? What is interesting about the 512? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:122: ParseMP4File("bear.1280x720_dash.mp4", 512); Unrelated to this CL, is this file really "dash" or just BMFF? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:126: ParseMP4File("bear.1280x720_dash.mp4", 768432); What do the sizes mean? http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser_unittest.cc:130: InitializeParser(); Where is the second initialization? Shouldn't re-initialization be tested after parsing some data? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:31: std::vector<uint8> aux_info_sizes; // Present if default_size == 0. Present => Populated or Non-empty http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:51: TimeDelta TimeDeltaFromFrac(int64 numer, int64 denom) { Function and var names should usually be complete unless standard abbreviations. http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:51: TimeDelta TimeDeltaFromFrac(int64 numer, int64 denom) { What does "FromFrac" mean? Isn't it really converting to a timebase? Then you could have more meaningful names. http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:52: DCHECK_LT((numer > 0 ? numer : -numer), use abs()? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:58: static const uint32 kSampleIsDifferenceSampleFlagMask = 0x10000; file constant should be at the top of the file in an anon namespace. http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:102: class CompareMinTrackRunDataOffset { What does this do? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:262: int64 offset = kint64max; max_offset? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:287: dts = run_dts; Why? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:343: const FrameCENCInfo& cenc_info = cenc_info_[sample_idx]; check size of the vector/array first. http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:356: std::string(cenc_info.iv, cenc_info.iv + arraysize(cenc_info.iv)), why not just size as second param? http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.h (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator.h:38: bool RunIsValid() const; Is* http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... File media/mp4/track_run_iterator_unittest.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/track_run_iterat... media/mp4/track_run_iterator_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Did not review. Will review after test CL is landed and this is rebased.
Sorry, I rebased on to the test CL before I saw your message. This doesn't include the WebM CL, though, since that was reverted. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/cenc.cc File media/mp4/cenc.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/cenc.cc#... media/mp4/cenc.cc:40: subsamples[i].cypher_bytes = cypher_bytes; On 2012/07/17 01:14:21, ddorwin wrote: > On 2012/07/13 00:47:07, strobe wrote: > > On 2012/07/03 21:03:47, ddorwin wrote: > > > This doesn't cause a possible overflow warning? (uint to int) > > > > Surprisingly, no. I'm more than happy to use uint32/int64/uint64; I used int > > here to comply with the style guide. > > I think if you're dealing with data and it can really be unsigned 32 bits, it's > okay to use uint32. They just don't want you adding more unsigned values. Done. https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/28001/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:257: int size; On 2012/07/03 21:03:47, ddorwin wrote: > Is there a reason you didn't change the name to buf_size? With all the sizes, it > wasn't immediately clear what "size" meant when reading the code below. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor.cc:22: const bool src_contains_clear_bytes, On 2012/07/17 01:14:21, ddorwin wrote: > Consider an enum since it's not clear that false means dst_contains... See > http://www.chromium.org/developers/coding-style#Code_Formatting. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor.cc:23: const uint8* src, uint8* dst) { On 2012/07/17 01:14:21, ddorwin wrote: > same line only for call sites, not defn/decl. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor.cc:31: memcpy(dst, src, subsample.cypher_bytes); On 2012/07/17 01:14:21, ddorwin wrote: > Should we verify we don't exceed either buffer size before copying? Since this is a static internal helper function, it might be okay to rely on the checks done by the caller. WDYT? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor.cc:62: if (!input.GetDecryptConfig()->subsamples().empty()) { On 2012/07/17 01:14:21, ddorwin wrote: > Prefer positive logic (unless there's a good reason not to). In this case, it is > essentially if-else and neither is an error or early return. This also makes the > if block smaller. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), On 2012/07/17 01:14:21, ddorwin wrote: > Why not just size as the second parameter? Would require typecasting. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/box_defi... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/box_defi... media/mp4/box_definitions.cc:519: if (reader->version() == 1) { On 2012/07/17 01:14:21, ddorwin wrote: > Comment. V1 was... All other versions ... Like most of this file, this is essentially a line-for-line translation of the box definitions in ISO 14496-12. The name "version" is misleading, since this pattern was always in the spec AFAICT (in multiple places) and is not tied to any spec version. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:160: RCHECK(entry.format == FOURCC_MP4A || On 2012/07/17 01:14:21, ddorwin wrote: > Is this worth logging since it means the file format is incompatible (i.e. > problem with user input)? I'd prefer to leave it in. DLOG() is cheaper than playing "find the right 'return false';" later on. (Additionally, we may later decide to provide informative messages to the application.) https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:190: 0, 1, On 2012/07/17 01:14:21, ddorwin wrote: > what is the 1? > if it's not paired with the 0 (or even if), probably better to put on separate > line since it's the only one that's doubled up. Framerate numerator and denominator. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:207: if (!init_cb_.is_null()) On 2012/07/17 01:14:21, ddorwin wrote: > checking wrong callback? No, I think this is correct. We currently use nullness of init_cb (which should only be called once ever in the current model, and is set to null after that first call) as a signal to indicate that config_cb should not be called again. We don't null config_cb directly because it will eventually be the right way to send this information. This is covered by a test. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:253: RCHECK(AVC::ConvertToAnnexB(size_of_nalu_length_, frame_buf)); On 2012/07/17 01:14:21, ddorwin wrote: > Is there a [D]CHECK that size_of_nalu_length_ <= 4 somewhere? Yes, in ConvertToAnnexB. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:265: // a frame. If subsample info is present, we also update the clear byte On 2012/07/17 01:14:21, ddorwin wrote: > update to what? Commented. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:270: avc_config = &moov_->tracks[t].media.information. On 2012/07/17 01:14:21, ddorwin wrote: > strange line break. I think everything after = should be on a new line(s). Rebased away. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:271: sample_table.description.video_entries[0].avcc; On 2012/07/17 01:14:21, ddorwin wrote: > is video_entries size guaranteed to be >= 1? Rebased away. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:277: RCHECK(AVC::ConvertParameterSets(*avc_config, ¶m_sets)); On 2012/07/17 01:14:21, ddorwin wrote: > ConvertConfigToParameterSets? Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:289: if (!runs_.RunIsValid()) { On 2012/07/17 01:14:21, ddorwin wrote: > Why isn't this IsRunValid? Unless you really are running "is valid". Evidence of my functional bias. Renamed. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:298: if (!runs_.SampleIsValid()) { On 2012/07/17 01:14:21, ddorwin wrote: > same Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:342: RCHECK(PrepareAVCBuffer(&frame_buf, &subsamples)); On 2012/07/17 01:14:21, ddorwin wrote: > If else above, we want to run this for the key frame logic? Sorry, can you clarify this comment? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:371: << ", cts=" << runs_.cts().InMilliseconds() On 2012/07/17 01:14:21, ddorwin wrote: > set cts before dts above. should it appear that way here too? Timestamp is the more important field, since DecodeTimestamp is only required due to an implementation detail of the source buffer and may be removed at a later time. However, 'dts' is monotonically increasing, making output traces easier to read if it is first since subsequent lines don't flick back and forth as much due to uneven field size. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); On 2012/07/17 01:14:21, ddorwin wrote: > Don't clear the buffers if there is no cb or it fails? When do they get cleared? Fixed. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.h (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.h:95: // We keep them around to avoid having to go digging through the moov with On 2012/07/17 01:14:21, ddorwin wrote: > s/them/these/ Rebased away. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:37: bool got_configs_; On 2012/07/17 01:14:21, ddorwin wrote: > s/got/have/ seems better Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:44: return AppendDataInPieces(data, length, 7); On 2012/07/17 01:14:21, ddorwin wrote: > Why 7? That's an odd size. Method unused, removed. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:85: EXPECT_LE(segment_start_, (*buf)->GetTimestamp()); On 2012/07/17 01:14:21, ddorwin wrote: > You're really testing the timestamp, which means this should be GE(TS, start_), > right? Well, not according to the macro definition ;) Aaron informed me that the convention is reversed in media/. Fixed. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:110: bool ParseMP4File(const std::string& filename, int append_size) { On 2012/07/17 01:14:21, ddorwin wrote: > size=bytes? Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:121: TEST_F(MP4StreamParserTest, TestParseBearDASH) { On 2012/07/17 01:14:21, ddorwin wrote: > Is "bear" really relevant to the test? What is interesting about the 512? Renamed and commented. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:122: ParseMP4File("bear.1280x720_dash.mp4", 512); On 2012/07/17 01:14:21, ddorwin wrote: > Unrelated to this CL, is this file really "dash" or just BMFF? It complies with 23009-1 6.3.4 and 14496-12 FAMD3 8.8.14. It's representative of what you might find in a DASH Live Profile media stream. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:126: ParseMP4File("bear.1280x720_dash.mp4", 768432); On 2012/07/17 01:14:21, ddorwin wrote: > What do the sizes mean? Commented. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:130: InitializeParser(); On 2012/07/17 01:14:21, ddorwin wrote: > Where is the second initialization? Shouldn't re-initialization be tested after > parsing some data? Commented. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:31: std::vector<uint8> aux_info_sizes; // Present if default_size == 0. On 2012/07/17 01:14:21, ddorwin wrote: > Present => Populated or Non-empty Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:51: TimeDelta TimeDeltaFromFrac(int64 numer, int64 denom) { On 2012/07/17 01:14:21, ddorwin wrote: > What does "FromFrac" mean? Isn't it really converting to a timebase? Then you > could have more meaningful names. The time base is referred to as the denominator elsewhere (e.g. video_decoder_config) (although an AVRational is also (improperly) referred to as a time base). Changed to FromRational to be more explicit. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:52: DCHECK_LT((numer > 0 ? numer : -numer), On 2012/07/17 01:14:21, ddorwin wrote: > use abs()? Nope, breaks build on cros-tegra2. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:58: static const uint32 kSampleIsDifferenceSampleFlagMask = 0x10000; On 2012/07/17 01:14:21, ddorwin wrote: > file constant should be at the top of the file in an anon namespace. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:102: class CompareMinTrackRunDataOffset { On 2012/07/17 01:14:21, ddorwin wrote: > What does this do? Comment added. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:262: int64 offset = kint64max; On 2012/07/17 01:14:21, ddorwin wrote: > max_offset? Well, internal to this function, it's really the min offset among all things that still need to read data. The min/max duality is a thicket I'd prefer to avoid. Comment added. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:287: dts = run_dts; On 2012/07/17 01:14:21, ddorwin wrote: > Why? Addressed by rebase. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:343: const FrameCENCInfo& cenc_info = cenc_info_[sample_idx]; On 2012/07/17 01:14:21, ddorwin wrote: > check size of the vector/array first. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.cc:356: std::string(cenc_info.iv, cenc_info.iv + arraysize(cenc_info.iv)), On 2012/07/17 01:14:21, ddorwin wrote: > why not just size as second param? Would require typecasting. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... File media/mp4/track_run_iterator.h (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/track_ru... media/mp4/track_run_iterator.h:38: bool RunIsValid() const; On 2012/07/17 01:14:21, ddorwin wrote: > Is* Done.
It seems like some of the things you said are addressed weren't in the latest patch. Other than that, looks fine. Need to rebase on Frank's CL when that lands, and then I need to review the tests. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > Why not just size as the second parameter? > > Would require typecasting. Just curious: to what? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:160: RCHECK(entry.format == FOURCC_MP4A || On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > Is this worth logging since it means the file format is incompatible (i.e. > > problem with user input)? > > I'd prefer to leave it in. DLOG() is cheaper than playing "find the right > 'return false';" later on. (Additionally, we may later decide to provide > informative messages to the application.) I think I meant should we ALWAYS log it. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:207: if (!init_cb_.is_null()) On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > checking wrong callback? > > No, I think this is correct. We currently use nullness of init_cb (which should > only be called once ever in the current model, and is set to null after that > first call) as a signal to indicate that config_cb should not be called again. > We don't null config_cb directly because it will eventually be the right way to > send this information. This is covered by a test. Hmm, might be worth a comment explaining that here since the pattern is unexpected. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:265: // a frame. If subsample info is present, we also update the clear byte On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > update to what? > > Commented. Where? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:270: avc_config = &moov_->tracks[t].media.information. On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > strange line break. I think everything after = should be on a new line(s). > > Rebased away. Still here. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:271: sample_table.description.video_entries[0].avcc; On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > is video_entries size guaranteed to be >= 1? > > Rebased away. Still here. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:342: RCHECK(PrepareAVCBuffer(&frame_buf, &subsamples)); On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > If else above, we want to run this for the key frame logic? > > Sorry, can you clarify this comment? If line 340 - if (decrypt_config.get()) - is not true, subsamples in null and PrepareAVCBuffer() just does something related to key frames. Is that correct? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > Don't clear the buffers if there is no cb or it fails? When do they get > cleared? > > Fixed. Where? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser_unittest.cc:130: InitializeParser(); On 2012/07/19 02:43:35, strobe wrote: > On 2012/07/17 01:14:21, ddorwin wrote: > > Where is the second initialization? Shouldn't re-initialization be tested > after > > parsing some data? > > Commented. Where? https://chromiumcodereview.appspot.com/10651006/diff/72001/media/mp4/box_defi... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/72001/media/mp4/box_defi... media/mp4/box_definitions.cc:397: AudioSampleEntry::~AudioSampleEntry() {} restore empty line after
https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > Why not just size as the second parameter? > > > > Would require typecasting. > > Just curious: to what? uint8 to int8. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:160: RCHECK(entry.format == FOURCC_MP4A || On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > Is this worth logging since it means the file format is incompatible (i.e. > > > problem with user input)? > > > > I'd prefer to leave it in. DLOG() is cheaper than playing "find the right > > 'return false';" later on. (Additionally, we may later decide to provide > > informative messages to the application.) > > I think I meant should we ALWAYS log it. Oh! RCHECK() does a DLOG(ERROR) on false. Should we promote that to a LOG() (it pretty much always indicates user/file error), or just add a separate LOG() here and below for these cases? https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:207: if (!init_cb_.is_null()) On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > checking wrong callback? > > > > No, I think this is correct. We currently use nullness of init_cb (which > should > > only be called once ever in the current model, and is set to null after that > > first call) as a signal to indicate that config_cb should not be called again. > > We don't null config_cb directly because it will eventually be the right way > to > > send this information. This is covered by a test. > > Hmm, might be worth a comment explaining that here since the pattern is > unexpected. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:270: avc_config = &moov_->tracks[t].media.information. On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > strange line break. I think everything after = should be on a new line(s). > > > > Rebased away. > > Still here. Sorry, *really* fixed. Thanks for your dilligence! https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:271: sample_table.description.video_entries[0].avcc; On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > is video_entries size guaranteed to be >= 1? > > > > Rebased away. > > Still here. Done. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:342: RCHECK(PrepareAVCBuffer(&frame_buf, &subsamples)); On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > If else above, we want to run this for the key frame logic? > > > > Sorry, can you clarify this comment? > > If line 340 - if (decrypt_config.get()) - is not true, subsamples in null and > PrepareAVCBuffer() just does something related to key frames. Is that correct? No. H.264 NALUs are stored in "AVC" format (length-prefixed) in BMFF, but most decoders expect data in Annex B format (start-code-prefixed). That conversion happens for every sample regardless of crypto or keyframe status. https://chromiumcodereview.appspot.com/10651006/diff/31007/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > Don't clear the buffers if there is no cb or it fails? When do they get > > cleared? > > > > Fixed. > > Where? Apparently the fix was also lost in the failed rebase, but on consideration the fix wasn't even necessary: these objects are allocated on the stack in MP4StreamParser::Parse. https://chromiumcodereview.appspot.com/10651006/diff/72001/media/mp4/box_defi... File media/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/72001/media/mp4/box_defi... media/mp4/box_definitions.cc:397: AudioSampleEntry::~AudioSampleEntry() {} On 2012/07/19 06:11:10, ddorwin wrote: > restore empty line after Done.
Rebased on top of WebM decryption CL.
Mostly nits, but the string construction seems odd. I will review aes_decryptor_unittest.cc and track_run_iterator_unittest.cc tomorrow. http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), On 2012/07/19 16:55:02, strobe wrote: > On 2012/07/19 06:11:10, ddorwin wrote: > > On 2012/07/19 02:43:35, strobe wrote: > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > Why not just size as the second parameter? > > > > > > Would require typecasting. > > > > Just curious: to what? > > uint8 to int8. The first parameter? uint8* to char*? So, it would be the following? std::string(static_cast<char*>(kInitialIv), arraysize(kInitialIv)), This (casting a pointer) seems easier/quicker to understand than pointer arithmetic and use of what I'm assuming is the iterator constructor. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:160: RCHECK(entry.format == FOURCC_MP4A || On 2012/07/19 16:55:02, strobe wrote: > On 2012/07/19 06:11:10, ddorwin wrote: > > On 2012/07/19 02:43:35, strobe wrote: > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > Is this worth logging since it means the file format is incompatible (i.e. > > > > problem with user input)? > > > > > > I'd prefer to leave it in. DLOG() is cheaper than playing "find the right > > > 'return false';" later on. (Additionally, we may later decide to provide > > > informative messages to the application.) > > > > I think I meant should we ALWAYS log it. > > Oh! RCHECK() does a DLOG(ERROR) on false. Should we promote that to a LOG() (it > pretty much always indicates user/file error), or just add a separate LOG() here > and below for these cases? I was thinking a separate LOG() for these cases since they are valid files that just aren't supported. Most of the others probably mean the file is somehow invalid. For example, if I try to play my file in Chrome and it doesn't work, this log would help me figure it out. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:265: // a frame. If subsample info is present, we also update the clear byte On 2012/07/19 06:11:10, ddorwin wrote: > On 2012/07/19 02:43:35, strobe wrote: > > On 2012/07/17 01:14:21, ddorwin wrote: > > > update to what? > > > > Commented. > > Where? Seem to have missed this one. :) http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:342: RCHECK(PrepareAVCBuffer(&frame_buf, &subsamples)); On 2012/07/19 16:55:02, strobe wrote: > On 2012/07/19 06:11:10, ddorwin wrote: > > On 2012/07/19 02:43:35, strobe wrote: > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > If else above, we want to run this for the key frame logic? > > > > > > Sorry, can you clarify this comment? > > > > If line 340 - if (decrypt_config.get()) - is not true, subsamples in null and > > PrepareAVCBuffer() just does something related to key frames. Is that correct? > > No. H.264 NALUs are stored in "AVC" format (length-prefixed) in BMFF, but most > decoders expect data in Annex B format (start-code-prefixed). That conversion > happens for every sample regardless of crypto or keyframe status. Ahh, I overlooked the conversion call. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); On 2012/07/19 16:55:02, strobe wrote: > On 2012/07/19 06:11:10, ddorwin wrote: > > On 2012/07/19 02:43:35, strobe wrote: > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > Don't clear the buffers if there is no cb or it fails? When do they get > > > cleared? > > > > > > Fixed. > > > > Where? > > Apparently the fix was also lost in the failed rebase, but on consideration the > fix wasn't even necessary: these objects are allocated on the stack in > MP4StreamParser::Parse. I don't understand. Are you intentionally keeping them around in the case of no callback or a failed Run()? If so, that should probably be a comment somewhere. If not, I don't understand why you wouldn't always clear them. http://codereview.chromium.org/10651006/diff/67005/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/67005/media/base/decrypt_config.... media/base/decrypt_config.h:64: std::string key_id_; Why was const removed from all these? http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:78: if (memcmp(input.GetDecryptConfig()->checksum().data(), Please add: TODO(fgalligan): Use hmac.Verify(). http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:247: // TODO(fgalligan): When ISO is added we will need to figure out how to This is probably redundant with the TODO below. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:306: // mechanism. ... and remove the webm_ member. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor.h File media/crypto/aes_decryptor.h (right): http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:68: crypto::SymmetricKey* webm_decryption_key() This should be the same as the above value. IOW, not a separate member. I see your TODO in the .cc. We can cover this comment there. http://codereview.chromium.org/10651006/diff/67005/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:217: scoped_array<char> counter_block_data( Can we allocate the string here (instead of 228) and access its data below? http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:291: RCHECK(AVC::ConvertParameterSetsToAnnexB(avc_config, ¶m_sets)); This is a bit confusing because "ParameterSets" would seem to be the input from the function name, but param_sets is an output parameter. If avc_config contains "paramater sets" and param_sets is AnnexB format, I guess that's fine, but a little confusing to read. http://codereview.chromium.org/10651006/diff/67005/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:421: std::string(), May help with readability: // No checksum in MP4. Similar below.
http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:106: std::string(kInitialIv, kInitialIv + arraysize(kInitialIv)), On 2012/07/24 01:00:09, ddorwin wrote: > On 2012/07/19 16:55:02, strobe wrote: > > On 2012/07/19 06:11:10, ddorwin wrote: > > > On 2012/07/19 02:43:35, strobe wrote: > > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > > Why not just size as the second parameter? > > > > > > > > Would require typecasting. > > > > > > Just curious: to what? > > > > uint8 to int8. > > The first parameter? uint8* to char*? > So, it would be the following? > std::string(static_cast<char*>(kInitialIv), arraysize(kInitialIv)), > This (casting a pointer) seems easier/quicker to understand than pointer > arithmetic and use of what I'm assuming is the iterator constructor. Will get this in next bundle since it touches many locations. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:265: // a frame. If subsample info is present, we also update the clear byte On 2012/07/24 01:00:09, ddorwin wrote: > On 2012/07/19 06:11:10, ddorwin wrote: > > On 2012/07/19 02:43:35, strobe wrote: > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > update to what? > > > > > > Commented. > > > > Where? > > Seem to have missed this one. :) Done. http://codereview.chromium.org/10651006/diff/31007/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:389: audio_buffers->clear(); On 2012/07/24 01:00:09, ddorwin wrote: > On 2012/07/19 16:55:02, strobe wrote: > > On 2012/07/19 06:11:10, ddorwin wrote: > > > On 2012/07/19 02:43:35, strobe wrote: > > > > On 2012/07/17 01:14:21, ddorwin wrote: > > > > > Don't clear the buffers if there is no cb or it fails? When do they get > > > > cleared? > > > > > > > > Fixed. > > > > > > Where? > > > > Apparently the fix was also lost in the failed rebase, but on consideration > the > > fix wasn't even necessary: these objects are allocated on the stack in > > MP4StreamParser::Parse. > > I don't understand. Are you intentionally keeping them around in the case of no > callback or a failed Run()? If so, that should probably be a comment somewhere. > If not, I don't understand why you wouldn't always clear them. Done. http://codereview.chromium.org/10651006/diff/67005/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10651006/diff/67005/media/base/decrypt_config.... media/base/decrypt_config.h:64: std::string key_id_; On 2012/07/24 01:00:10, ddorwin wrote: > Why was const removed from all these? ...I have no idea. Fixed. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:78: if (memcmp(input.GetDecryptConfig()->checksum().data(), On 2012/07/24 01:00:10, ddorwin wrote: > Please add: > TODO(fgalligan): Use hmac.Verify(). Done. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:247: // TODO(fgalligan): When ISO is added we will need to figure out how to On 2012/07/24 01:00:10, ddorwin wrote: > This is probably redundant with the TODO below. Done. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:306: // mechanism. On 2012/07/24 01:00:10, ddorwin wrote: > ... and remove the webm_ member. Done. http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor.h File media/crypto/aes_decryptor.h (right): http://codereview.chromium.org/10651006/diff/67005/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:68: crypto::SymmetricKey* webm_decryption_key() On 2012/07/24 01:00:10, ddorwin wrote: > This should be the same as the above value. IOW, not a separate member. I see > your TODO in the .cc. We can cover this comment there. OK, anything needed here? http://codereview.chromium.org/10651006/diff/67005/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:217: scoped_array<char> counter_block_data( On 2012/07/24 01:00:10, ddorwin wrote: > Can we allocate the string here (instead of 228) and access its data below? string.data() seems to be const. http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:291: RCHECK(AVC::ConvertParameterSetsToAnnexB(avc_config, ¶m_sets)); On 2012/07/24 01:00:10, ddorwin wrote: > This is a bit confusing because "ParameterSets" would seem to be the input from > the function name, but param_sets is an output parameter. If avc_config contains > "paramater sets" and param_sets is AnnexB format, I guess that's fine, but a > little confusing to read. I see your point. Suggestions for alternative name? GetAnnexBParameterSetsFromConfig? http://codereview.chromium.org/10651006/diff/67005/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:421: std::string(), On 2012/07/24 01:00:10, ddorwin wrote: > May help with readability: > // No checksum in MP4. > Similar below. Done.
LGTM % comments. Thanks. Most important comment is to add an additional test. http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... File media/mp4/mp4_stream_parser.cc (right): http://codereview.chromium.org/10651006/diff/67005/media/mp4/mp4_stream_parse... media/mp4/mp4_stream_parser.cc:291: RCHECK(AVC::ConvertParameterSetsToAnnexB(avc_config, ¶m_sets)); On 2012/07/25 01:05:13, strobe wrote: > On 2012/07/24 01:00:10, ddorwin wrote: > > This is a bit confusing because "ParameterSets" would seem to be the input > from > > the function name, but param_sets is an output parameter. If avc_config > contains > > "paramater sets" and param_sets is AnnexB format, I guess that's fine, but a > > little confusing to read. > > I see your point. Suggestions for alternative name? > GetAnnexBParameterSetsFromConfig? Convert[Config]ToAnnexB[ParameterSets]() "Config" is complete but maybe unnecessary. PamaremterSets is probably a good idea but could be dropped. http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:78: // TODO(fgalligan): Use hmac.Verify(). Thanks. I think this is now landed. I didn't realize he was working on it. Anyway, you'll probably get a rebase conflict and can delete this. http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:213: scoped_refptr<DecoderBuffer> CreateWebMEncryptedBuffer(const uint8* data, For consistency, pair params these up like the function below http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:293: const uint8* plain_text, int plain_text_size) { These two params aren't needed. I guess a WrongKey test (non-WebM) could theoretically use them, but we don't have one. http://codereview.chromium.org/10651006/diff/81001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:437: TEST_F(AesDecryptorTest, SubsampleDecryption) { Should have a test for non-WebM no subsample info case too. http://codereview.chromium.org/10651006/diff/81001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator.cc (right): http://codereview.chromium.org/10651006/diff/81001/media/mp4/track_run_iterat... media/mp4/track_run_iterator.cc:422: std::string(), // No checksum in CENC MP4 Period after comments. CENC after container seems odd. MP4 CENC? ISO CENC? http://codereview.chromium.org/10651006/diff/81001/media/mp4/track_run_iterat... File media/mp4/track_run_iterator_unittest.cc (right): http://codereview.chromium.org/10651006/diff/81001/media/mp4/track_run_iterat... media/mp4/track_run_iterator_unittest.cc:377: TEST_F(TrackRunIteratorTest, PathologicalOrderingTest) { Pathological isn't very descriptive of ordering. "Unexpected"?
https://chromiumcodereview.appspot.com/10651006/diff/67005/media/mp4/mp4_stre... File media/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/67005/media/mp4/mp4_stre... media/mp4/mp4_stream_parser.cc:291: RCHECK(AVC::ConvertParameterSetsToAnnexB(avc_config, ¶m_sets)); On 2012/07/25 07:13:47, ddorwin wrote: > On 2012/07/25 01:05:13, strobe wrote: > > On 2012/07/24 01:00:10, ddorwin wrote: > > > This is a bit confusing because "ParameterSets" would seem to be the input > > from > > > the function name, but param_sets is an output parameter. If avc_config > > contains > > > "paramater sets" and param_sets is AnnexB format, I guess that's fine, but a > > > little confusing to read. > > > > I see your point. Suggestions for alternative name? > > GetAnnexBParameterSetsFromConfig? > > Convert[Config]ToAnnexB[ParameterSets]() > "Config" is complete but maybe unnecessary. PamaremterSets is probably a good > idea but could be dropped. Done. https://chromiumcodereview.appspot.com/10651006/diff/81001/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/81001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:213: scoped_refptr<DecoderBuffer> CreateWebMEncryptedBuffer(const uint8* data, On 2012/07/25 07:13:47, ddorwin wrote: > For consistency, pair params these up like the function below Done. https://chromiumcodereview.appspot.com/10651006/diff/81001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:293: const uint8* plain_text, int plain_text_size) { On 2012/07/25 07:13:47, ddorwin wrote: > These two params aren't needed. > I guess a WrongKey test (non-WebM) could theoretically use them, but we don't > have one. Done. https://chromiumcodereview.appspot.com/10651006/diff/81001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:437: TEST_F(AesDecryptorTest, SubsampleDecryption) { On 2012/07/25 07:13:47, ddorwin wrote: > Should have a test for non-WebM no subsample info case too. Done. https://chromiumcodereview.appspot.com/10651006/diff/81001/media/mp4/track_ru... File media/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/81001/media/mp4/track_ru... media/mp4/track_run_iterator.cc:422: std::string(), // No checksum in CENC MP4 On 2012/07/25 07:13:47, ddorwin wrote: > Period after comments. > > CENC after container seems odd. > MP4 CENC? > ISO CENC? Done. https://chromiumcodereview.appspot.com/10651006/diff/81001/media/mp4/track_ru... File media/mp4/track_run_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/10651006/diff/81001/media/mp4/track_ru... media/mp4/track_run_iterator_unittest.cc:377: TEST_F(TrackRunIteratorTest, PathologicalOrderingTest) { On 2012/07/25 07:13:47, ddorwin wrote: > Pathological isn't very descriptive of ordering. "Unexpected"? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/strobe@google.com/10651006/84005
Change committed as 148453 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
