Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1490613005:
media config: expand is_encrypted to a struct. (Closed)
|
Created:
5 years ago by dougsteed Modified:
4 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, gunsch+watch_chromium.org, halliwell+watch_chromium.org, lcwu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia config: expand is_encrypted to a struct.
Provide more complete encryption metadata, rather than
just a bool. EncryptionScheme also allows specification
of the mode and the pattern, as will be allowed by
CENC (ISO's Common Encryption standard), 3rd Edition.
BUG=568326
Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f
Cr-Commit-Position: refs/heads/master@{#380710}
Committed: https://crrev.com/8d5275f7bbb6f79c83829a7931f1500128ef3f8f
Cr-Commit-Position: refs/heads/master@{#380791}
Patch Set 1 #
Total comments: 81
Patch Set 2 : #Patch Set 3 : accommodate comments on patchset 1 #Patch Set 4 : missed one change #Patch Set 5 : rebasing #Patch Set 6 : pepper depends on VideoDecoderConfig #Patch Set 7 : one more tweak in chromecast/common #
Total comments: 40
Patch Set 8 : #Patch Set 9 : rebase #Patch Set 10 : #Patch Set 11 : rebase #Patch Set 12 : rebase redux #Patch Set 13 : mojo changes; Message->base::Pickle #
Total comments: 52
Patch Set 14 : ddorwin comments #
Total comments: 25
Patch Set 15 : more ddorwin comments #
Total comments: 41
Patch Set 16 : xhwang comments mainly #Patch Set 17 : missed a couple comments #
Total comments: 11
Patch Set 18 : more xhwang comments #
Total comments: 10
Patch Set 19 : "tiny nits" #Patch Set 20 : rebase from master #Patch Set 21 : missed some mojo tweaks #Patch Set 22 : missed a change needed in content #Patch Set 23 : rebase needed #Patch Set 24 : add MEDIA_EXPORT to inner class #Patch Set 25 : oops #
Dependent Patchsets: Messages
Total messages: 79 (31 generated)
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as allowed by the later versions of CENC (ISO's Common Encryption standard). BUG= ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as allowed by the later versions of CENC (ISO's Common Encryption standard). BUG= ==========
dougsteed@chromium.org changed reviewers: + yucliu@chromium.org
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as allowed by the later versions of CENC (ISO's Common Encryption standard). BUG= ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as allowed by the later versions of CENC (ISO's Common Encryption standard). BUG=568326 ==========
dougsteed@chromium.org changed reviewers: + dalecurtis@chromium.org, lcwu@chromium.org
dalecurtis@chromium.org changed reviewers: + ddorwin@chromium.org
=> ddorwin
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
https://codereview.chromium.org/1490613005/diff/1/chromecast/common/media/cma... File chromecast/common/media/cma_param_traits.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/common/media/cma... chromecast/common/media/cma_param_traits.cc:28: IPC_ENUM_TRAITS_MAX_VALUE(media::CipherMode, media::kCipherModeMax); This line should be in cma_param_traits_macros.h I think. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.cc:256: #ifdef ENABLE_HLS_SAMPLE_AES Remove this block in this CL?
ddorwin@chromium.org changed reviewers: + xhwang@chromium.org
Most of my comments relate to the design of EncryptionScheme (encryption_scheme.h). What do others think? I did not review chromecast/ in detail. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... File chromecast/media/audio/cast_audio_output_stream.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream.cc:46: audio_config.encryption_scheme.is_encrypted = false; The constructor handles this (as well as all other members of the scheme). It seems odd to set just one of them explicitly here. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... File chromecast/media/audio/cast_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); Are there other members we can/should test? https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); Do we not have any similar tests for non-Chromecast code? Or perhaps those are hidden by is_encrypted() calls. Please find and update any such tests. https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:90: // Algorithm and mode that was used to encrypt the stream. Similar comments as in encryption_scheme.h. https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:124: bool is_encrypted; Public members make things more interesting (and dangerous?) than in encryption_scheme.h. Do these need to be public? https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:129: inline EncryptionScheme::PatternSpec::PatternSpec() Why are constructors and destructors inlined here? They are not supposed to be per https://www.chromium.org/developers/coding-style#TOC-Inline-functions. https://codereview.chromium.org/1490613005/diff/1/media/base/audio_decoder_co... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/audio_decoder_co... media/base/audio_decoder_config.h:110: bool is_encrypted() const { return encryption_scheme_.is_encrypted(); } Again, unclear if this is a "simple accessor." https://codereview.chromium.org/1490613005/diff/1/media/base/audio_decoder_co... media/base/audio_decoder_config.h:110: bool is_encrypted() const { return encryption_scheme_.is_encrypted(); } As mentioned earlier, I think this might be hiding tests that we should update to check encryption_scheme(). https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:7: #include "base/logging.h" I don't think this is currently used, but it may be after addressing one of my comments. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:12: : is_encrypted_(false), mode_(kCipherModeAesCtr), pattern_() {} Why not kCipherModeUnknown? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:41: EncryptionScheme::PatternSpec::PatternSpec() nit: Define in the same order as declared. This internal class was before the constructors. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_scheme.h File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:13: enum CipherMode { Should this be a member of the class? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:17: kCipherModeMax = kCipherModeAesCbc Do we need Max? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:23: // V3 of the CENC standard will add pattern encryption, through two new s/will add/adds/ to avoid comment rot? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:32: // If either or both of encrypt_blocks or skip_blocks is 0, pattern Where does this apply? Is this a rule for this class? Instead can we just disallow passing 0 to the non-default constructor? Better yet, can we use static CreateNoPattern() and CreatePattern(e, s) functions? (See discussion below.) https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:38: ~PatternSpec() {} Do not inline. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:40: void Initialize(uint32_t encrypt_blocks, uint32_t skip_blocks); Why do we need this and the matching constructor? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:43: uint32_t encrypt_blocks() const { return encrypt_blocks_; } DCHECK in_effect()? Will need to move to .cc file. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:46: bool in_effect() const { return encrypt_blocks_ != 0 || skip_blocks_ != 0; } It's unclear whether this is a "simple accessor" (https://www.chromium.org/developers/coding-style#TOC-Inline-functions) or should be in the .cc file. If not, it would also be isInEffect(). In any case, it should use "is" or something similar. Perhaps hasPattern() or usesPattern(). https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:49: uint32_t encrypt_blocks_; Can these be const? (I guess not as long as we have Initialize(), but do we need that?) https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:53: EncryptionScheme(); As above, these constructors would be better as CreateFoo functions. This seems especially true after looking at the implementations in the .cc file as since the object that is created is unclear from some of these signatures. Of course, that would require copying the object or returning a pointer. Copying might not be too bad since the objects are so small. At a minimum, we should reduce the number of different constructors so it is clear what is happening when this object is created. Also, add comments documenting what the remaining ones do. For example, this one creates a "not encrypted" scheme. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:54: EncryptionScheme(bool is_encrypted); explicit https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:54: EncryptionScheme(bool is_encrypted); Do we need this one? Wouldn't the ones above and below it suffice? https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:57: EncryptionScheme(bool is_encrypted, Why do we need this one? Why would you ever pass false with a mode? See also line 72. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:60: ~EncryptionScheme() {} Do not inline. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:71: private: Same question about const members. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:72: bool is_encrypted_; Do we need this? We could rename kCipherModeUnknown to kCipherModeNone or kCipherModeClear and just use that? (This might also help minimize the number of constructors.) https://codereview.chromium.org/1490613005/diff/1/media/base/video_decoder_co... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/video_decoder_co... media/base/video_decoder_config.h:98: bool is_encrypted() const { return encryption_scheme_.is_encrypted(); } ditto * 2 https://codereview.chromium.org/1490613005/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:367: return AVCodecContextToAudioDecoderConfig(stream->codec, is_encrypted, We could create the correct object here. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_adts.cc:229: EncryptionScheme scheme(false); Why do we need a local variable? https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.cc:234: RCHECK(UpdateVideoDecoderConfig(sps, scheme)); ditto https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.cc:256: #ifdef ENABLE_HLS_SAMPLE_AES On 2015/12/10 00:12:19, yucliu1 wrote: > Remove this block in this CL? Agreed. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.h:69: const EncryptionScheme& scheme); Fwd declare? https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:274: EncryptionScheme(is_audio_track_encrypted_), base::TimeDelta(), 0); Perhaps we should have a member documenting the encryption scheme rather than just |is_audio_track_encrypted_|. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:315: std::vector<uint8_t>(), EncryptionScheme(is_video_track_encrypted_)); ditto https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... media/formats/webm/webm_audio_client.cc:81: EncryptionScheme(is_encrypted), Even here, it might make sense to explicitly say AES-CTR no pattern because that is now WebM is specified. https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_vid... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_vid... media/formats/webm/webm_video_client.cc:94: EncryptionScheme(is_encrypted)); ditto
Please also be specific about which version of CENC adds this in the description.
https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_scheme.h File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:72: bool is_encrypted_; It's a bit odd that you have an EncryptionScheme with |is_encrypted| being false for an unencrypted stream. In {Audio|Video}DecoderConfig, can we actually just hold a scoped_ptr<EncryptionScheme> so that when it's null the config is NOT encrypted? That way, we can drop |is_encrypted| here in this class. Also, we can disallow copy and assign. Another benefit is that in cases where the config is not encrypted, you can simply use a nullptr for EncryptionScheme.
I only skimmed the CL and provided a suggestion. I'll take a look again after ddorwin's comments are addressed.
PTAL https://codereview.chromium.org/1490613005/diff/1/chromecast/common/media/cma... File chromecast/common/media/cma_param_traits.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/common/media/cma... chromecast/common/media/cma_param_traits.cc:28: IPC_ENUM_TRAITS_MAX_VALUE(media::CipherMode, media::kCipherModeMax); On 2015/12/10 00:12:19, yucliu1 wrote: > This line should be in cma_param_traits_macros.h I think. Done. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... File chromecast/media/audio/cast_audio_output_stream.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream.cc:46: audio_config.encryption_scheme.is_encrypted = false; On 2015/12/10 18:36:00, ddorwin wrote: > The constructor handles this (as well as all other members of the scheme). It > seems odd to set just one of them explicitly here. Done. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... File chromecast/media/audio/cast_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); On 2015/12/10 18:36:00, ddorwin wrote: > Do we not have any similar tests for non-Chromecast code? Or perhaps those are > hidden by is_encrypted() calls. Please find and update any such tests. Pretty sure I updated all the tests, as it would not compile otherwise. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); On 2015/12/10 18:36:00, ddorwin wrote: > Are there other members we can/should test? Do you mean members of audio_config, or of audio_config.encryption_scheme ? In any case, I am not familiar with the nuances of these tests, so all I am doing is trying to preserve the semantics of the existing test. https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:90: // Algorithm and mode that was used to encrypt the stream. On 2015/12/10 18:36:00, ddorwin wrote: > Similar comments as in encryption_scheme.h. Done. https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:124: bool is_encrypted; On 2015/12/10 18:36:00, ddorwin wrote: > Public members make things more interesting (and dangerous?) than in > encryption_scheme.h. Do these need to be public? This is the house style for the chromecast media API objects, which are defined by structs rather than classes (see the other examples that I didn't touch later in this file. Not sure of the history of that, but not practical to change at this point, as they are used in all the vendor APIs. https://codereview.chromium.org/1490613005/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:129: inline EncryptionScheme::PatternSpec::PatternSpec() On 2015/12/10 18:36:00, ddorwin wrote: > Why are constructors and destructors inlined here? They are not supposed to be > per https://www.chromium.org/developers/coding-style#TOC-Inline-functions. I'm following the existing style for the chromecast config structs (see later in this file). And in fact, there actually IS no cc file for this! Again, I don't know the history, but perhaps it is so the OEMs can use the corresponding APIs in their libraries without having to link in separate cc files from us. And I think this is OK under the guidelines, since there are no complex constructors, they just assign PODs. https://codereview.chromium.org/1490613005/diff/1/media/base/audio_decoder_co... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/audio_decoder_co... media/base/audio_decoder_config.h:110: bool is_encrypted() const { return encryption_scheme_.is_encrypted(); } On 2015/12/10 18:36:00, ddorwin wrote: > As mentioned earlier, I think this might be hiding tests that we should update > to check encryption_scheme(). I don't know all the usages of this class, and I don't want to impose fresh requirements on tests for components that are not adding support for CBC (CENCv3). So my philosophy was to not add any additional constraints on such unaffected classes and tests. If all they are interested in is a boolean "is_encrypted", then they can more or less continue to use EncryptionScheme as if it was a boolean. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:12: : is_encrypted_(false), mode_(kCipherModeAesCtr), pattern_() {} On 2015/12/10 18:36:00, ddorwin wrote: > Why not kCipherModeUnknown? Up until now, AES-CTR is the default (at least for CENC), and I don't know all the places where that is assumed. So I think it is safer to preserve a form of backwards-compatibility where AES-CTR is the default. However, in those places where the code is really just using an is_encrypted boolean, then it doesn't care about the mode anyway, and will never ask for it. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:41: EncryptionScheme::PatternSpec::PatternSpec() On 2015/12/10 18:36:00, ddorwin wrote: > nit: Define in the same order as declared. This internal class was before the > constructors. Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_scheme.h File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:13: enum CipherMode { On 2015/12/10 18:36:01, ddorwin wrote: > Should this be a member of the class? Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:17: kCipherModeMax = kCipherModeAesCbc On 2015/12/10 18:36:01, ddorwin wrote: > Do we need Max? Yes, needed for IPC ParamTraits. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:23: // V3 of the CENC standard will add pattern encryption, through two new On 2015/12/10 18:36:01, ddorwin wrote: > s/will add/adds/ to avoid comment rot? Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:32: // If either or both of encrypt_blocks or skip_blocks is 0, pattern On 2015/12/10 18:36:01, ddorwin wrote: > Where does this apply? Is this a rule for this class? Instead can we just > disallow passing 0 to the non-default constructor? > > Better yet, can we use static CreateNoPattern() and CreatePattern(e, s) > functions? (See discussion below.) I am trying to model the concept of CENCv3, where pattern encryption is in effect if both values are non-zero, and not in effect otherwise. So my interpretation is that either or both being zero is permitted by the class, and has the meaning of rendering pattern encryption not in effect. There is also a similar class in chromecast, and it is much more convenient to translate the objects back and forward by just copying the two values. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:38: ~PatternSpec() {} On 2015/12/10 18:36:01, ddorwin wrote: > Do not inline. Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:40: void Initialize(uint32_t encrypt_blocks, uint32_t skip_blocks); On 2015/12/10 18:36:01, ddorwin wrote: > Why do we need this and the matching constructor? Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:43: uint32_t encrypt_blocks() const { return encrypt_blocks_; } On 2015/12/10 18:36:01, ddorwin wrote: > DCHECK in_effect()? Will need to move to .cc file. No. As mentioned above I still want to access the individual fields even when in_effect is false. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:46: bool in_effect() const { return encrypt_blocks_ != 0 || skip_blocks_ != 0; } On 2015/12/10 18:36:01, ddorwin wrote: > It's unclear whether this is a "simple accessor" > (https://www.chromium.org/developers/coding-style#TOC-Inline-functions) or > should be in the .cc file. If not, it would also be isInEffect(). > > In any case, it should use "is" or something similar. Perhaps hasPattern() or > usesPattern(). Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:49: uint32_t encrypt_blocks_; On 2015/12/10 18:36:01, ddorwin wrote: > Can these be const? > (I guess not as long as we have Initialize(), but do we need that?) No, because want to have copy and assign. Latter requires them not to be const. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:53: EncryptionScheme(); On 2015/12/10 18:36:01, ddorwin wrote: > As above, these constructors would be better as CreateFoo functions. This seems > especially true after looking at the implementations in the .cc file as since > the object that is created is unclear from some of these signatures. > > Of course, that would require copying the object or returning a pointer. Copying > might not be too bad since the objects are so small. > > At a minimum, we should reduce the number of different constructors so it is > clear what is happening when this object is created. > > Also, add comments documenting what the remaining ones do. For example, this one > creates a "not encrypted" scheme. Reduced the number of constructors. Hopefully it is clear now. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:54: EncryptionScheme(bool is_encrypted); On 2015/12/10 18:36:01, ddorwin wrote: > explicit Ouch. This caused a lot of extra files to need to be touched! https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:57: EncryptionScheme(bool is_encrypted, On 2015/12/10 18:36:01, ddorwin wrote: > Why do we need this one? Why would you ever pass false with a mode? See also > line 72. Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:60: ~EncryptionScheme() {} On 2015/12/10 18:36:01, ddorwin wrote: > Do not inline. Done. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:71: private: On 2015/12/10 18:36:01, ddorwin wrote: > Same question about const members. Same answer about copy and assign. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:72: bool is_encrypted_; On 2015/12/10 20:03:38, xhwang wrote: > It's a bit odd that you have an EncryptionScheme with |is_encrypted| being false > for an unencrypted stream. In {Audio|Video}DecoderConfig, can we actually just > hold a scoped_ptr<EncryptionScheme> so that when it's null the config is NOT > encrypted? That way, we can drop |is_encrypted| here in this class. Also, we can > disallow copy and assign. Another benefit is that in cases where the config is > not encrypted, you can simply use a nullptr for EncryptionScheme. I tried this and it doesn't work (at least not without a bunch of extra refactoring) because {Audio|Video}DecoderConfig have automatically-provided copy operators, and these are not provided for scopers. https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.h:72: bool is_encrypted_; On 2015/12/10 18:36:01, ddorwin wrote: > Do we need this? We could rename kCipherModeUnknown to kCipherModeNone or > kCipherModeClear and just use that? (This might also help minimize the number of > constructors.) Done. https://codereview.chromium.org/1490613005/diff/1/media/base/video_decoder_co... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/1/media/base/video_decoder_co... media/base/video_decoder_config.h:98: bool is_encrypted() const { return encryption_scheme_.is_encrypted(); } On 2015/12/10 18:36:01, ddorwin wrote: > ditto * 2 Done. https://codereview.chromium.org/1490613005/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:367: return AVCodecContextToAudioDecoderConfig(stream->codec, is_encrypted, On 2015/12/10 18:36:01, ddorwin wrote: > We could create the correct object here. Would it be beneficial to push EncryptionScheme further into ffmpeg? I really don't know. My change has the benefit of not making more of ffmpeg aware of EncryptionScheme than is necessary, and it can continue to treat it as a kind of augmented boolean. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_adts.cc:229: EncryptionScheme scheme(false); On 2015/12/10 18:36:01, ddorwin wrote: > Why do we need a local variable? This is a placeholder for a change in the next CL, where the scheme variable is computed in a more complicated way. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.cc:234: RCHECK(UpdateVideoDecoderConfig(sps, scheme)); On 2015/12/10 18:36:01, ddorwin wrote: > ditto The second CL shows why it's like this. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.cc:256: #ifdef ENABLE_HLS_SAMPLE_AES On 2015/12/10 00:12:19, yucliu1 wrote: > Remove this block in this CL? Yes, this was an oversight when I split my work into two CLs. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp2t/es_parse... media/formats/mp2t/es_parser_h264.h:69: const EncryptionScheme& scheme); On 2015/12/10 18:36:02, ddorwin wrote: > Fwd declare? Done. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:274: EncryptionScheme(is_audio_track_encrypted_), base::TimeDelta(), 0); On 2015/12/10 18:36:02, ddorwin wrote: > Perhaps we should have a member documenting the encryption scheme rather than > just |is_audio_track_encrypted_|. I figured we could do that when we added support for CENCv3 to mp4 parser. Right now it takes the default of AES-CTR. https://codereview.chromium.org/1490613005/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:315: std::vector<uint8_t>(), EncryptionScheme(is_video_track_encrypted_)); On 2015/12/10 18:36:02, ddorwin wrote: > ditto See above comment. (Default is appropriately AES-CTR with no pattern). https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... media/formats/webm/webm_audio_client.cc:81: EncryptionScheme(is_encrypted), On 2015/12/10 18:36:02, ddorwin wrote: > Even here, it might make sense to explicitly say AES-CTR no pattern because that > is now WebM is specified. I wasn't aware that WebM was like that. But even so, wouldn't it make sense to do that later when/if WebM adds other options? For now, the default has been set up to be backwards compatible. https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_vid... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_vid... media/formats/webm/webm_video_client.cc:94: EncryptionScheme(is_encrypted)); On 2015/12/10 18:36:02, ddorwin wrote: > ditto See my earlier response. Will do it if you think appropriate however.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
I just reviewed chromecast/ and media/base/encryption_scheme.h https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... File chromecast/common/media/cma_param_traits.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... chromecast/common/media/cma_param_traits.cc:33: typedef media::EncryptionScheme::PatternSpec param_type; nit: 'using' https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... File chromecast/common/media/cma_param_traits.h (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... chromecast/common/media/cma_param_traits.h:36: typedef media::EncryptionScheme param_type; nit: 'using' preferred to typedef in style guide now. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:266: video_config.encryption_scheme = EncryptionScheme(false); Good example of why this constructor isn't clear. If the default is unencrypted, we could just delete this line? Or, we should use a line that is readably explicit about it being unencrypted. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:180: switch(mode) { nit: space after switch. Maybe worth running clang format. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:195: switch(mode) { ditto https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:220: // static nit, unnecessary https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:228: // static ditto https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... File chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:16: class PatternSpecMarshaller { I don't understand the point of this class? Why not just move the code down into the EncryptionSchemeMarshaller functions? https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc:70: output_config_.encryption_scheme = EncryptionScheme(false); another example of 'false' ... not instantly readable. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:84: ::media::EmptyExtraData(), ::media::EncryptionScheme(false)); another example. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:90: ::media::EmptyExtraData(), ::media::EncryptionScheme(false))); ditto https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... File chromecast/media/cma/test/mock_frame_provider.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... chromecast/media/cma/test/mock_frame_provider.cc:85: ::media::EncryptionScheme(false)); ditto https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... chromecast/media/cma/test/mock_frame_provider.cc:93: ::media::EncryptionScheme(false)); ditto https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:119: explicit EncryptionScheme(bool is_encrypted); same comment as ::media::EncryptionScheme constructors https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:142: : mode(is_encrypted ? kCipherModeAesCtr : kCipherModeNone), Don't like 'true' being implicitly AesCtr. Can we just lose the bool constructor? https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:189: encryption_scheme(false) { default initialisation of encryption_scheme should be fine here, remove line? https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:197: // sites are only interested in whether encryption has been applied or not. let's just delete this comment, doesn't add any value for me. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:238: // See comment above for AudioConfig::is_encrypted(). delete https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... media/base/encryption_scheme.h:55: explicit EncryptionScheme(bool is_encrypted); As mentioned in downstream CLs, I don't like callsites that look like this: EncryptionScheme(false); as it's not immediately clear what 'false' refers to. I'm not clear why we need this constructor anyway if 'none' means unencrypted?
https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... File chromecast/common/media/cma_param_traits.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... chromecast/common/media/cma_param_traits.cc:33: typedef media::EncryptionScheme::PatternSpec param_type; On 2016/01/13 03:29:40, halliwell wrote: > nit: 'using' See reply to comment on the .h file. https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... File chromecast/common/media/cma_param_traits.h (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... chromecast/common/media/cma_param_traits.h:36: typedef media::EncryptionScheme param_type; On 2016/01/13 03:29:40, halliwell wrote: > nit: 'using' preferred to typedef in style guide now. There are hundreds of existing instances of this pattern and as far as I know they all use typedef, so I think I will leave it that way. (You did say "nit:"!) https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:266: video_config.encryption_scheme = EncryptionScheme(false); On 2016/01/13 03:29:41, halliwell wrote: > Good example of why this constructor isn't clear. If the default is > unencrypted, we could just delete this line? Or, we should use a line that is > readably explicit about it being unencrypted. Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:180: switch(mode) { On 2016/01/13 03:29:41, halliwell wrote: > nit: space after switch. Maybe worth running clang format. Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:195: switch(mode) { On 2016/01/13 03:29:41, halliwell wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:220: // static On 2016/01/13 03:29:41, halliwell wrote: > nit, unnecessary Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:228: // static On 2016/01/13 03:29:41, halliwell wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... File chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/01/13 03:29:41, halliwell wrote: > nit: 2014 Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:16: class PatternSpecMarshaller { On 2016/01/13 03:29:41, halliwell wrote: > I don't understand the point of this class? Why not just move the code down > into the EncryptionSchemeMarshaller functions? For the same reason that PatternSpec is a nested struct rather than just having its members included directly in EncryptionScheme. Just a more logical structure in my opinion. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc:70: output_config_.encryption_scheme = EncryptionScheme(false); On 2016/01/13 03:29:41, halliwell wrote: > another example of 'false' ... not instantly readable. Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:84: ::media::EmptyExtraData(), ::media::EncryptionScheme(false)); On 2016/01/13 03:29:41, halliwell wrote: > another example. Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:90: ::media::EmptyExtraData(), ::media::EncryptionScheme(false))); On 2016/01/13 03:29:41, halliwell wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... File chromecast/media/cma/test/mock_frame_provider.cc (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... chromecast/media/cma/test/mock_frame_provider.cc:85: ::media::EncryptionScheme(false)); On 2016/01/13 03:29:41, halliwell wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... chromecast/media/cma/test/mock_frame_provider.cc:93: ::media::EncryptionScheme(false)); On 2016/01/13 03:29:41, halliwell wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:119: explicit EncryptionScheme(bool is_encrypted); On 2016/01/13 03:29:41, halliwell wrote: > same comment as ::media::EncryptionScheme constructors Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:142: : mode(is_encrypted ? kCipherModeAesCtr : kCipherModeNone), On 2016/01/13 03:29:41, halliwell wrote: > Don't like 'true' being implicitly AesCtr. Can we just lose the bool > constructor? Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:189: encryption_scheme(false) { On 2016/01/13 03:29:41, halliwell wrote: > default initialisation of encryption_scheme should be fine here, remove line? Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:197: // sites are only interested in whether encryption has been applied or not. On 2016/01/13 03:29:41, halliwell wrote: > let's just delete this comment, doesn't add any value for me. Done. https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... chromecast/public/media/decoder_config.h:238: // See comment above for AudioConfig::is_encrypted(). On 2016/01/13 03:29:41, halliwell wrote: > delete Done. https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... media/base/encryption_scheme.h:55: explicit EncryptionScheme(bool is_encrypted); On 2016/01/13 03:29:42, halliwell wrote: > As mentioned in downstream CLs, I don't like callsites that look like this: > > EncryptionScheme(false); > > as it's not immediately clear what 'false' refers to. I'm not clear why we need > this constructor anyway if 'none' means unencrypted? Following our discussion, I added a static function "unencrypted" that returns an EncryptionScheme object with mode of none. This covers the vast majority of cases sprinkled around chromium.
On 2016/02/09 22:58:54, dougsteed wrote: > https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... > File chromecast/common/media/cma_param_traits.cc (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... > chromecast/common/media/cma_param_traits.cc:33: typedef > media::EncryptionScheme::PatternSpec param_type; > On 2016/01/13 03:29:40, halliwell wrote: > > nit: 'using' > > See reply to comment on the .h file. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... > File chromecast/common/media/cma_param_traits.h (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/common/medi... > chromecast/common/media/cma_param_traits.h:36: typedef media::EncryptionScheme > param_type; > On 2016/01/13 03:29:40, halliwell wrote: > > nit: 'using' preferred to typedef in style guide now. > > There are hundreds of existing instances of this pattern and as far as I know > they all use typedef, so I think I will leave it that way. (You did say "nit:"!) > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc > (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:266: > video_config.encryption_scheme = EncryptionScheme(false); > On 2016/01/13 03:29:41, halliwell wrote: > > Good example of why this constructor isn't clear. If the default is > > unencrypted, we could just delete this line? Or, we should use a line that is > > readably explicit about it being unencrypted. > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/base/decoder_config_adapter.cc:180: switch(mode) { > On 2016/01/13 03:29:41, halliwell wrote: > > nit: space after switch. Maybe worth running clang format. > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/base/decoder_config_adapter.cc:195: switch(mode) { > On 2016/01/13 03:29:41, halliwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/base/decoder_config_adapter.cc:220: // static > On 2016/01/13 03:29:41, halliwell wrote: > > nit, unnecessary > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/base/decoder_config_adapter.cc:228: // static > On 2016/01/13 03:29:41, halliwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... > File chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... > chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:1: // > Copyright 2014 The Chromium Authors. All rights reserved. > On 2016/01/13 03:29:41, halliwell wrote: > > nit: 2014 > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/i... > chromecast/media/cma/ipc_streamer/encryption_scheme_marshaller.cc:16: class > PatternSpecMarshaller { > On 2016/01/13 03:29:41, halliwell wrote: > > I don't understand the point of this class? Why not just move the code down > > into the EncryptionSchemeMarshaller functions? > > For the same reason that PatternSpec is a nested struct rather than just having > its members included directly in EncryptionScheme. Just a more logical structure > in my opinion. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... > File chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... > chromecast/media/cma/pipeline/audio_decoder_software_wrapper.cc:70: > output_config_.encryption_scheme = EncryptionScheme(false); > On 2016/01/13 03:29:41, halliwell wrote: > > another example of 'false' ... not instantly readable. > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... > File chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc > (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... > chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:84: > ::media::EmptyExtraData(), ::media::EncryptionScheme(false)); > On 2016/01/13 03:29:41, halliwell wrote: > > another example. > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/p... > chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc:90: > ::media::EmptyExtraData(), ::media::EncryptionScheme(false))); > On 2016/01/13 03:29:41, halliwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... > File chromecast/media/cma/test/mock_frame_provider.cc (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... > chromecast/media/cma/test/mock_frame_provider.cc:85: > ::media::EncryptionScheme(false)); > On 2016/01/13 03:29:41, halliwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/media/cma/t... > chromecast/media/cma/test/mock_frame_provider.cc:93: > ::media::EncryptionScheme(false)); > On 2016/01/13 03:29:41, halliwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > File chromecast/public/media/decoder_config.h (right): > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:119: explicit EncryptionScheme(bool > is_encrypted); > On 2016/01/13 03:29:41, halliwell wrote: > > same comment as ::media::EncryptionScheme constructors > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:142: : mode(is_encrypted ? > kCipherModeAesCtr : kCipherModeNone), > On 2016/01/13 03:29:41, halliwell wrote: > > Don't like 'true' being implicitly AesCtr. Can we just lose the bool > > constructor? > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:189: encryption_scheme(false) { > On 2016/01/13 03:29:41, halliwell wrote: > > default initialisation of encryption_scheme should be fine here, remove line? > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:197: // sites are only interested in > whether encryption has been applied or not. > On 2016/01/13 03:29:41, halliwell wrote: > > let's just delete this comment, doesn't add any value for me. > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:238: // See comment above for > AudioConfig::is_encrypted(). > On 2016/01/13 03:29:41, halliwell wrote: > > delete > > Done. > > https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... > File media/base/encryption_scheme.h (right): > > https://codereview.chromium.org/1490613005/diff/120001/media/base/encryption_... > media/base/encryption_scheme.h:55: explicit EncryptionScheme(bool is_encrypted); > On 2016/01/13 03:29:42, halliwell wrote: > > As mentioned in downstream CLs, I don't like callsites that look like this: > > > > EncryptionScheme(false); > > > > as it's not immediately clear what 'false' refers to. I'm not clear why we > need > > this constructor anyway if 'none' means unencrypted? > > Following our discussion, I added a static function "unencrypted" that returns > an EncryptionScheme object with mode of none. This covers the vast majority of > cases sprinkled around chromium. chromecast/ lgtm
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as allowed by the later versions of CENC (ISO's Common Encryption standard). BUG=568326 ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 ==========
Thanks for addressing my feedback in PS1. I think there a few more opportunities for improvement, especially related to being more explicit at call sites. Three replies in PS1. The rest in PS13. I did not review all the unittest changes since those may change based on these comments. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... File chromecast/media/audio/cast_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); On 2015/12/14 21:19:01, dougsteed wrote: > On 2015/12/10 18:36:00, ddorwin wrote: > > Do we not have any similar tests for non-Chromecast code? Or perhaps those are > > hidden by is_encrypted() calls. Please find and update any such tests. > > Pretty sure I updated all the tests, as it would not compile otherwise. This comment doesn't actually apply here. I moved it to audio_decoder_config.h. https://codereview.chromium.org/1490613005/diff/1/chromecast/media/audio/cast... chromecast/media/audio/cast_audio_output_stream_unittest.cc:384: EXPECT_FALSE(audio_config.encryption_scheme.is_encrypted); On 2015/12/14 21:19:01, dougsteed wrote: > On 2015/12/10 18:36:00, ddorwin wrote: > > Are there other members we can/should test? > > Do you mean members of audio_config, or of audio_config.encryption_scheme ? In > any case, I am not familiar with the nuances of these tests, so all I am doing > is trying to preserve the semantics of the existing test. I was referring to EncryptionScheme. The values should be whatever the defaults are (no encryption) or the values for CENC v1. https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... media/formats/webm/webm_audio_client.cc:81: EncryptionScheme(is_encrypted), On 2015/12/14 21:19:02, dougsteed wrote: > On 2015/12/10 18:36:02, ddorwin wrote: > > Even here, it might make sense to explicitly say AES-CTR no pattern because > that > > is now WebM is specified. > > I wasn't aware that WebM was like that. But even so, wouldn't it make sense to > do that later when/if WebM adds other options? For now, the default has been set > up to be backwards compatible. I'd rather be explicit than rely on a default, especially since it won't be clear from reading the actual code that deals with the containers. By specifying it now, we are being clear about what the spec says now. I don't think whether there are options is relevant to this. https://codereview.chromium.org/1490613005/diff/240001/chromecast/media/audio... File chromecast/media/audio/cast_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/240001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream_unittest.cc:388: EXPECT_EQ(audio_config.encryption_scheme.mode, Add checks for is_encrypted() and pattern(). https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... chromecast/public/media/decoder_config.h:105: // V3 of the CENC standard will add pattern encryption, through two new s/will add/adds/ https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... chromecast/public/media/decoder_config.h:119: ~PatternSpec() {} Do not inline. https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... File media/base/audio_decoder_config.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... media/base/audio_decoder_config.cc:59: encryption_scheme_(false), default constructor or EncryptionScheme::unencrypted()? https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... media/base/audio_decoder_config.h:113: bool is_encrypted() const; My concern in PS1 of cast_audio_output_stream_unittest.cc was really about this change (and the corresponding one for video). We may have tests calling is_encrypted() on the config, and these will continue to compile without changes. However, we really should be checking all properties of the scheme. We should try to identify and update those tests. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.cc:29: EncryptionScheme::EncryptionScheme(bool is_encrypted) This one does not initialize pattern_() like the others. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.cc:30: : mode_(is_encrypted ? kCipherModeAesCtr : kCipherModeNone) {} The caller can do this. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:19: kCipherModeNone, If we make this kCipherModeUnencrypted, is that clearer and does that simplify things, such as the constructor that takes a bool? https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:35: // If either or both of encrypt_blocks or skip_blocks is 0, pattern On 2015/12/14 21:19:01, dougsteed wrote: > On 2015/12/10 18:36:01, ddorwin wrote: > > Where does this apply? Is this a rule for this class? Instead can we just > > disallow passing 0 to the non-default constructor? > > > > Better yet, can we use static CreateNoPattern() and CreatePattern(e, s) > > functions? (See discussion below.) > > I am trying to model the concept of CENCv3, where pattern encryption is in > effect if both values are non-zero, and not in effect otherwise. So my > interpretation is that either or both being zero is permitted by the class, and > has the meaning of rendering pattern encryption not in effect. There is also a > similar class in chromecast, and it is much more convenient to translate the > objects back and forward by just copying the two values. Where do these values come from? Media? App? Internally based on something else? Is there any value in tracking a non-zero value when the other is 0? Is the default constructor necessary/useful? If not, perhaps we should _always_ track pass these values. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:57: EncryptionScheme(); // Same as CreateUnencrypted(). https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:58: explicit EncryptionScheme(bool is_encrypted); Does it make sense to specify `true` without passing a mode? Can we replace uses of this with the next one? https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:59: explicit EncryptionScheme(CipherMode mode); Do we need this one? Should we be explicit about requiring the pattern to be specified? If tests are the main reason for some of these, we can create a CreateForTests() wrapper that the presubmit check will only allow to be called from test files and keep the production code more explicit. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:62: static EncryptionScheme unencrypted(); CreateUnencrypted? It at least needs to use CamelCase. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:64: void Initialize(CipherMode mode, const PatternSpec& pattern); Do we need this? https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:67: bool is_encrypted() const; This might not be a simple accessor anymore. https://codereview.chromium.org/1490613005/diff/240001/media/base/fake_demuxe... File media/base/fake_demuxer_stream.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/fake_demuxe... media/base/fake_demuxer_stream.cc:154: EmptyExtraData(), EncryptionScheme(is_encrypted_)); For example, in this case, I'd rather store the encryption scheme (or have a function that converts the bool) than support an extra constructor, especially since it hides an important detail that some callers should care about. https://codereview.chromium.org/1490613005/diff/240001/media/base/video_decod... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/video_decod... media/base/video_decoder_config.h:102: bool is_encrypted() const; ditto https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:375: extra_data, EncryptionScheme(is_encrypted), seek_preroll, FFmpeg only supports CTR without a pattern, so we should not hide that information. Perhaps this should be pushed up the the call stack, but for now we can pass the correct scheme. https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:534: EncryptionScheme(is_encrypted)); ditto https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_h264.h:73: bool UpdateVideoDecoderConfig(const H264SPS* sps, This should probably be passed by const ref. (Should be a different CL.) https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... File media/formats/mp2t/es_parser_mpeg1audio.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:5: #include "media/formats/mp2t/es_parser_mpeg1audio.h" Why aren't all of the other codecs affected by this change? https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:175: std::vector<uint8_t>(), EncryptionScheme::unencrypted()); This codec cannot be encrypted? Is that guaranteed somehow? https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:307: EncryptionScheme(is_audio_track_encrypted_), base::TimeDelta(), 0); For now, it's always CTR no pattern. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:348: std::vector<uint8_t>(), EncryptionScheme(is_video_track_encrypted_)); ditto https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_audio_client.cc:84: EncryptionScheme(is_encrypted), WebM is always CTR no pattern. https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:7: #include "media/base/video_decoder_config.h" #include "media/base/encryption_scheme.h" https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:96: EncryptionScheme(is_encrypted)); ditto
Sorry for the delay. It seems ddorwin has some comments that need to be addressed first. I'll take a quick look after those comments are resolved. Thanks!
David, Xiaohan: PTAL https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/base/encryption_schem... media/base/encryption_scheme.cc:7: #include "base/logging.h" On 2015/12/10 18:36:01, ddorwin wrote: > I don't think this is currently used, but it may be after addressing one of my > comments. Done. https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/1/media/formats/webm/webm_aud... media/formats/webm/webm_audio_client.cc:81: EncryptionScheme(is_encrypted), On 2016/03/01 02:17:41, ddorwin wrote: > On 2015/12/14 21:19:02, dougsteed wrote: > > On 2015/12/10 18:36:02, ddorwin wrote: > > > Even here, it might make sense to explicitly say AES-CTR no pattern because > > that > > > is now WebM is specified. > > > > I wasn't aware that WebM was like that. But even so, wouldn't it make sense to > > do that later when/if WebM adds other options? For now, the default has been > set > > up to be backwards compatible. > > I'd rather be explicit than rely on a default, especially since it won't be > clear from reading the actual code that deals with the containers. > > By specifying it now, we are being clear about what the spec says now. I don't > think whether there are options is relevant to this. Done. https://codereview.chromium.org/1490613005/diff/240001/chromecast/media/audio... File chromecast/media/audio/cast_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1490613005/diff/240001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream_unittest.cc:388: EXPECT_EQ(audio_config.encryption_scheme.mode, On 2016/03/01 02:17:41, ddorwin wrote: > Add checks for is_encrypted() and pattern(). I added inline is_encrypted() check to the chromecast EncryptionScheme struct, which is being used here. It is essentially equivalent to the check for kCipherModeNone. And that is the only one that needs to be checked. Pattern is undefined if !is_encrypted(). https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... chromecast/public/media/decoder_config.h:105: // V3 of the CENC standard will add pattern encryption, through two new On 2016/03/01 02:17:41, ddorwin wrote: > s/will add/adds/ Done. https://codereview.chromium.org/1490613005/diff/240001/chromecast/public/medi... chromecast/public/media/decoder_config.h:119: ~PatternSpec() {} On 2016/03/01 02:17:41, ddorwin wrote: > Do not inline. Done. https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... File media/base/audio_decoder_config.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... media/base/audio_decoder_config.cc:59: encryption_scheme_(false), On 2016/03/01 02:17:41, ddorwin wrote: > default constructor or EncryptionScheme::unencrypted()? Done. https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... media/base/audio_decoder_config.h:113: bool is_encrypted() const; On 2016/03/01 02:17:41, ddorwin wrote: > My concern in PS1 of cast_audio_output_stream_unittest.cc was really about this > change (and the corresponding one for video). > > We may have tests calling is_encrypted() on the config, and these will continue > to compile without changes. However, we really should be checking all properties > of the scheme. We should try to identify and update those tests. It seems to me that generally in the code (there are 67 places in 30 files where is_encrypted() is called) the caller is interested in just the boolean, not the details. For example, in many contexts, both in the actual code and in unittests, encryption is not allowed or supported, so being able to test that is_encrypted() is false seems very appropriate. In those contexts there is nothing to be gained in checking the mode or pattern. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.cc:29: EncryptionScheme::EncryptionScheme(bool is_encrypted) On 2016/03/01 02:17:41, ddorwin wrote: > This one does not initialize pattern_() like the others. Done. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:19: kCipherModeNone, On 2016/03/01 02:17:41, ddorwin wrote: > If we make this kCipherModeUnencrypted, is that clearer and does that simplify > things, such as the constructor that takes a bool? Done. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:35: // If either or both of encrypt_blocks or skip_blocks is 0, pattern On 2016/03/01 02:17:41, ddorwin wrote: > On 2015/12/14 21:19:01, dougsteed wrote: > > On 2015/12/10 18:36:01, ddorwin wrote: > > > Where does this apply? Is this a rule for this class? Instead can we just > > > disallow passing 0 to the non-default constructor? > > > > > > Better yet, can we use static CreateNoPattern() and CreatePattern(e, s) > > > functions? (See discussion below.) > > > > I am trying to model the concept of CENCv3, where pattern encryption is in > > effect if both values are non-zero, and not in effect otherwise. So my > > interpretation is that either or both being zero is permitted by the class, > and > > has the meaning of rendering pattern encryption not in effect. There is also a > > similar class in chromecast, and it is much more convenient to translate the > > objects back and forward by just copying the two values. > > Where do these values come from? Media? App? Internally based on something else? > > Is there any value in tracking a non-zero value when the other is 0? > > Is the default constructor necessary/useful? If not, perhaps we should _always_ > track pass these values. In general they will come from the media, but only for certain encryption schemes. Having the default, which disables pattern encryption, is very useful right now, as that is widely applicable to most formats. While there is no specific value I can see right now in tracking a non-zero value when the other is zero, that seems to best model the way it is defined in CENC v3 draft, and I don't see any harm. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:58: explicit EncryptionScheme(bool is_encrypted); On 2016/03/01 02:17:41, ddorwin wrote: > Does it make sense to specify `true` without passing a mode? Can we replace uses > of this with the next one? Done. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:59: explicit EncryptionScheme(CipherMode mode); On 2016/03/01 02:17:41, ddorwin wrote: > Do we need this one? Should we be explicit about requiring the pattern to be > specified? > > If tests are the main reason for some of these, we can create a CreateForTests() > wrapper that the presubmit check will only allow to be called from test files > and keep the production code more explicit. As mentioned above I think the default pattern (i.e. pattern disabled) is the overwhelming use case right now, and in most cases pattern is not even defined for the mode in use, so I don't see any need to keep saying that explicitly. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:62: static EncryptionScheme unencrypted(); On 2016/03/01 02:17:41, ddorwin wrote: > CreateUnencrypted? It at least needs to use CamelCase. Changed it to Unencrypted. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:64: void Initialize(CipherMode mode, const PatternSpec& pattern); On 2016/03/01 02:17:41, ddorwin wrote: > Do we need this? Appears not. https://codereview.chromium.org/1490613005/diff/240001/media/base/encryption_... media/base/encryption_scheme.h:67: bool is_encrypted() const; On 2016/03/01 02:17:41, ddorwin wrote: > This might not be a simple accessor anymore. Not sure what you mean by that. It is very simple, just tests whether mode is kCipherModeUnencrypted. I think it justifies being considered simple accessor. https://codereview.chromium.org/1490613005/diff/240001/media/base/fake_demuxe... File media/base/fake_demuxer_stream.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/fake_demuxe... media/base/fake_demuxer_stream.cc:154: EmptyExtraData(), EncryptionScheme(is_encrypted_)); On 2016/03/01 02:17:41, ddorwin wrote: > For example, in this case, I'd rather store the encryption scheme (or have a > function that converts the bool) than support an extra constructor, especially > since it hides an important detail that some callers should care about. Done. https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:375: extra_data, EncryptionScheme(is_encrypted), seek_preroll, On 2016/03/01 02:17:41, ddorwin wrote: > FFmpeg only supports CTR without a pattern, so we should not hide that > information. Perhaps this should be pushed up the the call stack, but for now we > can pass the correct scheme. Done. https://codereview.chromium.org/1490613005/diff/240001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:534: EncryptionScheme(is_encrypted)); On 2016/03/01 02:17:41, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... File media/formats/mp2t/es_parser_mpeg1audio.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:5: #include "media/formats/mp2t/es_parser_mpeg1audio.h" On 2016/03/01 02:17:42, ddorwin wrote: > Why aren't all of the other codecs affected by this change? For encrypted HLS streams, which is the initial focus, only a few codecs are involved. The change for other codecs is just to achieve backwards compatibility. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:175: std::vector<uint8_t>(), EncryptionScheme::unencrypted()); On 2016/03/01 02:17:42, ddorwin wrote: > This codec cannot be encrypted? Is that guaranteed somehow? My understanding is that it is not supported for HLS, and thus can not be encrypted. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:307: EncryptionScheme(is_audio_track_encrypted_), base::TimeDelta(), 0); On 2016/03/01 02:17:42, ddorwin wrote: > For now, it's always CTR no pattern. Done. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:348: std::vector<uint8_t>(), EncryptionScheme(is_video_track_encrypted_)); On 2016/03/01 02:17:42, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_audio_client.cc:84: EncryptionScheme(is_encrypted), On 2016/03/01 02:17:42, ddorwin wrote: > WebM is always CTR no pattern. Done. https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:7: #include "media/base/video_decoder_config.h" On 2016/03/01 02:17:42, ddorwin wrote: > #include "media/base/encryption_scheme.h" Done. https://codereview.chromium.org/1490613005/diff/240001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:96: EncryptionScheme(is_encrypted)); On 2016/03/01 02:17:42, ddorwin wrote: > ditto Done.
https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/240001/media/base/audio_decod... media/base/audio_decoder_config.h:113: bool is_encrypted() const; On 2016/03/02 18:07:52, dougsteed wrote: > On 2016/03/01 02:17:41, ddorwin wrote: > > My concern in PS1 of cast_audio_output_stream_unittest.cc was really about > this > > change (and the corresponding one for video). > > > > We may have tests calling is_encrypted() on the config, and these will > continue > > to compile without changes. However, we really should be checking all > properties > > of the scheme. We should try to identify and update those tests. > > It seems to me that generally in the code (there are 67 places in 30 files where > is_encrypted() is called) the caller is interested in just the boolean, not the > details. For example, in many contexts, both in the actual code and in > unittests, encryption is not allowed or supported, so being able to test that > is_encrypted() is false seems very appropriate. In those contexts there is > nothing to be gained in checking the mode or pattern. Okay. I guess we should look for places where we expect it to be true in tests and then check the other members in those cases. https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... File media/formats/mp2t/es_parser_mpeg1audio.cc (right): https://codereview.chromium.org/1490613005/diff/240001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:5: #include "media/formats/mp2t/es_parser_mpeg1audio.h" On 2016/03/02 18:07:53, dougsteed wrote: > On 2016/03/01 02:17:42, ddorwin wrote: > > Why aren't all of the other codecs affected by this change? > > For encrypted HLS streams, which is the initial focus, only a few codecs are > involved. The change for other codecs is just to achieve backwards > compatibility. Ack. I thought there were more parsers, but this is all of them. https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:105: // V3 of the CENC standard adds pattern encryption, through two new nit: CENC 3rd edition adds... https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:146: inline EncryptionScheme::PatternSpec::~PatternSpec() {} Hmm. I didn't realize there was so much inlined in this file. I'm not sure this actually changes anything. This is consistent with the file, but in general, see https://www.chromium.org/developers/coding-style#TOC-Inline-functions and the linked C++ advice. https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:215: return encryption_scheme.mode != EncryptionScheme::kCipherModeUnencrypted; Just call is_encrypted()? https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:254: return encryption_scheme.mode != EncryptionScheme::kCipherModeUnencrypted; ditto https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... media/base/audio_decoder_config.h:83: bool is_encrypted() const; From PS13: I guess we should look for places where we expect it to be true in tests and then check the other members in those cases. https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... media/base/encryption_scheme.h:57: EncryptionScheme(); Clarify that this makes a "no encryption" object. https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... media/base/encryption_scheme.h:61: static EncryptionScheme Unencrypted(); This doesn't seem like a style-compliant name. It's not an action and doesn't follow the common factory Create... naming. https://codereview.chromium.org/1490613005/diff/260001/media/base/video_decod... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/video_decod... media/base/video_decoder_config.h:104: bool is_encrypted() const; ditto on finding places where we expect true in the tests and checking the other values. https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:376: EncryptionScheme encryption_scheme( It's weird that the audio path has an extra function call. I'll look into that. For now, though, it seems better for the code to be consistent (creating a scheme rather than bool), and we should put this information as close to where we processed the media data as possible. Thus, I suggest passing in a const EncryptionScheme&. Going further, we could abstract this to GetEncryptionScheme(const AVStream* stream) so we don't even need to duplicate the metadata processing. https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... media/formats/webm/webm_audio_client.cc:9: #include "media/base/encryption_scheme.h" This is no longer necessary since we're just passing through. https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:7: #include "media/base/encryption_scheme.h" This is no longer necessary since we're just passing through.
https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:146: inline EncryptionScheme::PatternSpec::~PatternSpec() {} On 2016/03/02 23:24:05, ddorwin wrote: > Hmm. I didn't realize there was so much inlined in this file. I'm not sure this > actually changes anything. > > This is consistent with the file, but in general, see > https://www.chromium.org/developers/coding-style#TOC-Inline-functions and the > linked C++ advice. This file (actually, this directory) has to be inlined, no cc files allowed for now.
https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:105: // V3 of the CENC standard adds pattern encryption, through two new On 2016/03/02 23:24:05, ddorwin wrote: > nit: CENC 3rd edition adds... Done. https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:215: return encryption_scheme.mode != EncryptionScheme::kCipherModeUnencrypted; On 2016/03/02 23:24:05, ddorwin wrote: > Just call is_encrypted()? Done. https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:254: return encryption_scheme.mode != EncryptionScheme::kCipherModeUnencrypted; On 2016/03/02 23:24:05, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... media/base/audio_decoder_config.h:83: bool is_encrypted() const; On 2016/03/02 23:24:05, ddorwin wrote: > From PS13: I guess we should look for places where we expect it to be true in > tests and then check the other members in those cases. OK, I'll scan through all instances with that in mind .... ... looked through all occurrences of is_encrypted() applied to {Audio|Video}DecoderConfig. Most of them are essentially asserting that it is not encrypted. I changed media/filters/source_buffer_stream.cc to check the whole encryption_scheme rather than just is_encrypted. A few of the call sites are using is_encrypted() to evaluate a higher-level notion of "IsEncrypted". I didn't follow all those paths. The ones that I would like your input on are in: PpapiDecryptor, DecryptingVideoDecoder, VideoDecodeAccelerator. https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... media/base/encryption_scheme.h:57: EncryptionScheme(); On 2016/03/02 23:24:05, ddorwin wrote: > Clarify that this makes a "no encryption" object. Done. https://codereview.chromium.org/1490613005/diff/260001/media/base/encryption_... media/base/encryption_scheme.h:61: static EncryptionScheme Unencrypted(); On 2016/03/02 23:24:05, ddorwin wrote: > This doesn't seem like a style-compliant name. It's not an action and doesn't > follow the common factory Create... naming. I want this to be named as just returning a suitable "no encryption" object. I don't want to emphasize that something is actually being created by making that so prominent in the name. This might as well be a static instance, but for something so lightweight, I didn't think it was worth setting up the machinery of singletons. https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:376: EncryptionScheme encryption_scheme( On 2016/03/02 23:24:05, ddorwin wrote: > It's weird that the audio path has an extra function call. I'll look into that. > For now, though, it seems better for the code to be consistent (creating a > scheme rather than bool), and we should put this information as close to where > we processed the media data as possible. Thus, I suggest passing in a const > EncryptionScheme&. > > Going further, we could abstract this to GetEncryptionScheme(const AVStream* > stream) so we don't even need to duplicate the metadata processing. Done. The extra audio function AVCodecContextToAudioDecoderConfig is labelled MEDIA_EXPORT. OK to change the signature? (Although it only seems to be used in unit tests). https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... File media/formats/webm/webm_audio_client.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... media/formats/webm/webm_audio_client.cc:9: #include "media/base/encryption_scheme.h" On 2016/03/02 23:24:05, ddorwin wrote: > This is no longer necessary since we're just passing through. Done. https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... File media/formats/webm/webm_video_client.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/formats/webm/web... media/formats/webm/webm_video_client.cc:7: #include "media/base/encryption_scheme.h" On 2016/03/02 23:24:05, ddorwin wrote: > This is no longer necessary since we're just passing through. Done.
LG with a few comments. Thank you! xhwang and/or I will give another full pass over the non-chromecast/ files before giving an L G T M. https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/chromecast/public/medi... chromecast/public/media/decoder_config.h:146: inline EncryptionScheme::PatternSpec::~PatternSpec() {} On 2016/03/02 23:39:53, halliwell wrote: > On 2016/03/02 23:24:05, ddorwin wrote: > > Hmm. I didn't realize there was so much inlined in this file. I'm not sure > this > > actually changes anything. > > > > This is consistent with the file, but in general, see > > https://www.chromium.org/developers/coding-style#TOC-Inline-functions and the > > linked C++ advice. > > This file (actually, this directory) has to be inlined, no cc files allowed for > now. Acknowledged. https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/260001/media/base/audio_decod... media/base/audio_decoder_config.h:83: bool is_encrypted() const; On 2016/03/03 04:45:00, dougsteed wrote: > On 2016/03/02 23:24:05, ddorwin wrote: > > From PS13: I guess we should look for places where we expect it to be true in > > tests and then check the other members in those cases. > > OK, I'll scan through all instances with that in mind .... > ... looked through all occurrences of is_encrypted() applied to > {Audio|Video}DecoderConfig. > > Most of them are essentially asserting that it is not encrypted. I changed > media/filters/source_buffer_stream.cc to check the whole encryption_scheme > rather than just is_encrypted. > A few of the call sites are using is_encrypted() to evaluate a higher-level > notion of "IsEncrypted". I didn't follow all those paths. > The ones that I would like your input on are in: PpapiDecryptor, > DecryptingVideoDecoder, VideoDecodeAccelerator. Thanks. Those should three be fine since we only support CTR no pattern there. I was specifically referring to tests. If there were any tests that expected true, then we should check the mode and pattern (almost certainly CTR and none, respectively). https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/1490613005/diff/260001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:376: EncryptionScheme encryption_scheme( On 2016/03/03 04:45:00, dougsteed wrote: > On 2016/03/02 23:24:05, ddorwin wrote: > > It's weird that the audio path has an extra function call. I'll look into > that. > > For now, though, it seems better for the code to be consistent (creating a > > scheme rather than bool), and we should put this information as close to where > > we processed the media data as possible. Thus, I suggest passing in a const > > EncryptionScheme&. > > > > Going further, we could abstract this to GetEncryptionScheme(const AVStream* > > stream) so we don't even need to duplicate the metadata processing. > > Done. The extra audio function AVCodecContextToAudioDecoderConfig is labelled > MEDIA_EXPORT. > OK to change the signature? (Although it only seems to be used in unit tests). Yes. No interfaces are guaranteed not to change except things like PPAPI. I found that the audio wrapper was added in https://chromiumcodereview.appspot.com/10829470/diff/120001/media/ffmpeg/ffmp... to make it match the video case. I think we can eliminate the internal function. I'll look into it. https://codereview.chromium.org/1490613005/diff/280001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/280001/chromecast/public/medi... chromecast/public/media/decoder_config.h:105: // CENC 3d Edition adds pattern encryption, through two new 3rd https://codereview.chromium.org/1490613005/diff/280001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/280001/media/base/encryption_... media/base/encryption_scheme.h:17: // Algorithm and mode used for encryption. Allows an indication that no kCipherModeUnencrypted indicates that no... ? https://codereview.chromium.org/1490613005/diff/280001/media/base/encryption_... media/base/encryption_scheme.h:63: // Returns an instance of EncryptionScheme indicating no encryption. This is good, but I meant to add this to the default constructor.
https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/medi... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/medi... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:72: default_config.encryption_scheme = EncryptionScheme::unencrypted(); Should this be Unencrypted()? https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... File chromecast/public/media/decoder_config.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:97: struct EncryptionScheme { Is there any reason you need to duplication this in chromecast code? Also, why don't you keep it exactly the same as the one in media/base? https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:216: } nit: you can implement this in line 182 directly. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:255: } ditto https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... File media/base/encryption_scheme.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:20: kCipherModeUnencrypted, In new code we should use UPPER_CASE for enums: https://www.chromium.org/developers/coding-style#TOC-Naming https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:29: // The pattern applies only to the 'encrypted' part of the frame (as Does the patter apply to all "encrypted" blocks (as defined by the subsample entries) independently? I assume so, but it would be nicer to make this comment more clear on that. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:36: // If either or both of encrypt_blocks or skip_blocks is 0, pattern nit: "or both of" is redundant https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:38: class PatternSpec { "Spec" is not in the standard and doesn't add any value here. Can we just call it "Pattern"? You are calling the variable "pattern" already. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:64: static EncryptionScheme Unencrypted(); +ddorwin We use EncryptionScheme::Unencrypted() a lot. Can we have a helper function with a shorter name in media/base/media_util.cc to return an unencrypted scheme, something like Unencrypted() or UnencrytpedScheme(). We already have EmptyExtraData() there. Another choice is to just use EncryptionScheme(), which is okay but not as readable. We can also have a function AesCtrEncryptionScheme() to replace EncryptionScheme(EncryptionScheme::kCipherModeAesCtr). https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:68: bool is_encrypted() const; nit: you can inline this as well. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:73: CipherMode mode_; You can specify default mode here. Personally I feel that's more clear especially when there are multiple constructors. See "Non-Static Class Member Initializers" in https://chromium-cpp.appspot.com/ Same for members in PatternSpec. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... File media/base/video_decoder_config.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... media/base/video_decoder_config.cc:43: encryption_scheme_() {} This is not necessary. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... media/base/video_decoder_config.cc:118: } nit: this can be inlined in the header file https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... File media/formats/mp2t/es_parser_h264.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... media/formats/mp2t/es_parser_h264.cc:319: natural_size, std::vector<uint8_t>(), scheme); We can use EmptyExtraData() here for std::vector<uint8_t>() https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... File media/formats/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... media/formats/mp4/mp4_stream_parser.cc:354: std::vector<uint8_t>(), encryption_scheme); ditto about EmptyExtraData()
ddorwin and I also chat about the idea of holding a scoped_ptr<EncryptionScheme> in the configs such that we can just use nullptr to represent Unencrypted(). One trouble is that since those configs are copiable today, having a scoped_ptr will force us to define our own copy constructor. That seems a stopper at this point. Let us know if you have any strong opinions.
https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... File chromecast/public/media/decoder_config.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:97: struct EncryptionScheme { On 2016/03/03 22:33:00, xhwang wrote: > Is there any reason you need to duplication this in chromecast code? Yep, we ship just this directory (chromecast/public) as headers for Cast hardware vendors to include and build against, so they don't need a full Chromium checkout. I'm not sure why it's not the same as the other version though.
https://chromiumcodereview.appspot.com/1490613005/diff/260001/media/base/audi... File media/base/audio_decoder_config.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/260001/media/base/audi... media/base/audio_decoder_config.h:83: bool is_encrypted() const; On 2016/03/03 18:38:04, ddorwin wrote: > On 2016/03/03 04:45:00, dougsteed wrote: > > On 2016/03/02 23:24:05, ddorwin wrote: > > > From PS13: I guess we should look for places where we expect it to be true > in > > > tests and then check the other members in those cases. > > > > OK, I'll scan through all instances with that in mind .... > > ... looked through all occurrences of is_encrypted() applied to > > {Audio|Video}DecoderConfig. > > > > Most of them are essentially asserting that it is not encrypted. I changed > > media/filters/source_buffer_stream.cc to check the whole encryption_scheme > > rather than just is_encrypted. > > A few of the call sites are using is_encrypted() to evaluate a higher-level > > notion of "IsEncrypted". I didn't follow all those paths. > > The ones that I would like your input on are in: PpapiDecryptor, > > DecryptingVideoDecoder, VideoDecodeAccelerator. > > Thanks. > > Those should three be fine since we only support CTR no pattern there. I was > specifically referring to tests. If there were any tests that expected true, > then we should check the mode and pattern (almost certainly CTR and none, > respectively). In many cases the test uses a boolean to decide whether it should be configured to use encryption, and then uses an EXPECT with the boolean is_encrypted to verify whether that was done correctly. It doesn't seem to be testing much outside the test itself. I didn't see any where I thought it would be useful to verify the mode. Please take a quick peek yourself and see if you agree. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/medi... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/medi... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:72: default_config.encryption_scheme = EncryptionScheme::unencrypted(); On 2016/03/03 22:33:00, xhwang wrote: > Should this be Unencrypted()? I was preserving the original name in the chromecast/public/media files, because those names have already been exposed to OEMs. But after further consultation, prefer consistency with media. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... File chromecast/public/media/decoder_config.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:97: struct EncryptionScheme { On 2016/03/03 22:56:44, halliwell wrote: > On 2016/03/03 22:33:00, xhwang wrote: > > Is there any reason you need to duplication this in chromecast code? > > Yep, we ship just this directory (chromecast/public) as headers for Cast > hardware vendors to include and build against, so they don't need a full > Chromium checkout. > > I'm not sure why it's not the same as the other version though. Not sure what you mean by "not the same". As far as I know it is the same except for systemic changes: this file uses structs while media/base uses classes, etc. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:105: // CENC 3d Edition adds pattern encryption, through two new On 2016/03/03 18:38:04, ddorwin wrote: > 3rd aargh. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:216: } On 2016/03/03 22:33:00, xhwang wrote: > nit: you can implement this in line 182 directly. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:255: } On 2016/03/03 22:33:00, xhwang wrote: > ditto Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... File media/base/encryption_scheme.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:17: // Algorithm and mode used for encryption. Allows an indication that no On 2016/03/03 18:38:04, ddorwin wrote: > kCipherModeUnencrypted indicates that no... ? Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:20: kCipherModeUnencrypted, On 2016/03/03 22:33:00, xhwang wrote: > In new code we should use UPPER_CASE for enums: > > https://www.chromium.org/developers/coding-style#TOC-Naming Prefer consistency with the other enums in media. Also we have already exposed these names to our OEM partners, so would rather not change at this point. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:29: // The pattern applies only to the 'encrypted' part of the frame (as On 2016/03/03 22:33:00, xhwang wrote: > Does the patter apply to all "encrypted" blocks (as defined by the subsample > entries) independently? I assume so, but it would be nicer to make this comment > more clear on that. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:36: // If either or both of encrypt_blocks or skip_blocks is 0, pattern On 2016/03/03 22:33:00, xhwang wrote: > nit: "or both of" is redundant Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:38: class PatternSpec { On 2016/03/03 22:33:00, xhwang wrote: > "Spec" is not in the standard and doesn't add any value here. Can we just call > it "Pattern"? You are calling the variable "pattern" already. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:63: // Returns an instance of EncryptionScheme indicating no encryption. On 2016/03/03 18:38:04, ddorwin wrote: > This is good, but I meant to add this to the default constructor. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:64: static EncryptionScheme Unencrypted(); On 2016/03/03 22:33:00, xhwang wrote: > +ddorwin > > We use EncryptionScheme::Unencrypted() a lot. Can we have a helper function with > a shorter name in media/base/media_util.cc to return an unencrypted scheme, > something like Unencrypted() or UnencrytpedScheme(). > > We already have EmptyExtraData() there. > > Another choice is to just use EncryptionScheme(), which is okay but not as > readable. > > We can also have a function AesCtrEncryptionScheme() to replace > EncryptionScheme(EncryptionScheme::kCipherModeAesCtr). Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:68: bool is_encrypted() const; On 2016/03/03 22:33:00, xhwang wrote: > nit: you can inline this as well. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:73: CipherMode mode_; On 2016/03/03 22:33:00, xhwang wrote: > You can specify default mode here. Personally I feel that's more clear > especially when there are multiple constructors. > > See "Non-Static Class Member Initializers" in https://chromium-cpp.appspot.com/ > > Same for members in PatternSpec. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... File media/base/video_decoder_config.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... media/base/video_decoder_config.cc:43: encryption_scheme_() {} On 2016/03/03 22:33:01, xhwang wrote: > This is not necessary. Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/vide... media/base/video_decoder_config.cc:118: } On 2016/03/03 22:33:01, xhwang wrote: > nit: this can be inlined in the header file Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... File media/formats/mp2t/es_parser_h264.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... media/formats/mp2t/es_parser_h264.cc:319: natural_size, std::vector<uint8_t>(), scheme); On 2016/03/03 22:33:01, xhwang wrote: > We can use EmptyExtraData() here for std::vector<uint8_t>() Done. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... File media/formats/mp4/mp4_stream_parser.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/formats/m... media/formats/mp4/mp4_stream_parser.cc:354: std::vector<uint8_t>(), encryption_scheme); On 2016/03/03 22:33:01, xhwang wrote: > ditto about EmptyExtraData() Done.
looking good, just a few nits left. https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... File chromecast/public/media/decoder_config.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/chromecast/publ... chromecast/public/media/decoder_config.h:97: struct EncryptionScheme { On 2016/03/04 19:07:29, dougsteed wrote: > On 2016/03/03 22:56:44, halliwell wrote: > > On 2016/03/03 22:33:00, xhwang wrote: > > > Is there any reason you need to duplication this in chromecast code? > > > > Yep, we ship just this directory (chromecast/public) as headers for Cast > > hardware vendors to include and build against, so they don't need a full > > Chromium checkout. > > > > I'm not sure why it's not the same as the other version though. > > Not sure what you mean by "not the same". As far as I know it is the same except > for systemic changes: this file uses structs while media/base uses classes, etc. Yeah, I was talking about struct/class and unencrypted/Unencrypted. https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... File media/base/encryption_scheme.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/280001/media/base/encr... media/base/encryption_scheme.h:20: kCipherModeUnencrypted, On 2016/03/04 19:07:30, dougsteed wrote: > On 2016/03/03 22:33:00, xhwang wrote: > > In new code we should use UPPER_CASE for enums: > > > > https://www.chromium.org/developers/coding-style#TOC-Naming > > Prefer consistency with the other enums in media. Also we have already exposed > these names to our OEM partners, so would rather not change at this point. Those enums predate the requirement of using UPPER_CASE for enums in Chromium. They are just not updated yet to meet the new requirement. Also, it sounds a bit odd that media/base can't be changed because chromecast OEM partners are using one of the interfaces... https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... File media/base/encryption_scheme.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... media/base/encryption_scheme.cc:9: EncryptionScheme::Pattern::Pattern() : encrypt_blocks_(0), skip_blocks_(0) {} initialization list not needed any more since you have default values https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... media/base/encryption_scheme.cc:27: : mode_(kCipherModeUnencrypted), pattern_() {} ditto https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... media/base/encryption_scheme.cc:27: : mode_(kCipherModeUnencrypted), pattern_() {} pattern_() not needed https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/formats/m... File media/formats/mp2t/es_parser_mpeg1audio.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/formats/m... media/formats/mp2t/es_parser_mpeg1audio.cc:175: std::vector<uint8_t>(), Unencrypted()); nti: EmptyExtraData(), thanks!
My last few comments have been addressed. I'll let xhwang take it from here. https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... File media/base/encryption_scheme.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... media/base/encryption_scheme.h:60: // This constructor allows specification of the cipher mode with no pattern. Now that we have the medi_util helpers, how much do we use this constructor? Can we eliminate it now?
dougsteed@chromium.org changed reviewers: + raymes@chromium.org
raymes@: please check the one change in pepper https://codereview.chromium.org/1490613005/diff/280001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1490613005/diff/280001/chromecast/public/medi... chromecast/public/media/decoder_config.h:97: struct EncryptionScheme { On 2016/03/04 20:00:48, xhwang wrote: > On 2016/03/04 19:07:29, dougsteed wrote: > > On 2016/03/03 22:56:44, halliwell wrote: > > > On 2016/03/03 22:33:00, xhwang wrote: > > > > Is there any reason you need to duplication this in chromecast code? > > > > > > Yep, we ship just this directory (chromecast/public) as headers for Cast > > > hardware vendors to include and build against, so they don't need a full > > > Chromium checkout. > > > > > > I'm not sure why it's not the same as the other version though. > > > > Not sure what you mean by "not the same". As far as I know it is the same > except > > for systemic changes: this file uses structs while media/base uses classes, > etc. > > Yeah, I was talking about struct/class and unencrypted/Unencrypted. I eliminated the unencrypted/Unencrypted distinction. https://codereview.chromium.org/1490613005/diff/280001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/280001/media/base/encryption_... media/base/encryption_scheme.h:20: kCipherModeUnencrypted, On 2016/03/04 20:00:48, xhwang wrote: > On 2016/03/04 19:07:30, dougsteed wrote: > > On 2016/03/03 22:33:00, xhwang wrote: > > > In new code we should use UPPER_CASE for enums: > > > > > > https://www.chromium.org/developers/coding-style#TOC-Naming > > > > Prefer consistency with the other enums in media. Also we have already exposed > > these names to our OEM partners, so would rather not change at this point. > > Those enums predate the requirement of using UPPER_CASE for enums in Chromium. > They are just not updated yet to meet the new requirement. > > Also, it sounds a bit odd that media/base can't be changed because chromecast > OEM partners are using one of the interfaces... I misspoke in my comment about OEMs. I was actually thinking of the similar enums in chromecast/public/media. I completely agree that media/base should not be influenced by that. It sounds like you are not concerned about the CipherMode naming being inconsistent with the other enums in this same file (which was my main concern) so I will go ahead and change. https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... media/base/encryption_scheme.cc:9: EncryptionScheme::Pattern::Pattern() : encrypt_blocks_(0), skip_blocks_(0) {} On 2016/03/04 20:00:48, xhwang wrote: > initialization list not needed any more since you have default values Done. https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... media/base/encryption_scheme.cc:27: : mode_(kCipherModeUnencrypted), pattern_() {} On 2016/03/04 20:00:48, xhwang wrote: > pattern_() not needed Done. https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... media/base/encryption_scheme.h:60: // This constructor allows specification of the cipher mode with no pattern. On 2016/03/04 20:24:36, ddorwin wrote: > Now that we have the medi_util helpers, how much do we use this constructor? Can > we eliminate it now? Done. https://codereview.chromium.org/1490613005/diff/320001/media/formats/mp2t/es_... File media/formats/mp2t/es_parser_mpeg1audio.cc (right): https://codereview.chromium.org/1490613005/diff/320001/media/formats/mp2t/es_... media/formats/mp2t/es_parser_mpeg1audio.cc:175: std::vector<uint8_t>(), Unencrypted()); On 2016/03/04 20:00:48, xhwang wrote: > nti: EmptyExtraData(), thanks! Done.
Thanks! I only have some tiny nits left. https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... File media/base/encryption_scheme.h (right): https://chromiumcodereview.appspot.com/1490613005/diff/320001/media/base/encr... media/base/encryption_scheme.h:60: // This constructor allows specification of the cipher mode with no pattern. On 2016/03/07 17:49:13, dougsteed wrote: > On 2016/03/04 20:24:36, ddorwin wrote: > > Now that we have the medi_util helpers, how much do we use this constructor? > Can > > we eliminate it now? > > Done. This is not done yet... https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/encr... File media/base/encryption_scheme.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/encr... media/base/encryption_scheme.cc:26: EncryptionScheme::EncryptionScheme() : mode_(CIPHER_MODE_UNENCRYPTED) {} Initialization list not needed since CIPHER_MODE_UNENCRYPTED is the default mode. https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/encr... media/base/encryption_scheme.cc:28: EncryptionScheme::EncryptionScheme(CipherMode mode) : mode_(mode), pattern_() {} pattern_() not needed https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/fake... File media/base/fake_demuxer_stream.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/fake... media/base/fake_demuxer_stream.cc:157: EmptyExtraData(), encryption_scheme); nit: This can now be is_encrypted_ ? AesCtrEncryptionScheme() : Unencrypted() https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/medi... File media/base/media_util.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/medi... media/base/media_util.cc:19: EncryptionScheme::Pattern()); nit: EncryptionScheme(EncryptionScheme::CIPHER_MODE_AES_CTR) should work? Well ignore this since you'll drop this constructor per ddorwin's comment :) https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/test... File media/base/test_helpers.cc (right): https://chromiumcodereview.appspot.com/1490613005/diff/340001/media/base/test... media/base/test_helpers.cc:140: EmptyExtraData(), encryption_scheme); ditto
https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... File media/base/encryption_scheme.h (right): https://codereview.chromium.org/1490613005/diff/320001/media/base/encryption_... media/base/encryption_scheme.h:60: // This constructor allows specification of the cipher mode with no pattern. On 2016/03/07 18:39:52, xhwang wrote: > On 2016/03/07 17:49:13, dougsteed wrote: > > On 2016/03/04 20:24:36, ddorwin wrote: > > > Now that we have the medi_util helpers, how much do we use this constructor? > > Can > > > we eliminate it now? > > > > Done. > > This is not done yet... Done. https://codereview.chromium.org/1490613005/diff/340001/media/base/encryption_... File media/base/encryption_scheme.cc (right): https://codereview.chromium.org/1490613005/diff/340001/media/base/encryption_... media/base/encryption_scheme.cc:26: EncryptionScheme::EncryptionScheme() : mode_(CIPHER_MODE_UNENCRYPTED) {} On 2016/03/07 18:39:52, xhwang wrote: > Initialization list not needed since CIPHER_MODE_UNENCRYPTED is the default > mode. Done. https://codereview.chromium.org/1490613005/diff/340001/media/base/encryption_... media/base/encryption_scheme.cc:28: EncryptionScheme::EncryptionScheme(CipherMode mode) : mode_(mode), pattern_() {} On 2016/03/07 18:39:52, xhwang wrote: > pattern_() not needed Done. https://codereview.chromium.org/1490613005/diff/340001/media/base/fake_demuxe... File media/base/fake_demuxer_stream.cc (right): https://codereview.chromium.org/1490613005/diff/340001/media/base/fake_demuxe... media/base/fake_demuxer_stream.cc:157: EmptyExtraData(), encryption_scheme); On 2016/03/07 18:39:52, xhwang wrote: > nit: This can now be > > is_encrypted_ ? AesCtrEncryptionScheme() : Unencrypted() Done. https://codereview.chromium.org/1490613005/diff/340001/media/base/media_util.cc File media/base/media_util.cc (right): https://codereview.chromium.org/1490613005/diff/340001/media/base/media_util.... media/base/media_util.cc:19: EncryptionScheme::Pattern()); On 2016/03/07 18:39:52, xhwang wrote: > nit: EncryptionScheme(EncryptionScheme::CIPHER_MODE_AES_CTR) should work? > > Well ignore this since you'll drop this constructor per ddorwin's comment :) Done. https://codereview.chromium.org/1490613005/diff/340001/media/base/test_helper... File media/base/test_helpers.cc (right): https://codereview.chromium.org/1490613005/diff/340001/media/base/test_helper... media/base/test_helpers.cc:140: EmptyExtraData(), encryption_scheme); On 2016/03/07 18:39:52, xhwang wrote: > ditto Done.
LGTM! Thanks!
lgtm
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/patch-status/1490613005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
dougsteed@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@ need to add you back in, since there is a change needed in //content that I missed, and for which you are an owner.
lgtm
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/patch-status/1490613005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougsteed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, xhwang@chromium.org, raymes@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1490613005/#ps420001 (title: "missed a change needed in content")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490613005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougsteed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490613005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougsteed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, raymes@chromium.org, dalecurtis@chromium.org, halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1490613005/#ps440001 (title: "rebase needed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490613005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougsteed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490613005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/440001
Message was sent while issue was closed.
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710}
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/1786733004/ by alexmos@chromium.org. The reason for reverting is: Appears to be breaking compile on Win x64 GN (dbg): https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29... Output: media_type_converters.obj : error LNK2019: unresolved external symbol "public: __cdecl media::EncryptionScheme::Pattern::Pattern(unsigned int,unsigned int)" (??0Pattern@EncryptionScheme@media@@QEAA@II@Z) referenced in function "public: static class media::EncryptionScheme::Pattern __cdecl mojo::TypeConverter<class media::EncryptionScheme::Pattern,class mojo::InlinedStructPtr<class media::interfaces::Pattern> >::Convert(class mojo::InlinedStructPtr<class media::interfaces::Pattern> const &)" (?Convert@?$TypeConverter@VPattern@EncryptionScheme@media@@V?$InlinedStructPtr@VPattern@interfaces@media@@@mojo@@@mojo@@SA?AVPattern@EncryptionScheme@media@@AEBV?$InlinedStructPtr@VPattern@interfaces@media@@@2@@Z) media_type_converters.obj : error LNK2019: unresolved external symbol "public: __cdecl media::EncryptionScheme::Pattern::~Pattern(void)" (??1Pattern@EncryptionScheme@media@@QEAA@XZ) referenced in function "public: static class media::EncryptionScheme __cdecl mojo::TypeConverter<class media::EncryptionScheme,class mojo::StructPtr<class media::interfaces::EncryptionScheme> >::Convert(class mojo::StructPtr<class media::interfaces::EncryptionScheme> const &)" (?Convert@?$TypeConverter@VEncryptionScheme@media@@V?$StructPtr@VEncryptionScheme@interfaces@media@@@mojo@@@mojo@@SA?AVEncryptionScheme@media@@AEBV?$StructPtr@VEncryptionScheme@interfaces@media@@@2@@Z) ./media_library.dll : fatal error LNK1120: 2 unresolved externals .
Message was sent while issue was closed.
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ==========
The CQ bit was checked by dougsteed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, raymes@chromium.org, dalecurtis@chromium.org, halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1490613005/#ps460001 (title: "add MEDIA_EXPORT to inner class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490613005/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490613005/460001
Message was sent while issue was closed.
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} ========== to ========== media config: expand is_encrypted to a struct. Provide more complete encryption metadata, rather than just a bool. EncryptionScheme also allows specification of the mode and the pattern, as will be allowed by CENC (ISO's Common Encryption standard), 3rd Edition. BUG=568326 Committed: https://crrev.com/c9d2206c62f65e29b141e08df2b2dcb88f54162f Cr-Commit-Position: refs/heads/master@{#380710} Committed: https://crrev.com/8d5275f7bbb6f79c83829a7931f1500128ef3f8f Cr-Commit-Position: refs/heads/master@{#380791} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/8d5275f7bbb6f79c83829a7931f1500128ef3f8f Cr-Commit-Position: refs/heads/master@{#380791} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
