Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1998333002:
MP4 support for Common Encryption 'cbcs' scheme. (Closed)
DescriptionMP4 support for Common Encryption 'cbcs' scheme.
In order to support SAMPLE-AES encrypted packed audio streams
in HLS, the Cast media player library cast will reformat
them into MP4. The change herein, by adding support for
the 'cbcs' scheme will allow them to be decrypted by the platform.
BUG=568326
Committed: https://crrev.com/9554f3d53f6bd4db8b797b189fbfc21722c7d106
Cr-Commit-Position: refs/heads/master@{#435739}
Patch Set 1 #Patch Set 2 : fix a problem in hasty change before upload #
Total comments: 20
Patch Set 3 : accommodate comments #
Total comments: 8
Patch Set 4 : handle comments #
Total comments: 53
Patch Set 5 : rebase #Patch Set 6 : ddorwin more detailed comments #
Total comments: 11
Patch Set 7 : rebase #Patch Set 8 : kq nits #
Total comments: 10
Patch Set 9 : rebase #Patch Set 10 : possibly unneeded rebase upload #Patch Set 11 : fix CL build err unused var #Patch Set 12 : another unused var #Patch Set 13 : some ddorwin comments #Patch Set 14 : another ddorwin comment #
Messages
Total messages: 48 (24 generated)
Description was changed from ========== MP4 support for Common Encryption 2nd & 3rd Editions. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the newer CENC schemes (notably 'cbcs') will allow them to be decrypted by the Cast platform. BUG=568326 ========== to ========== MP4 support for Common Encryption 2nd & 3rd Editions. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the newer CENC schemes (notably 'cbcs') will allow them to be decrypted by the platform. BUG=568326 ==========
dougsteed@chromium.org changed reviewers: + ddorwin@chromium.org, halliwell@chromium.org, kqyang@chromium.org, xhwang@chromium.org
Please hold off on reviewing for now. Made some last minute changes before submission that are not correct. Will update again shortly. Sorry.
On 2016/05/20 22:49:02, dougsteed wrote: > Please hold off on reviewing for now. Made some last minute changes before > submission that are not correct. Will update again shortly. Sorry. OK, fixed the problem. Please take a look.
Looks good with a couple of nits https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/box_definitions.cc:252: reader->Read1(&default_iv_size) && reader->ReadVec(&default_kid, 16)); s/16/kKeyIdSize/ https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/box_definitions.cc:256: #if BUILDFLAG(ENABLE_CENC_NEW_EDITIONS) I'd prefer as less preprocessor primitive in the code as possible. I think we can rename the flag to ENABLE_CENC_PATTERN_ENCRYPTION and have it in HasSupportedScheme() alone should be sufficient. +ddorwin, do you have a preference / guideline or preprocessor conditions? https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/box_definitions.h (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/box_definitions.h:136: uint8_t default_constant_iv_size; Please initialize the values in the constructor or here. Similar below. https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/mp4_stream_parser.cc:75: } nit: } // namespace https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/track_run_iterator.cc (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/track_run_iterator.cc:424: : info_entry->constant_iv; Add a RCHECK or DCHECK that constant_iv_size != 0? https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/track_run_iterator.cc:434: track_encryption->default_skip_byte_block); The two RCHECKs should be enclosed in index != 0 or info_entry != null_ptr.
https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/box_definitions.cc (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/box_definitions.cc:256: #if BUILDFLAG(ENABLE_CENC_NEW_EDITIONS) On 2016/05/23 20:57:34, kqyang wrote: > I'd prefer as less preprocessor primitive in the code as possible. I think we > can rename the flag to ENABLE_CENC_PATTERN_ENCRYPTION and have it in > HasSupportedScheme() alone should be sufficient. > > +ddorwin, do you have a preference / guideline or preprocessor conditions? I'm worried about all the ifdef'd code being added, but I'm also wary of including code in all builds that is not used by nearly all Chrome platforms. Most importantly, though, I think this flag name is inaccurate. See my comments in the .gni file. https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/box_definitions.cc:307: return fourCC == FOURCC_CBC1 || fourCC == FOURCC_CBCS || Are there actual use cases for the other two? If not, I think we should avoid advertising them unless there is a proven need. https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... File media/formats/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/formats/mp... media/formats/mp4/mp4_stream_parser.cc:64: break; Should this be a DLOG that we don't recognize the scheme? https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/media_opti... File media/media_options.gni (right): https://chromiumcodereview.appspot.com/1998333002/diff/20001/media/media_opti... media/media_options.gni:42: # Enable support for newer editions of MPEG Common Encryption (CENC) This comment and variable (and define) name are inaccurate. This specifically adds support for additional schemes. EME is by definition referencing 3rd edition now, but only 'cenc' is currently supported in the registry. Thus, this should be something like enable_additional_cenc_schemes. Or maybe just enable_cbcs.
https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:252: reader->Read1(&default_iv_size) && reader->ReadVec(&default_kid, 16)); On 2016/05/23 20:57:34, kqyang wrote: > s/16/kKeyIdSize/ Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:256: #if BUILDFLAG(ENABLE_CENC_NEW_EDITIONS) On 2016/05/24 23:25:03, ddorwin wrote: > On 2016/05/23 20:57:34, kqyang wrote: > > I'd prefer as less preprocessor primitive in the code as possible. I think we > > can rename the flag to ENABLE_CENC_PATTERN_ENCRYPTION and have it in > > HasSupportedScheme() alone should be sufficient. > > > > +ddorwin, do you have a preference / guideline or preprocessor conditions? > > I'm worried about all the ifdef'd code being added, but I'm also wary of > including code in all builds that is not used by nearly all Chrome platforms. > > Most importantly, though, I think this flag name is inaccurate. See my comments > in the .gni file. Changed the name as suggested. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:307: return fourCC == FOURCC_CBC1 || fourCC == FOURCC_CBCS || On 2016/05/24 23:25:03, ddorwin wrote: > Are there actual use cases for the other two? If not, I think we should avoid > advertising them unless there is a proven need. Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:136: uint8_t default_constant_iv_size; On 2016/05/23 20:57:34, kqyang wrote: > Please initialize the values in the constructor or here. Similar below. Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:64: break; On 2016/05/24 23:25:03, ddorwin wrote: > Should this be a DLOG that we don't recognize the scheme? Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:75: } On 2016/05/23 20:57:34, kqyang wrote: > nit: } // namespace Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:424: : info_entry->constant_iv; On 2016/05/23 20:57:34, kqyang wrote: > Add a RCHECK or DCHECK that constant_iv_size != 0? Done. https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:434: track_encryption->default_skip_byte_block); On 2016/05/23 20:57:34, kqyang wrote: > The two RCHECKs should be enclosed in index != 0 or info_entry != null_ptr. good catch, thank you. https://codereview.chromium.org/1998333002/diff/20001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/1998333002/diff/20001/media/media_options.gni... media/media_options.gni:42: # Enable support for newer editions of MPEG Common Encryption (CENC) On 2016/05/24 23:25:03, ddorwin wrote: > This comment and variable (and define) name are inaccurate. This specifically > adds support for additional schemes. EME is by definition referencing 3rd > edition now, but only 'cenc' is currently supported in the registry. Thus, this > should be something like enable_additional_cenc_schemes. Or maybe just > enable_cbcs. I see. In this name I had intended to mean 'cenc' as the abbreviation for the "common encryption" standard, rather than the 'cenc' scheme itself. Quite ambiguous.
Description was changed from ========== MP4 support for Common Encryption 2nd & 3rd Editions. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the newer CENC schemes (notably 'cbcs') will allow them to be decrypted by the platform. BUG=568326 ========== to ========== MP4 support for Common Encryption 'cbcs' scheme. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the 'cbcs' scheme will allow them to be decrypted by the platform. BUG=568326 ==========
lgtm LGTM with nits https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:136: uint8_t default_constant_iv_size; On 2016/05/25 17:23:21, dougsteed wrote: > On 2016/05/23 20:57:34, kqyang wrote: > > Please initialize the values in the constructor or here. Similar below. > > Done. Initialize is_encrypted and default_iv_size here as well to be consistent? https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:438: #endif Consider add a new unittest in track_run_iterator_unittest.cc
sorry, another comment https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (left): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:418: } The implementation here only supports the case when SampleEncryption 'senc' box is present. However, according to the spec, 'senc' is optional and can be omitted. For the case of 'senc' absent, can you either implement it or add a logging stating that it is not implemented? (when constant iv is applied to non video tracks, I am not sure if it is still meaningful to generate 'senc' box which is empty; the spec says that the sample auxiliary information would then be empty and should be omitted - it does not say anything on 'senc' or 'saio' or 'saiz' explicitly though)
LG other than comments. I defer to KQ on the details. https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:130: bool is_encrypted; As KQ noted, initialize all the values consistently. The current pattern is in the constructor, so we should probably stick with that unless you want to change all of these structs in a separate CL. https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/fourc... File media/formats/mp4/fourccs.h (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/fourc... media/formats/mp4/fourccs.h:25: FOURCC_CBC1 = 0x63626331, Should we remove these? Probably, unless this is intended to be a able of everything.
https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:130: bool is_encrypted; On 2016/05/27 23:05:59, ddorwin wrote: > As KQ noted, initialize all the values consistently. The current pattern is in > the constructor, so we should probably stick with that unless you want to change > all of these structs in a separate CL. Done. https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/fourc... File media/formats/mp4/fourccs.h (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/fourc... media/formats/mp4/fourccs.h:25: FOURCC_CBC1 = 0x63626331, On 2016/05/27 23:05:59, ddorwin wrote: > Should we remove these? Probably, unless this is intended to be a able of > everything. Done. https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (left): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:418: } On 2016/05/27 17:45:58, kqyang wrote: > The implementation here only supports the case when SampleEncryption 'senc' box > is present. However, according to the spec, 'senc' is optional and can be > omitted. For the case of 'senc' absent, can you either implement it or add a > logging stating that it is not implemented? > > (when constant iv is applied to non video tracks, I am not sure if it is still > meaningful to generate 'senc' box which is empty; the spec says that the sample > auxiliary information would then be empty and should be omitted - it does not > say anything on 'senc' or 'saio' or 'saiz' explicitly though) Done. I added code to cover the other cases. https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/40001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:438: #endif On 2016/05/26 17:34:50, kqyang wrote: > Consider add a new unittest in track_run_iterator_unittest.cc Done.
I reviewed the code in more detail. I did not review track_run_iterator_unittest.cc. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:166: // 64-bit (8-byte) or 128-bit (16-byte). Subsequent editions allow |iv_size| nit: It would be nice to specify which edition added this. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:168: // the constant IV must be ensured by the caller. Do you mean the caller of this function? I assume that is implemented in this CL. Is that line 273? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:260: RCHECK(reader->ReadFullBoxHeader() && reader->SkipBytes(1) && What is being skipped? Should we document this? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:261: reader->Read1(&possible_pattern_info) && reader->Read1(&flag) && Did they use reserved bits or something? Why is this just possible? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:267: #if BUILDFLAG(ENABLE_CBCS_ENCRYPTION_SCHEME) Should this be inside the else if - with a #else that causes this to fail? I guess line 280 covers this, but (as with the comments below) it might be easier to understand other ways / the way it was. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:268: } else if (is_encrypted) { What is the relationship between this block and default_iv_size? For CBCS is it 0? It seems it would be easier to understand if the IV size check was inside is_encrypted even though that might be more logic. Also, this block only applies to cbcs, but do we already know we are using cbcs? If not, it seems we should have something like: if (foo) { // Must be 'cbcs' (or must be a skip scheme). https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:318: return fourCC == FOURCC_CBCS; nit: If you follow the style for CENC, you don't need #else and at least the return false; is always compiled. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1153: if (iv_size != 0) { ditto on structure note to self: I skipped this. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1224: reader->MaybeReadChild(&sample_encryption)); Was this missed in a previous CL? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:134: uint8_t default_crypt_byte_block; Is "crypt" a common term in the spec? If not, use "encrypted" - abbreviations are discouraged unless common. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:135: uint8_t default_skip_byte_block; Is this a count? If so, specify that because it's not obvious. Same above. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:300: uint8_t crypt_byte_block; ditto on names https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:41: return Unencrypted(); Is that the best way to handle this? We know it is encrypted. Or is the caller supposed to know to handle this as an unsupported scheme, in which case we should comment this above this function? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:45: bool pattern_encryption = false; nit: is_/uses_... https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:57: DLOG(WARNING) << "Unsupported encryption scheme: " << fourCC; Should this be a DCHECK based on line 40? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:62: uint8_t crypt = sinf.info.track_encryption.default_crypt_byte_block; ditto on "crypt" https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:355: is_audio_track_encrypted_ ? GetEncryptionScheme(entry.sinf) Continuing from line 41, we know it's encrypted, but if the scheme is not recognized, we just say "unencrypted". We should probably fail in some way rather than silently continuing, which will be harder to debug. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:417: // if we don't have a per-sample IV, get the constant IV. nit: If https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:430: // We only support setting the pattern values in the 'tenc' box for nit: Is this dependent on the logic above? If not could we put it at 417 to make this clear? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:435: RCHECK(info_entry->crypt_byte_block == If this is legal but not supported, it would be nice to log this somewhere. Ideally to media-internals where authors can see it, but at least a LOG(). https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:631: // Later editions of the CENC standard, notably the 'cbcs' scheme, allow Specify the first edition. Or maybe you don't actually need to refer to the editions and just need to note that some schemes, including 'cbcs', ... https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:633: // That case will fall through to here. Should we DCHECK that the scheme is not CENC (outside the ifdef)? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:637: std::string(reinterpret_cast<const char*>(&kid[0]), kid.size()), is kid guaranteed not to be empty? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:699: return true; Explain why? https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:704: : GetSampleEncryptionInfoEntry(*run_itr_, index)->constant_iv_size; GetSampleEncryptionInfoEntry can return nullptr. You need to check the return value before dereferencing. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:708: : GetSampleEncryptionInfoEntry(*run_itr_, index)->constant_iv; ditto
I think this CL needs some love. It is needed for CMAF support. dougsteed@ are you still working on it? Thanks.
Yes, working on it as we speak. On Fri, Oct 7, 2016 at 10:52 AM, <kqyang@chromium.org> wrote: > I think this CL needs some love. It is needed for CMAF support. > > dougsteed@ are you still working on it? > > Thanks. > > https://codereview.chromium.org/1998333002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Cool. Thanks! On 2016/10/07 18:01:58, chromium-reviews wrote: > Yes, working on it as we speak. > > On Fri, Oct 7, 2016 at 10:52 AM, <mailto:kqyang@chromium.org> wrote: > > > I think this CL needs some love. It is needed for CMAF support. > > > > dougsteed@ are you still working on it? > > > > Thanks. > > > > https://codereview.chromium.org/1998333002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
kq: I think I got all the comments, PTAL https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:166: // 64-bit (8-byte) or 128-bit (16-byte). Subsequent editions allow |iv_size| On 2016/06/17 23:32:40, ddorwin wrote: > nit: It would be nice to specify which edition added this. Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:168: // the constant IV must be ensured by the caller. On 2016/06/17 23:32:40, ddorwin wrote: > Do you mean the caller of this function? I assume that is implemented in this > CL. Is that line 273? Actually it's in track_run_iterator.cc, line 502. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:260: RCHECK(reader->ReadFullBoxHeader() && reader->SkipBytes(1) && On 2016/06/17 23:32:40, ddorwin wrote: > What is being skipped? Should we document this? A reserved byte. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:261: reader->Read1(&possible_pattern_info) && reader->Read1(&flag) && On 2016/06/17 23:32:40, ddorwin wrote: > Did they use reserved bits or something? Why is this just possible? Yes. There were two reserved bytes. They reassigned the second one to be the pattern, but only if the isProtected flag is set, and only if the version is 1 or greater. I named it "possible" because the value is read before it is known whether it is to be interpreted as pattern info or not (it may turn out to be skipped). https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:267: #if BUILDFLAG(ENABLE_CBCS_ENCRYPTION_SCHEME) On 2016/06/17 23:32:40, ddorwin wrote: > Should this be inside the else if - with a #else that causes this to fail? I > guess line 280 covers this, but (as with the comments below) it might be easier > to understand other ways / the way it was. I agree. It was unclear. Rearranged it a bit to (hopefully) improve clarity. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:268: } else if (is_encrypted) { On 2016/06/17 23:32:40, ddorwin wrote: > What is the relationship between this block and default_iv_size? For CBCS is it > 0? > > It seems it would be easier to understand if the IV size check was inside > is_encrypted even though that might be more logic. > > Also, this block only applies to cbcs, but do we already know we are using cbcs? > If not, it seems we should have something like: > if (foo) { > // Must be 'cbcs' (or must be a skip scheme). see above. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:318: return fourCC == FOURCC_CBCS; On 2016/06/17 23:32:40, ddorwin wrote: > nit: If you follow the style for CENC, you don't need #else and at least the > return false; is always compiled. Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1153: if (iv_size != 0) { On 2016/06/17 23:32:40, ddorwin wrote: > ditto on structure > note to self: I skipped this. Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1224: reader->MaybeReadChild(&sample_encryption)); On 2016/06/17 23:32:40, ddorwin wrote: > Was this missed in a previous CL? It looks like this box was added in the 2nd Edition (at least it is not mentioned in the 1st Edition). https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:134: uint8_t default_crypt_byte_block; On 2016/06/17 23:32:40, ddorwin wrote: > Is "crypt" a common term in the spec? If not, use "encrypted" - abbreviations > are discouraged unless common. yes, this is the term used in the spec. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:135: uint8_t default_skip_byte_block; On 2016/06/17 23:32:40, ddorwin wrote: > Is this a count? If so, specify that because it's not obvious. Same above. I used the exact term in the spec that names this field. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.h:300: uint8_t crypt_byte_block; On 2016/06/17 23:32:40, ddorwin wrote: > ditto on names ditto. This is the actual name for the field in the standard. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:41: return Unencrypted(); On 2016/06/17 23:32:41, ddorwin wrote: > Is that the best way to handle this? We know it is encrypted. Or is the caller > supposed to know to handle this as an unsupported scheme, in which case we > should comment this above this function? Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:45: bool pattern_encryption = false; On 2016/06/17 23:32:41, ddorwin wrote: > nit: is_/uses_... Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:57: DLOG(WARNING) << "Unsupported encryption scheme: " << fourCC; On 2016/06/17 23:32:40, ddorwin wrote: > Should this be a DCHECK based on line 40? Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:62: uint8_t crypt = sinf.info.track_encryption.default_crypt_byte_block; On 2016/06/17 23:32:40, ddorwin wrote: > ditto on "crypt" as mentioned above, this is the term used in the standard. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:355: is_audio_track_encrypted_ ? GetEncryptionScheme(entry.sinf) On 2016/06/17 23:32:40, ddorwin wrote: > Continuing from line 41, we know it's encrypted, but if the scheme is not > recognized, we just say "unencrypted". We should probably fail in some way > rather than silently continuing, which will be harder to debug. Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:417: // if we don't have a per-sample IV, get the constant IV. On 2016/06/17 23:32:41, ddorwin wrote: > nit: If Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:430: // We only support setting the pattern values in the 'tenc' box for On 2016/06/17 23:32:41, ddorwin wrote: > nit: Is this dependent on the logic above? If not could we put it at 417 to make > this clear? Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:435: RCHECK(info_entry->crypt_byte_block == On 2016/06/17 23:32:41, ddorwin wrote: > If this is legal but not supported, it would be nice to log this somewhere. > Ideally to media-internals where authors can see it, but at least a LOG(). RCHECK will log this. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:631: // Later editions of the CENC standard, notably the 'cbcs' scheme, allow On 2016/06/17 23:32:41, ddorwin wrote: > Specify the first edition. > Or maybe you don't actually need to refer to the editions and just need to note > that some schemes, including 'cbcs', ... Done. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:633: // That case will fall through to here. On 2016/06/17 23:32:41, ddorwin wrote: > Should we DCHECK that the scheme is not CENC (outside the ifdef)? I'm not sure we independently know the scheme in this context. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:637: std::string(reinterpret_cast<const char*>(&kid[0]), kid.size()), On 2016/06/17 23:32:41, ddorwin wrote: > is kid guaranteed not to be empty? I believe so, same usage as in (new) line 661. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:704: : GetSampleEncryptionInfoEntry(*run_itr_, index)->constant_iv_size; On 2016/06/17 23:32:41, ddorwin wrote: > GetSampleEncryptionInfoEntry can return nullptr. You need to check the return > value before dereferencing. This mimics the logic in the previous two functions. GetSampleEncryptionInfoEntry has a DCHECK to check it is not returning nullptr. I suspect there is a prior check that the sample_index is valid. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:708: : GetSampleEncryptionInfoEntry(*run_itr_, index)->constant_iv; On 2016/06/17 23:32:41, ddorwin wrote: > ditto ditto
Do you need the code to support other schemes than cenc and cbcs ? I had originally implemented at least parts of the other schemes (cens & cbc1). But since we in Cast team currently needed to add only cbcs (to support HLS with SAMPLE-AES) and I had not fully fleshed out the other schemes, ddorwin@ suggested it might be better to restrict the scope to cbcs. (I did so, although I realize I never actually got round to downscoping the CL title). /Doug On Fri, Oct 7, 2016 at 11:03 AM, <kqyang@chromium.org> wrote: > Cool. Thanks! > > On 2016/10/07 18:01:58, chromium-reviews wrote: > > Yes, working on it as we speak. > > > > On Fri, Oct 7, 2016 at 10:52 AM, <mailto:kqyang@chromium.org> wrote: > > > > > I think this CL needs some love. It is needed for CMAF support. > > > > > > dougsteed@ are you still working on it? > > > > > > Thanks. > > > > > > https://codereview.chromium.org/1998333002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/1998333002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No, we are already interested in cenc and cbcs as well at this moment :) And thanks for working on this. I'll review the code some time today or tomorrow. -- KongQun Yang (KQ) On Mon, Oct 10, 2016 at 3:10 PM, Doug Steedman <dougsteed@google.com> wrote: > Do you need the code to support other schemes than cenc and cbcs ? > I had originally implemented at least parts of the other schemes (cens & > cbc1). > > But since we in Cast team currently needed to add only cbcs (to support > HLS with SAMPLE-AES) and I had not fully fleshed out the other schemes, > ddorwin@ suggested it might be better to restrict the scope to cbcs. > (I did so, although I realize I never actually got round to downscoping > the CL title). > > /Doug > > On Fri, Oct 7, 2016 at 11:03 AM, <kqyang@chromium.org> wrote: > >> Cool. Thanks! >> >> On 2016/10/07 18:01:58, chromium-reviews wrote: >> > Yes, working on it as we speak. >> > >> > On Fri, Oct 7, 2016 at 10:52 AM, <mailto:kqyang@chromium.org> wrote: >> > >> > > I think this CL needs some love. It is needed for CMAF support. >> > > >> > > dougsteed@ are you still working on it? >> > > >> > > Thanks. >> > > >> > > https://codereview.chromium.org/1998333002/ >> > > >> > >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> >> https://codereview.chromium.org/1998333002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looks good with some nits https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:276: (default_constant_iv_size == 8 || default_constant_iv_size == 16)); nit: prefer a separate RCHECK for this line Same below. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:282: RCHECK(default_iv_size == 8 || default_iv_size == 16); Prefer duplicate this line in #if block, something like: #if BUILDFLAG(ENABLE_CBCS_ENCRYPTION_SCHEME) ... } else { RCHECK(default_iv_size == 8 || default_iv_size == 16); } #else RCHECK(default_iv_size == 8 || default_iv_size == 16); #endif Same below. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:45: FourCC fourCC = sinf.type.type; s/fourCC/fourcc/ variable name should not be capitalized. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:361: sample_per_second, extra_data, scheme, I don't think it is a good idea to pass encryption scheme in codec configs. CDM may not know codec configs, e.g. for decrypt only operations. Encryption scheme should be passed in DecryptConfig instead. That said, I am fine with check in this code first and refactor later. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.cc:709: memcpy(entry.initialization_vector, constant_iv, constant_iv_size); It is safer to do: memcpy(entry.initialization_vector, constant_iv, sizeof(constant_iv)); Since both entry.initialization_vector and constant_iv are 16-byte array; if constant_iv_size is 8, the extra 8 bytes in constant_iv is zeroed, the extra 8 bytes in entry.initialization_vector should also be zeroed. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... File media/formats/mp4/track_run_iterator.h (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.h:104: bool ApplyConstantIv(size_t sample_index, SampleEncryptionEntry& entry) const; s/SampleEncryptionEntry&/SampleEncryptionEntry*/ https://google.github.io/styleguide/cppguide.html#Reference_Arguments
https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:276: (default_constant_iv_size == 8 || default_constant_iv_size == 16)); On 2016/10/12 22:16:52, kqyang wrote: > nit: prefer a separate RCHECK for this line > > Same below. Done. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:282: RCHECK(default_iv_size == 8 || default_iv_size == 16); On 2016/10/12 22:16:52, kqyang wrote: > Prefer duplicate this line in #if block, something like: > > #if BUILDFLAG(ENABLE_CBCS_ENCRYPTION_SCHEME) > ... > } else { > RCHECK(default_iv_size == 8 || default_iv_size == 16); > } > #else > RCHECK(default_iv_size == 8 || default_iv_size == 16); > #endif > > Same below. Done. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:45: FourCC fourCC = sinf.type.type; On 2016/10/12 22:16:52, kqyang wrote: > s/fourCC/fourcc/ > > variable name should not be capitalized. Done. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:361: sample_per_second, extra_data, scheme, On 2016/10/12 22:16:52, kqyang wrote: > I don't think it is a good idea to pass encryption scheme in codec configs. CDM > may not know codec configs, e.g. for decrypt only operations. > > Encryption scheme should be passed in DecryptConfig instead. > > That said, I am fine with check in this code first and refactor later. Let's discuss further. I'm not sure I agree that scheme should be passed in DecryptConfig. The latter is specific to a particular sample, whereas the scheme is really associated with all usage of the key and has a much longer scope. That is why I have viewed the scheme as a generalization of the boolean is_encrypted that was in the code config before. https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... File media/formats/mp4/track_run_iterator.h (right): https://codereview.chromium.org/1998333002/diff/100001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.h:104: bool ApplyConstantIv(size_t sample_index, SampleEncryptionEntry& entry) const; On 2016/10/12 22:16:52, kqyang wrote: > s/SampleEncryptionEntry&/SampleEncryptionEntry*/ > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments Done.
Thanks. Still LGTM.
LGTM approval based on my rough review and kqyang's detailed review. https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:435: RCHECK(info_entry->crypt_byte_block == On 2016/10/10 18:18:02, dougsteed wrote: > On 2016/06/17 23:32:41, ddorwin wrote: > > If this is legal but not supported, it would be nice to log this somewhere. > > Ideally to media-internals where authors can see it, but at least a LOG(). > > RCHECK will log this. RCHECK DLOGs. RCHECK_MEDIA_LOGGED probably logs to media-internals. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:263: RCHECK(reader->ReadFullBoxHeader() && reader->SkipBytes(1) && RCHECK(reader->ReadFullBoxHeader() && reader->SkipBytes(1) && // Skip reserved byte. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:315: // non-CENC protection scheme types. It is the parent box's responsibility to ...for unsupported CENC cipher modes or non-CENC protection schemes ? https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.h:28: const int kInitializationVectorSize = 16; Is this always constant for all types of CENC? https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.cc:218: return (group_description_index > entries->size()) This duplicated logic is not immediately clear. Can we change this to an if statement that NOTREACHED() and returns nullptr followed by returning the happy path value? Maybe do this in a ceparate CL. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.cc:698: return true; Explain why? Or should we never get here, in which case this should be a DCHECK.
The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/60001/media/formats/mp4/track... media/formats/mp4/track_run_iterator.cc:435: RCHECK(info_entry->crypt_byte_block == On 2016/11/06 00:57:51, ddorwin wrote: > On 2016/10/10 18:18:02, dougsteed wrote: > > On 2016/06/17 23:32:41, ddorwin wrote: > > > If this is legal but not supported, it would be nice to log this somewhere. > > > Ideally to media-internals where authors can see it, but at least a LOG(). > > > > RCHECK will log this. > > RCHECK DLOGs. RCHECK_MEDIA_LOGGED probably logs to media-internals. Done. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:263: RCHECK(reader->ReadFullBoxHeader() && reader->SkipBytes(1) && On 2016/11/06 00:57:51, ddorwin wrote: > RCHECK(reader->ReadFullBoxHeader() && > reader->SkipBytes(1) && // Skip reserved byte. Done. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:315: // non-CENC protection scheme types. It is the parent box's responsibility to On 2016/11/06 00:57:51, ddorwin wrote: > ...for unsupported CENC cipher modes or non-CENC protection schemes ? Done. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.h (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/box_... media/formats/mp4/box_definitions.h:28: const int kInitializationVectorSize = 16; On 2016/11/06 00:57:51, ddorwin wrote: > Is this always constant for all types of CENC? Added comment. IV may be shorter but this is the size needed for storage. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.cc:218: return (group_description_index > entries->size()) On 2016/11/06 00:57:51, ddorwin wrote: > This duplicated logic is not immediately clear. Can we change this to an if > statement that NOTREACHED() and returns nullptr followed by returning the happy > path value? > Maybe do this in a separate CL. Separate CL sounds good. However I don't think this should be a DCHECK, since I don't see where the range of a value parsed from the stream is checked, and we shouldn't be DCHECKing based on untrusted input. Probably should just remove the DCHECK or replace with RCHECK. https://codereview.chromium.org/1998333002/diff/140001/media/formats/mp4/trac... media/formats/mp4/track_run_iterator.cc:698: return true; On 2016/11/06 00:57:51, ddorwin wrote: > Explain why? Or should we never get here, in which case this should be a DCHECK. Done.
The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougsteed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, kqyang@chromium.org Link to the patchset: https://codereview.chromium.org/1998333002/#ps260001 (title: "another ddorwin comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1480630984592770,
"parent_rev": "4d4d22108cdcf30ee64366d6e1de9a84b5a53455", "commit_rev":
"1c723f2888ca00b7b1b8d482417657196f81bd6e"}
Message was sent while issue was closed.
Description was changed from ========== MP4 support for Common Encryption 'cbcs' scheme. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the 'cbcs' scheme will allow them to be decrypted by the platform. BUG=568326 ========== to ========== MP4 support for Common Encryption 'cbcs' scheme. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the 'cbcs' scheme will allow them to be decrypted by the platform. BUG=568326 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== MP4 support for Common Encryption 'cbcs' scheme. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the 'cbcs' scheme will allow them to be decrypted by the platform. BUG=568326 ========== to ========== MP4 support for Common Encryption 'cbcs' scheme. In order to support SAMPLE-AES encrypted packed audio streams in HLS, the Cast media player library cast will reformat them into MP4. The change herein, by adding support for the 'cbcs' scheme will allow them to be decrypted by the platform. BUG=568326 Committed: https://crrev.com/9554f3d53f6bd4db8b797b189fbfc21722c7d106 Cr-Commit-Position: refs/heads/master@{#435739} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9554f3d53f6bd4db8b797b189fbfc21722c7d106 Cr-Commit-Position: refs/heads/master@{#435739} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
