Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829470:
Support for parsing encrypted WebM streams by src. (Closed)
|
Created:
8 years, 4 months ago by fgalligan1 Modified:
7 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport for parsing encrypted WebM streams by src.
- Note: Only looking for comments on direction. A lot of work
still needs to be done before committing.
- Added support to FFmpegDemuxer to decrypt encrypted WebM streams.
- Added support to FFmpegDemuxer to handle the needKey and keyAdded
messages.
- Added support to WebMediaPlayerImpl to handle the needKey and
keyAdded messages.
BUG=123426
TEST=All media_unittests pass
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188228
Patch Set 1 #
Total comments: 51
Patch Set 2 : Addressing comments. #
Total comments: 4
Patch Set 3 : Fixed bug with playing latest encrypted WebM. Addressing comments. #
Total comments: 47
Patch Set 4 : Rebase code. Add support for audio. #
Total comments: 41
Patch Set 5 : Refactored decryption code. Add 2 unittests. #
Total comments: 34
Patch Set 6 : Addressing comments #
Total comments: 8
Patch Set 7 : Addressing comments #
Total comments: 6
Patch Set 8 : Add Mock method #Patch Set 9 : Addressing comments #
Total comments: 5
Patch Set 10 : #Patch Set 11 : Added output if DecryptConfig failed #Patch Set 12 : #Patch Set 13 : Fix 2 tools. One mac issue. #Patch Set 14 : Fix sign issue #Patch Set 15 : Fix tool player_x11 #
Messages
Total messages: 26 (0 generated)
I'm not sure why we need keyadded or a separate needkey CB. I also have some suggestions on how to handle the key ID. @xhwang: Please skim, especially around my comments, since you may have suggestions based on your experience. http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h#newcode67 media/base/demuxer.h:67: virtual void KeyAdded(); Comment. Also, curious why this is needed now and wasn't before. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. The CTR counter block format is a We seem to do this a lot. Can we put it in media/base or something? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:87: CHECK(!enc_key_id_.empty()); DCHECK? Do we prevent this in FFmpeg? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:88: need_decryption_key_ = true; The lifetime of this object is the entire media stream, right? Does it support more than one Track (and whatever holds PSSHs for ISO)? I worry about holding onto this data when more may come in. Does this class all run on the same thread? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:93: //enc_key_id_ = "DEBUG_KEY_REMOVE"; Could you av_dict_set() at 81 instead and thus exercise the above code? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:97: } I think the use of enc_key_id_ for both reporting needkey AND creating the DecryptConfig is confusing. I propose current_key_id_, which works for ISO and WebM. Then here: 1) base64 decode "initData" into a local string 2) Report it via NeedKey 3) Container-specific extraction of "current key" into current_key_id_ http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:127: // Every encrypted Block has an HMAC and IV prepended to it. Current We seem to do this a lot. Can we put it in media/base or something? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:130: // If encrypted skip past the HMAC. Encrypted buffers must include the IV If encrypted, skip past the HMAC to get the buffer. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:140: if (!enc_key_id_.empty()) { We now have WebM code in the middle of the generic demuxer. Can we avoid that? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:251: if (!read_queue_.empty() && !need_decryption_key_) { Why not post if need a decryption key? It's up to the decoder to stall. If we don't need to check this, I don't think we need need_decryption_key_ . http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h File media/filters/ffmpeg_demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:139: bool need_decryption_key_; Curious why the demuxer cares. It shouldn't be blocking. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:140: std::string enc_key_id_; Why do we store the key ID as state? There can be multiple. This is specific to WebM too. (I'm fine with only supporting WebM now, but we should try to name appropriately from the start.) http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:183: void NeedKey(const std::string& key_id); Should this be public? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:287: FFmpegNeedKeyCB need_key_cb_; const http://codereview.chromium.org/10829470/diff/1/webkit/media/filter_helpers.h File webkit/media/filter_helpers.h (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/filter_helpers.h#... webkit/media/filter_helpers.h:10: #include "media/filters/ffmpeg_demuxer.h" This shouldn't rely on FFmpeg-specific code. We need something in base for this callback. Currently, NeedKeyCB is in media/base/stream_parser.h, but we could move it elsewhere. http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... webkit/media/webmediaplayer_impl.cc:910: proxy_->DemuxerNeedKey(init_data.Pass(), init_data_size); Why is this different from the above? Isn't the chunk demuxer using the above? http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... File webkit/media/webmediaplayer_impl.h (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... webkit/media/webmediaplayer_impl.h:251: bool SendNeedKey(scoped_array<uint8> init_data, int init_data_size); Why can't you use OnNeedKey?
https://chromiumcodereview.appspot.com/10829470/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://chromiumcodereview.appspot.com/10829470/diff/1/media/base/demuxer.h#n... media/base/demuxer.h:67: virtual void KeyAdded(); On 2012/08/22 23:20:29, ddorwin wrote: > Comment. > Also, curious why this is needed now and wasn't before. I'm not sure I understand the question. Do you mean why did we not need this in the first media source implementation? Because the javascript was controlling when the encrypted data was passed in. And in the demo we didn't pass any data until we gave media source the key. I'm guessing if the application didn't wait the decoder would not have been happy. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. The CTR counter block format is a On 2012/08/22 23:20:29, ddorwin wrote: > We seem to do this a lot. Can we put it in media/base or something? Sure is there a convention where it should go? https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:87: CHECK(!enc_key_id_.empty()); On 2012/08/22 23:20:29, ddorwin wrote: > DCHECK? Do we prevent this in FFmpeg? In the current code in FFmpeg we will only ads an enc_key_id metadata if ContentEncKeyID's size is > 0. If you want me to not have the DCHECK I can remove it. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:88: need_decryption_key_ = true; On 2012/08/22 23:20:29, ddorwin wrote: > The lifetime of this object is the entire media stream, right? Does it support > more than one Track (and whatever holds PSSHs for ISO)? I worry about holding > onto this data when more may come in. > > Does this class all run on the same thread? I don't think FFmpegDemuxerStream supports more than one Track. Currently WebM has one key_id per stream. We can always change this when ISO support is added or WebM can changes keys within the same Track. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:93: //enc_key_id_ = "DEBUG_KEY_REMOVE"; On 2012/08/22 23:20:29, ddorwin wrote: > Could you av_dict_set() at 81 instead and thus exercise the above code? Done. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:97: } On 2012/08/22 23:20:29, ddorwin wrote: > I think the use of enc_key_id_ for both reporting needkey AND creating the > DecryptConfig is confusing. I propose current_key_id_, which works for ISO and > WebM. > Then here: > 1) base64 decode "initData" into a local string > 2) Report it via NeedKey > 3) Container-specific extraction of "current key" into current_key_id_ PTAL and let me if what I did is what you were asking for. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:127: // Every encrypted Block has an HMAC and IV prepended to it. Current On 2012/08/22 23:20:29, ddorwin wrote: > We seem to do this a lot. Can we put it in media/base or something? You want me to remove the comment? https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:130: // If encrypted skip past the HMAC. Encrypted buffers must include the IV On 2012/08/22 23:20:29, ddorwin wrote: > If encrypted, skip past the HMAC to get the buffer. Done. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:140: if (!enc_key_id_.empty()) { On 2012/08/22 23:20:29, ddorwin wrote: > We now have WebM code in the middle of the generic demuxer. Can we avoid that? Added a TODO, which can be done before the CL is submitted so we don't lose this. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:251: if (!read_queue_.empty() && !need_decryption_key_) { On 2012/08/22 23:20:29, ddorwin wrote: > Why not post if need a decryption key? It's up to the decoder to stall. > If we don't need to check this, I don't think we need need_decryption_key_ . I thought this was a pretty clean break point in waiting for the key. There is not much code in the demux_task between reading a frame and decoding it. So you would want the decoder (which currently contains the decryptor) to block until we have the key? That would mean decoder would then have to know that the stream is encrypted also. I wanted to keep as much of the encryption handling code out of the decoder. I don't think the decoder should know anything about decryption. That should be handled before it even gets to the decoder. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.h (right): https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:139: bool need_decryption_key_; On 2012/08/22 23:20:29, ddorwin wrote: > Curious why the demuxer cares. It shouldn't be blocking. See the other comment. Also if we don't have the demuxer block I think we will need to re-architect the demuxer decoder interface. I think the demuxer expects the decoder will only have one buffer at a time. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:140: std::string enc_key_id_; On 2012/08/22 23:20:29, ddorwin wrote: > Why do we store the key ID as state? There can be multiple. This is specific to > WebM too. (I'm fine with only supporting WebM now, but we should try to name > appropriately from the start.) I'm using this to decide if to create a DecryptConfig. Renamed to current_key_id_. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:183: void NeedKey(const std::string& key_id); On 2012/08/22 23:20:29, ddorwin wrote: > Should this be public? This is called from FFmpegDemuxerStream. https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:287: FFmpegNeedKeyCB need_key_cb_; On 2012/08/22 23:20:29, ddorwin wrote: > const Done. https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/filter_he... File webkit/media/filter_helpers.h (right): https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/filter_he... webkit/media/filter_helpers.h:10: #include "media/filters/ffmpeg_demuxer.h" On 2012/08/22 23:20:29, ddorwin wrote: > This shouldn't rely on FFmpeg-specific code. We need something in base for this > callback. Currently, NeedKeyCB is in media/base/stream_parser.h, but we could > move it elsewhere. Right, what I was thinking of doing was creating a new interface NeedKey or something that both ChunkDemuxer and FFmpegDemuxer would implement. I just didn't have time to do it. Added a TODO. https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... File webkit/media/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... webkit/media/webmediaplayer_impl.cc:910: proxy_->DemuxerNeedKey(init_data.Pass(), init_data_size); On 2012/08/22 23:20:29, ddorwin wrote: > Why is this different from the above? Isn't the chunk demuxer using the above? Because right now the the interface is defined in ChunkDemuxer. I didn't have time to split it out. That was my plan to move the interface definition from ChunkDemuxer to somewhere else and have both ChunkDemuxer and FFMpegDemuxer implement the interface. https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... File webkit/media/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... webkit/media/webmediaplayer_impl.h:251: bool SendNeedKey(scoped_array<uint8> init_data, int init_data_size); On 2012/08/22 23:20:29, ddorwin wrote: > Why can't you use OnNeedKey? See comment in .cc
Only looked at the big picture. In summary: 1, it'll be great to have the ChunkDemuxer and FFmpegDemuxer share the same code to report NeedKey. 2, The demuxer shouldn't care if the key is added or not. It should just report NeedKey when it finds a key_id, return a buffer when Read() is called and should not worry about if the buffer can be decrypted or now. On 2012/08/23 02:39:10, fgalligan1 wrote: > https://chromiumcodereview.appspot.com/10829470/diff/1/media/base/demuxer.h > File media/base/demuxer.h (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/base/demuxer.h#n... > media/base/demuxer.h:67: virtual void KeyAdded(); > On 2012/08/22 23:20:29, ddorwin wrote: > > Comment. > > Also, curious why this is needed now and wasn't before. > > I'm not sure I understand the question. Do you mean why did we not need this in > the first media source implementation? Because the javascript was controlling > when the encrypted data was passed in. And in the demo we didn't pass any data > until we gave media source the key. I'm guessing if the application didn't wait > the decoder would not have been happy. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > File media/filters/ffmpeg_demuxer.cc (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. > The CTR counter block format is a > On 2012/08/22 23:20:29, ddorwin wrote: > > We seem to do this a lot. Can we put it in media/base or something? > > Sure is there a convention where it should go? > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:87: CHECK(!enc_key_id_.empty()); > On 2012/08/22 23:20:29, ddorwin wrote: > > DCHECK? Do we prevent this in FFmpeg? > > In the current code in FFmpeg we will only ads an enc_key_id metadata if > ContentEncKeyID's size is > 0. If you want me to not have the DCHECK I can > remove it. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:88: need_decryption_key_ = true; > On 2012/08/22 23:20:29, ddorwin wrote: > > The lifetime of this object is the entire media stream, right? Does it support > > more than one Track (and whatever holds PSSHs for ISO)? I worry about holding > > onto this data when more may come in. > > > > Does this class all run on the same thread? > > I don't think FFmpegDemuxerStream supports more than one Track. Currently WebM > has one key_id per stream. We can always change this when ISO support is added > or WebM can changes keys within the same Track. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:93: //enc_key_id_ = "DEBUG_KEY_REMOVE"; > On 2012/08/22 23:20:29, ddorwin wrote: > > Could you av_dict_set() at 81 instead and thus exercise the above code? > > Done. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:97: } > On 2012/08/22 23:20:29, ddorwin wrote: > > I think the use of enc_key_id_ for both reporting needkey AND creating the > > DecryptConfig is confusing. I propose current_key_id_, which works for ISO and > > WebM. > > Then here: > > 1) base64 decode "initData" into a local string > > 2) Report it via NeedKey > > 3) Container-specific extraction of "current key" into current_key_id_ > PTAL and let me if what I did is what you were asking for. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:127: // Every encrypted Block has an HMAC and IV > prepended to it. Current > On 2012/08/22 23:20:29, ddorwin wrote: > > We seem to do this a lot. Can we put it in media/base or something? > You want me to remove the comment? > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:130: // If encrypted skip past the HMAC. > Encrypted buffers must include the IV > On 2012/08/22 23:20:29, ddorwin wrote: > > If encrypted, skip past the HMAC to get the buffer. > > Done. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:140: if (!enc_key_id_.empty()) { > On 2012/08/22 23:20:29, ddorwin wrote: > > We now have WebM code in the middle of the generic demuxer. Can we avoid that? > Added a TODO, which can be done before the CL is submitted so we don't lose > this. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:251: if (!read_queue_.empty() && > !need_decryption_key_) { > On 2012/08/22 23:20:29, ddorwin wrote: > > Why not post if need a decryption key? It's up to the decoder to stall. > > If we don't need to check this, I don't think we need need_decryption_key_ . > > I thought this was a pretty clean break point in waiting for the key. There is > not much code in the demux_task between reading a frame and decoding it. > > So you would want the decoder (which currently contains the decryptor) to block > until we have the key? That would mean decoder would then have to know that the > stream is encrypted also. > > I wanted to keep as much of the encryption handling code out of the decoder. I > don't think the decoder should know anything about decryption. That should be > handled before it even gets to the decoder. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > File media/filters/ffmpeg_demuxer.h (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.h:139: bool need_decryption_key_; > On 2012/08/22 23:20:29, ddorwin wrote: > > Curious why the demuxer cares. It shouldn't be blocking. > > See the other comment. Also if we don't have the demuxer block I think we will > need to re-architect the demuxer decoder interface. I think the demuxer expects > the decoder will only have one buffer at a time. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.h:140: std::string enc_key_id_; > On 2012/08/22 23:20:29, ddorwin wrote: > > Why do we store the key ID as state? There can be multiple. This is specific > to > > WebM too. (I'm fine with only supporting WebM now, but we should try to name > > appropriately from the start.) > > I'm using this to decide if to create a DecryptConfig. Renamed to > current_key_id_. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.h:183: void NeedKey(const std::string& key_id); > On 2012/08/22 23:20:29, ddorwin wrote: > > Should this be public? > This is called from FFmpegDemuxerStream. > > https://chromiumcodereview.appspot.com/10829470/diff/1/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.h:287: FFmpegNeedKeyCB need_key_cb_; > On 2012/08/22 23:20:29, ddorwin wrote: > > const > > Done. > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/filter_he... > File webkit/media/filter_helpers.h (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/filter_he... > webkit/media/filter_helpers.h:10: #include "media/filters/ffmpeg_demuxer.h" > On 2012/08/22 23:20:29, ddorwin wrote: > > This shouldn't rely on FFmpeg-specific code. We need something in base for > this > > callback. Currently, NeedKeyCB is in media/base/stream_parser.h, but we could > > move it elsewhere. > Right, what I was thinking of doing was creating a new interface NeedKey or > something that both ChunkDemuxer and FFmpegDemuxer would implement. I just > didn't have time to do it. Added a TODO. > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... > File webkit/media/webmediaplayer_impl.cc (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... > webkit/media/webmediaplayer_impl.cc:910: > proxy_->DemuxerNeedKey(init_data.Pass(), init_data_size); > On 2012/08/22 23:20:29, ddorwin wrote: > > Why is this different from the above? Isn't the chunk demuxer using the above? > Because right now the the interface is defined in ChunkDemuxer. I didn't have > time to split it out. That was my plan to move the interface definition from > ChunkDemuxer to somewhere else and have both ChunkDemuxer and FFMpegDemuxer > implement the interface. > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... > File webkit/media/webmediaplayer_impl.h (right): > > https://chromiumcodereview.appspot.com/10829470/diff/1/webkit/media/webmediap... > webkit/media/webmediaplayer_impl.h:251: bool SendNeedKey(scoped_array<uint8> > init_data, int init_data_size); > On 2012/08/22 23:20:29, ddorwin wrote: > > Why can't you use OnNeedKey? > See comment in .cc
Oops, forgot to send my comments out. Please see comments. http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h#newcode67 media/base/demuxer.h:67: virtual void KeyAdded(); On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > Comment. > > Also, curious why this is needed now and wasn't before. > > I'm not sure I understand the question. Do you mean why did we not need this in > the first media source implementation? Because the javascript was controlling > when the encrypted data was passed in. And in the demo we didn't pass any data > until we gave media source the key. I'm guessing if the application didn't wait > the decoder would not have been happy. I removed the "waiting for key" part in the demo, so this is not necessary. If the decryptor doesn't have a key when trying to decrypt an encrypted frame, the media pipeline will wait until the key is added, or the pipeline is stopped. In spec about this: section 5.1.9: Key Presense The CL in chrome: http://codereview.chromium.org/10822026/ http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. The CTR counter block format is a On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > We seem to do this a lot. Can we put it in media/base or something? > > Sure is there a convention where it should go? media/crypto/decryptor_helpers.* ? http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h File media/filters/ffmpeg_demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:139: bool need_decryption_key_; On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > Curious why the demuxer cares. It shouldn't be blocking. > > See the other comment. Also if we don't have the demuxer block I think we will > need to re-architect the demuxer decoder interface. I think the demuxer expects > the decoder will only have one buffer at a time. I am not quite sure what's the problem. But the DemuxerStream just give out a buffer when Read() is called if it can, or hold the callback and try to get more data until it can give a buffer out. It should not worry about if the decoder can decrypt or not. So in summary, when the demuxer found a key_id, it should fire NeedKey with the key_id and forget it. Whenever Read() is called the demuxer should hand out a frame (encrypted or not) and should not worry about if the decryptor can decrypt it. http://codereview.chromium.org/10829470/diff/1/webkit/media/filter_helpers.h File webkit/media/filter_helpers.h (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/filter_helpers.h#... webkit/media/filter_helpers.h:10: #include "media/filters/ffmpeg_demuxer.h" On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > This shouldn't rely on FFmpeg-specific code. We need something in base for > this > > callback. Currently, NeedKeyCB is in media/base/stream_parser.h, but we could > > move it elsewhere. > Right, what I was thinking of doing was creating a new interface NeedKey or > something that both ChunkDemuxer and FFmpegDemuxer would implement. I just > didn't have time to do it. Added a TODO. > Yes, having ChunkDemuxer and FFmpegDemuxer share the same way to report NeedKey would be great!
I think the summary is: * Common DemuxerClient. * Probably don't need to wait and thus don't need a member and AddKey(). * Extract/generalize WebM. (TODOs would be fine here.) http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. The CTR counter block format is a On 2012/08/23 19:04:53, xhwang wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > We seem to do this a lot. Can we put it in media/base or something? > > > > Sure is there a convention where it should go? > > media/crypto/decryptor_helpers.* ? and/or webm_helpers.* depending on what it is. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:87: CHECK(!enc_key_id_.empty()); On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > DCHECK? Do we prevent this in FFmpeg? > > In the current code in FFmpeg we will only ads an enc_key_id metadata if > ContentEncKeyID's size is > 0. If you want me to not have the DCHECK I can > remove it. I was just wondering whether DCHECK was sufficient or whether we really need the check in opt. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:88: need_decryption_key_ = true; On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > The lifetime of this object is the entire media stream, right? Does it support > > more than one Track (and whatever holds PSSHs for ISO)? I worry about holding > > onto this data when more may come in. > > > > Does this class all run on the same thread? > > I don't think FFmpegDemuxerStream supports more than one Track. Currently WebM > has one key_id per stream. We can always change this when ISO support is added > or WebM can changes keys within the same Track. > We should find out how acolwell's changes to handle changes work. Anyway, hopefully we can just eliminate this since xhwang has fixed blocking for the key inside Chrome. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:127: // Every encrypted Block has an HMAC and IV prepended to it. Current On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > We seem to do this a lot. Can we put it in media/base or something? > You want me to remove the comment? No. Move the code like line 32. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:251: if (!read_queue_.empty() && !need_decryption_key_) { On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > Why not post if need a decryption key? It's up to the decoder to stall. > > If we don't need to check this, I don't think we need need_decryption_key_ . > > I thought this was a pretty clean break point in waiting for the key. There is > not much code in the demux_task between reading a frame and decoding it. > > So you would want the decoder (which currently contains the decryptor) to block > until we have the key? That would mean decoder would then have to know that the > stream is encrypted also. > > I wanted to keep as much of the encryption handling code out of the decoder. I > don't think the decoder should know anything about decryption. That should be > handled before it even gets to the decoder. As xhwang mentioned, I think this is already handled. You could try removing it and confirm it works. Then you can remove AddKey(), etc. http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... webkit/media/webmediaplayer_impl.cc:910: proxy_->DemuxerNeedKey(init_data.Pass(), init_data_size); On 2012/08/23 02:39:11, fgalligan1 wrote: > On 2012/08/22 23:20:29, ddorwin wrote: > > Why is this different from the above? Isn't the chunk demuxer using the above? > Because right now the the interface is defined in ChunkDemuxer. I didn't have > time to split it out. That was my plan to move the interface definition from > ChunkDemuxer to somewhere else and have both ChunkDemuxer and FFMpegDemuxer > implement the interface. Okay, maybe a media/base version of ChunkDemuxerClient, which maybe ChunkDemuxerClient inherits from. Can you avoid SendNeedKey by binding the proxy directly above. Probably irrelevant since we would just do the above refactoring before committing, though. http://codereview.chromium.org/10829470/diff/1011/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1011/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:86: AVDictionaryEntry *key = av_dict_get(stream->metadata, "enc_key_id", NULL, 0); // Only supports WebM for now. http://codereview.chromium.org/10829470/diff/1011/webkit/media/filter_helpers.h File webkit/media/filter_helpers.h (right): http://codereview.chromium.org/10829470/diff/1011/webkit/media/filter_helpers... webkit/media/filter_helpers.h:11: // TODO(fgalligan): Remove the dependency on FFmpeg. above line 10
http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/base/demuxer.h#newcode67 media/base/demuxer.h:67: virtual void KeyAdded(); On 2012/08/23 19:04:53, xhwang wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > Comment. > > > Also, curious why this is needed now and wasn't before. > > > > I'm not sure I understand the question. Do you mean why did we not need this > in > > the first media source implementation? Because the javascript was controlling > > when the encrypted data was passed in. And in the demo we didn't pass any data > > until we gave media source the key. I'm guessing if the application didn't > wait > > the decoder would not have been happy. > > I removed the "waiting for key" part in the demo, so this is not necessary. If > the decryptor doesn't have a key when trying to decrypt an encrypted frame, the > media pipeline will wait until the key is added, or the pipeline is stopped. > > In spec about this: section 5.1.9: Key Presense > The CL in chrome: http://codereview.chromium.org/10822026/ Removed KeyAdded(). http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:32: // Generates a 16 byte CTR counter block. The CTR counter block format is a On 2012/08/24 00:20:30, ddorwin wrote: > On 2012/08/23 19:04:53, xhwang wrote: > > On 2012/08/23 02:39:11, fgalligan1 wrote: > > > On 2012/08/22 23:20:29, ddorwin wrote: > > > > We seem to do this a lot. Can we put it in media/base or something? > > > > > > Sure is there a convention where it should go? > > > > media/crypto/decryptor_helpers.* ? > > and/or webm_helpers.* depending on what it is. Done. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:87: CHECK(!enc_key_id_.empty()); On 2012/08/24 00:20:30, ddorwin wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > DCHECK? Do we prevent this in FFmpeg? > > > > In the current code in FFmpeg we will only ads an enc_key_id metadata if > > ContentEncKeyID's size is > 0. If you want me to not have the DCHECK I can > > remove it. > > I was just wondering whether DCHECK was sufficient or whether we really need the > check in opt. I had it as a DCHECK because at one point in time key_id was optional. But all of the code will fail if the value is NULL so made it a check. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:127: // Every encrypted Block has an HMAC and IV prepended to it. Current On 2012/08/24 00:20:30, ddorwin wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > We seem to do this a lot. Can we put it in media/base or something? > > You want me to remove the comment? > > No. Move the code like line 32. Done. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.c... media/filters/ffmpeg_demuxer.cc:251: if (!read_queue_.empty() && !need_decryption_key_) { On 2012/08/24 00:20:30, ddorwin wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > Why not post if need a decryption key? It's up to the decoder to stall. > > > If we don't need to check this, I don't think we need need_decryption_key_ . > > > > I thought this was a pretty clean break point in waiting for the key. There is > > not much code in the demux_task between reading a frame and decoding it. > > > > So you would want the decoder (which currently contains the decryptor) to > block > > until we have the key? That would mean decoder would then have to know that > the > > stream is encrypted also. > > > > I wanted to keep as much of the encryption handling code out of the decoder. I > > don't think the decoder should know anything about decryption. That should be > > handled before it even gets to the decoder. > > As xhwang mentioned, I think this is already handled. You could try removing it > and confirm it works. Then you can remove AddKey(), etc. Done. Works with xhwang changes. http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h File media/filters/ffmpeg_demuxer.h (right): http://codereview.chromium.org/10829470/diff/1/media/filters/ffmpeg_demuxer.h... media/filters/ffmpeg_demuxer.h:139: bool need_decryption_key_; On 2012/08/23 19:04:53, xhwang wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > Curious why the demuxer cares. It shouldn't be blocking. > > > > See the other comment. Also if we don't have the demuxer block I think we will > > need to re-architect the demuxer decoder interface. I think the demuxer > expects > > the decoder will only have one buffer at a time. > > I am not quite sure what's the problem. But the DemuxerStream just give out a > buffer when Read() is called if it can, or hold the callback and try to get more > data until it can give a buffer out. It should not worry about if the decoder > can decrypt or not. > > So in summary, when the demuxer found a key_id, it should fire NeedKey with the > key_id and forget it. Whenever Read() is called the demuxer should hand out a > frame (encrypted or not) and should not worry about if the decryptor can decrypt > it. Removed. With xhwang's change this will work now. http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10829470/diff/1/webkit/media/webmediaplayer_im... webkit/media/webmediaplayer_impl.cc:910: proxy_->DemuxerNeedKey(init_data.Pass(), init_data_size); On 2012/08/24 00:20:30, ddorwin wrote: > On 2012/08/23 02:39:11, fgalligan1 wrote: > > On 2012/08/22 23:20:29, ddorwin wrote: > > > Why is this different from the above? Isn't the chunk demuxer using the > above? > > Because right now the the interface is defined in ChunkDemuxer. I didn't have > > time to split it out. That was my plan to move the interface definition from > > ChunkDemuxer to somewhere else and have both ChunkDemuxer and FFMpegDemuxer > > implement the interface. > > Okay, maybe a media/base version of ChunkDemuxerClient, which maybe > ChunkDemuxerClient inherits from. > > Can you avoid SendNeedKey by binding the proxy directly above. Probably > irrelevant since we would just do the above refactoring before committing, > though. Removed SendNeedKey http://codereview.chromium.org/10829470/diff/1011/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/1011/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:86: AVDictionaryEntry *key = av_dict_get(stream->metadata, "enc_key_id", NULL, 0); On 2012/08/24 00:20:30, ddorwin wrote: > // Only supports WebM for now. Done. http://codereview.chromium.org/10829470/diff/1011/webkit/media/filter_helpers.h File webkit/media/filter_helpers.h (right): http://codereview.chromium.org/10829470/diff/1011/webkit/media/filter_helpers... webkit/media/filter_helpers.h:11: // TODO(fgalligan): Remove the dependency on FFmpeg. On 2012/08/24 00:20:30, ddorwin wrote: > above line 10 Done.
Looks nice! Just a few more comments. http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... File media/crypto/decryptor_helpers.cc (right): http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.cc:17: } // media // namespace media http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... File media/crypto/decryptor_helpers.h (right): http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.h:10: #include <string> c++ headers should go before chromium headers. So move this line before line 8. http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.h:19: } // webkit_media This should be "// namespace media" http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.cc (right): http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:70: CHECK(key->value); Can a malformed file generate a |key| without value? Or does av_dict_get enforce this is always true? We shouldn't crash on malformed files. http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:74: CHECK(!enc_key_id.empty()); Same as above, add error handling code instead of crashing? http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:348: need_key_cb_.Run(key_id_local.Pass(), key_id_size); Sad to see this conversion again. I think we need to address types we use for initData eventually. http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.h (right): http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.h:50: typedef base::Callback<void(scoped_array<uint8>, int)> FFmpegNeedKeyCB; Move this typedef into FFmpegDemuxer and rename it to just NeedKeyCB? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... File media/webm/webm_crypt_helpers.cc (right): http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:5: #include "media/webm/webm_crypt_helpers.h" How about rename this to "webm_crypto_helpers.h", to be consistent with "base/crypto" and "media/crypto"? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:49: } // media // namespace media http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... File media/webm/webm_crypt_helpers.h (right): http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:17: // encrypted. Currently encrypted streams must be WebM. This function should not worry about how the buffer is handled (by the Decryptor or decoder). http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:27: const uint8* key_id, int key_id_size); The only reason we use pointer/size for initData is that it *can* be large for ISO. For webm key_id is always smallish. So feel free to use std::string for key_id for webm specific code. http://codereview.chromium.org/10829470/diff/13001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): http://codereview.chromium.org/10829470/diff/13001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:279: base::Unretained(proxy_.get()))); Do you need base::Unretained? The WebMediaPlayerProxy should be refcounted.
http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... File media/crypto/decryptor_helpers.cc (right): http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.cc:12: std::string counter_block(reinterpret_cast<char*>(&iv), sizeof(iv)); I wonder if we'd be better off reserving the final size (kDecryptionKeySize) and copying in. http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... File media/crypto/decryptor_helpers.h (right): http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Are there likely to be more functions in this file? Maybe when ISO is supported. Having two files for one function seems odd. If this is WebM-specific - see below - can it go in the webm helper file? http://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_hel... media/crypto/decryptor_helpers.h:17: std::string GenerateCounterBlock(uint64 iv); GenerateWebMCounterBlock? http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.h (right): http://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.h:50: typedef base::Callback<void(scoped_array<uint8>, int)> FFmpegNeedKeyCB; On 2012/08/29 05:08:14, xhwang wrote: > Move this typedef into FFmpegDemuxer and rename it to just NeedKeyCB? We're going to eliminate this and replace it with a common Demuxer class, right? Add a TODO to do that and remove this? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... File media/webm/webm_crypt_helpers.cc (right): http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:5: #include "media/webm/webm_crypt_helpers.h" On 2012/08/29 05:08:14, xhwang wrote: > How about rename this to "webm_crypto_helpers.h", to be consistent with > "base/crypto" and "media/crypto"? Agreed. Is there a reason you chose "crypt"? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:7: #include "base/logging.h" Not needed? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:15: scoped_refptr<DecoderBuffer> WebMCopyBufferCheckIfEncrypted( This function does not "check if encrypted". So, it won't work for unencrypted WebM (or ISO). Right now, it's being called in all cases, which means anything but encrypted WebM is broken. http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:19: data + kWebMHmacSize, data_size - kWebMHmacSize); line breaks and alignment seem questionable. http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:23: int data_offset = sizeof(signal_byte); const int kDataOffset? "data_offset" is not directly related to "data", which is confusing. Is there a better name? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:31: uint64 network_iv; Might comment "IV is only present for encrypted frames" somewhere. http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.cc:35: GenerateCounterBlock(iv); on same line as above http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... File media/webm/webm_crypt_helpers.h (right): http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Same as the other helper file - two files for one function. Are there others that might end up in this file or can we merge the files? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:15: // Returns an initialized DecoderBuffer which can be sent to the Decryptor if nit: , which http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:17: // encrypted. Currently encrypted streams must be WebM. Isn't the "Currently..." sentence implicit in the name? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:21: // If encrypted, skip past the HMAC to get the buffer. Encrypted buffers This probably needs to be updated to handle verified but unencrypted blocks. http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:25: scoped_refptr<DecoderBuffer> WebMCopyBufferCheckIfEncrypted( The function name does not "flow" - it's two separate fragments. How about something like WebMCopyBufferWithEncryptionCheck? What does "CheckIfEncrypted" mean anyway? It's really just checking as part of the implementation. Maybe this should be WebMCopyPossiblyEncryptedBuffer? http://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_help... media/webm/webm_crypt_helpers.h:29: } // webkit_media media
Shall we resume this work? It would be nice to have EME working with src.
Some of the comments were obsolete as the code has been sitting for a long time. https://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_he... File media/crypto/decryptor_helpers.h (right): https://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_he... media/crypto/decryptor_helpers.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/01 16:46:41, ddorwin wrote: > Are there likely to be more functions in this file? Maybe when ISO is supported. > Having two files for one function seems odd. If this is WebM-specific - see > below - can it go in the webm helper file? Done. https://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_he... media/crypto/decryptor_helpers.h:10: #include <string> On 2012/08/29 05:08:14, xhwang wrote: > c++ headers should go before chromium headers. So move this line before line 8. Done. https://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_he... media/crypto/decryptor_helpers.h:17: std::string GenerateCounterBlock(uint64 iv); On 2012/09/01 16:46:41, ddorwin wrote: > GenerateWebMCounterBlock? Done. https://codereview.chromium.org/10829470/diff/13001/media/crypto/decryptor_he... media/crypto/decryptor_helpers.h:19: } // webkit_media On 2012/08/29 05:08:14, xhwang wrote: > This should be "// namespace media" Done. https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:70: CHECK(key->value); On 2012/08/29 05:08:14, xhwang wrote: > Can a malformed file generate a |key| without value? Or does av_dict_get enforce > this is always true? We shouldn't crash on malformed files. Done. https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:74: CHECK(!enc_key_id.empty()); On 2012/08/29 05:08:14, xhwang wrote: > Same as above, add error handling code instead of crashing? Done. https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:74: CHECK(!enc_key_id.empty()); On 2012/08/29 05:08:14, xhwang wrote: > Same as above, add error handling code instead of crashing? Done. https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/10829470/diff/13001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:50: typedef base::Callback<void(scoped_array<uint8>, int)> FFmpegNeedKeyCB; On 2012/09/01 16:46:41, ddorwin wrote: > On 2012/08/29 05:08:14, xhwang wrote: > > Move this typedef into FFmpegDemuxer and rename it to just NeedKeyCB? > > We're going to eliminate this and replace it with a common Demuxer class, right? > Add a TODO to do that and remove this? What should I do with this at this point? https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... File media/webm/webm_crypt_helpers.cc (right): https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:5: #include "media/webm/webm_crypt_helpers.h" On 2012/09/01 16:46:41, ddorwin wrote: > On 2012/08/29 05:08:14, xhwang wrote: > > How about rename this to "webm_crypto_helpers.h", to be consistent with > > "base/crypto" and "media/crypto"? > > Agreed. Is there a reason you chose "crypt"? Done. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:5: #include "media/webm/webm_crypt_helpers.h" On 2012/09/01 16:46:41, ddorwin wrote: > On 2012/08/29 05:08:14, xhwang wrote: > > How about rename this to "webm_crypto_helpers.h", to be consistent with > > "base/crypto" and "media/crypto"? > > Agreed. Is there a reason you chose "crypt"? Done. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:15: scoped_refptr<DecoderBuffer> WebMCopyBufferCheckIfEncrypted( On 2012/09/01 16:46:41, ddorwin wrote: > This function does not "check if encrypted". So, it won't work for unencrypted > WebM (or ISO). Right now, it's being called in all cases, which means anything > but encrypted WebM is broken. Renamed. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:19: data + kWebMHmacSize, data_size - kWebMHmacSize); On 2012/09/01 16:46:41, ddorwin wrote: > line breaks and alignment seem questionable. Removed. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:23: int data_offset = sizeof(signal_byte); On 2012/09/01 16:46:41, ddorwin wrote: > const int kDataOffset? > "data_offset" is not directly related to "data", which is confusing. Is there a > better name? Changed to frame_offset. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:31: uint64 network_iv; On 2012/09/01 16:46:41, ddorwin wrote: > Might comment "IV is only present for encrypted frames" somewhere. Done. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.cc:49: } // media On 2012/08/29 05:08:14, xhwang wrote: > // namespace media Done. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... File media/webm/webm_crypt_helpers.h (right): https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/01 16:46:41, ddorwin wrote: > Same as the other helper file - two files for one function. Are there others > that might end up in this file or can we merge the files? Done. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.h:21: // If encrypted, skip past the HMAC to get the buffer. Encrypted buffers On 2012/09/01 16:46:41, ddorwin wrote: > This probably needs to be updated to handle verified but unencrypted blocks. hmac removed. https://codereview.chromium.org/10829470/diff/13001/media/webm/webm_crypt_hel... media/webm/webm_crypt_helpers.h:29: } // webkit_media On 2012/09/01 16:46:41, ddorwin wrote: > media Done.
Thanks. https://codereview.chromium.org/10829470/diff/26001/media/ffmpeg/ffmpeg_common.h File media/ffmpeg/ffmpeg_common.h (right): https://codereview.chromium.org/10829470/diff/26001/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.h:75: void AVCodecContextToAudioDecoderConfig( Should this be local to the .cc file now? https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:74: if (audio_config_.is_encrypted() || video_config_.is_encrypted()) { Shouldn't we only check the one for this stream type? Maybe set a local variable in the switch statement at 52? https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:77: if (!key || !key->value) DCHECK too? https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:85: if (stream_->codec->codec_id == CODEC_ID_VORBIS) { I don't think codec matters. If we read enc_key_id, it's WebM. I think we can simplify this block to: encryption_key_id_.assign(enc_key_id); demuxer_->FireNeedKey(kWebMEncryptInitDataType, enc_key_id); https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:115: if (type() == DemuxerStream::AUDIO && audio_config_.is_encrypted() && I think we can collapse this to: if (type() == DemuxerStream::AUDIO && audio_config_.is_encrypted() || type() == DemuxerStream::VIDEO && video_config_.is_encrypted()) { buffer->SetDecryptConfig(create_decrtyp_config_cb_(...)); } https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:116: stream_->codec->codec_id == CODEC_ID_VORBIS) { Checking codecs doesn't scale. I think we may need to save the container type or a callback to create the DecryptConfig in the constructor. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:47: // First parameter - The type of encryption. s/encryption/initialization data/ https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:133: std::string audio_encryption_key_id_; A demuxer stream only supports audio or video, right? So we only need encryption_key_id_. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:156: // Calls |need_key_cb_| with the encryption key id of the decryption key // Calls |need_key_cb_| with the initialization data encountered in the file. ? https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:180: void NeedKeyCB(const std::string& type, Once we have the FFmpeg changes, should this be a MOCK_METHOD with new tests? https://codereview.chromium.org/10829470/diff/26001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: need_key_cb_.Run("", "", type, init_data.Pass(), init_data_size); ASSERT_TRUE(need_key_cb_); It's a test failure if it's called and not expected. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:23: CHECK(data_size >= kWebMSignalByteSize) Is this a programming or media error? https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:24: << "Got an encrypted block with not enough data " I assume this is included in the prod binary. Should probably be short. Something like "Invalid data size: " https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:34: if (signal_byte & kWebMFlagEncryptedFrame) { Should we DLOG_IF(unsupported bits set) somewhere? Is this the common location for all paths for processing WebM files? https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:36: CHECK(data_size >= kWebMSignalByteSize + kWebMIvSize) same https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.h (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:14: // TODO(xhwang): Figure out the init data type appropriately once it's spec'ed. You could link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=19096 https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:16: Should some of the existing Chunk Demuxer code be updated to use these? https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: std::string GenerateWebMCounterBlock(const uint8* iv, int iv_size); This is not currently used outside the .cc file. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_stream_pa... File media/webm/webm_stream_parser.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_stream_pa... media/webm/webm_stream_parser.cc:157: AVCodecContextToAudioDecoderConfig(stream->codec, false, &audio_config_); Just call AVStreamToAudioDecoderConfig like the video path below?
https://codereview.chromium.org/10829470/diff/26001/media/ffmpeg/ffmpeg_common.h File media/ffmpeg/ffmpeg_common.h (right): https://codereview.chromium.org/10829470/diff/26001/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.h:75: void AVCodecContextToAudioDecoderConfig( On 2013/03/10 21:29:39, ddorwin wrote: > Should this be local to the .cc file now? Done. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:74: if (audio_config_.is_encrypted() || video_config_.is_encrypted()) { On 2013/03/10 21:29:39, ddorwin wrote: > Shouldn't we only check the one for this stream type? Maybe set a local variable > in the switch statement at 52? Done. But I think having the two configs in demuxer stream is confusing. Should I add a todo about generalizing the configs? https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:77: if (!key || !key->value) On 2013/03/10 21:29:39, ddorwin wrote: > DCHECK too? Done. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:85: if (stream_->codec->codec_id == CODEC_ID_VORBIS) { On 2013/03/10 21:29:39, ddorwin wrote: > I don't think codec matters. If we read enc_key_id, it's WebM. > I think we can simplify this block to: > encryption_key_id_.assign(enc_key_id); > demuxer_->FireNeedKey(kWebMEncryptInitDataType, enc_key_id); The code I added to FFmpeg is not WebM specific. It will parse the EncKeyID element and base64 encoded that data into enc_key_id. I removed the checks as I was probably just being overly cautious. Done on the second part per the comment from the h file. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:115: if (type() == DemuxerStream::AUDIO && audio_config_.is_encrypted() && On 2013/03/10 21:29:39, ddorwin wrote: > I think we can collapse this to: > if (type() == DemuxerStream::AUDIO && audio_config_.is_encrypted() || > type() == DemuxerStream::VIDEO && video_config_.is_encrypted()) { > buffer->SetDecryptConfig(create_decrtyp_config_cb_(...)); > } Done. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:116: stream_->codec->codec_id == CODEC_ID_VORBIS) { On 2013/03/10 21:29:39, ddorwin wrote: > Checking codecs doesn't scale. I think we may need to save the container type or > a callback to create the DecryptConfig in the constructor. I removed the checks. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:47: // First parameter - The type of encryption. On 2013/03/10 21:29:39, ddorwin wrote: > s/encryption/initialization data/ Done. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:133: std::string audio_encryption_key_id_; On 2013/03/10 21:29:39, ddorwin wrote: > A demuxer stream only supports audio or video, right? So we only need > encryption_key_id_. Done. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.h:156: // Calls |need_key_cb_| with the encryption key id of the decryption key On 2013/03/10 21:29:39, ddorwin wrote: > // Calls |need_key_cb_| with the initialization data encountered in the file. > ? Done. I agree. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:180: void NeedKeyCB(const std::string& type, On 2013/03/10 21:29:39, ddorwin wrote: > Once we have the FFmpeg changes, should this be a MOCK_METHOD with new tests? I wanted to get opinions on this. I added an integration test that passes with the FFmpeg changes. https://codereview.chromium.org/10829470/diff/26001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: need_key_cb_.Run("", "", type, init_data.Pass(), init_data_size); On 2013/03/10 21:29:39, ddorwin wrote: > ASSERT_TRUE(need_key_cb_); > It's a test failure if it's called and not expected. Done. I also added to the media source test too. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:23: CHECK(data_size >= kWebMSignalByteSize) On 2013/03/10 21:29:39, ddorwin wrote: > Is this a programming or media error? Could be either I guess. But most likely a media error. I added a unittest to chunk_demuxer to test this code. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:24: << "Got an encrypted block with not enough data " On 2013/03/10 21:29:39, ddorwin wrote: > I assume this is included in the prod binary. Should probably be short. > Something like "Invalid data size: " I copied this from media source. Should I change it? https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:34: if (signal_byte & kWebMFlagEncryptedFrame) { On 2013/03/10 21:29:39, ddorwin wrote: > Should we DLOG_IF(unsupported bits set) somewhere? Is this the common location > for all paths for processing WebM files? I changed this up to return a NULL scoped_ptr so we could reuse this code with MediaSource. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:36: CHECK(data_size >= kWebMSignalByteSize + kWebMIvSize) On 2013/03/10 21:29:39, ddorwin wrote: > same ditto https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.h (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:14: // TODO(xhwang): Figure out the init data type appropriately once it's spec'ed. On 2013/03/10 21:29:39, ddorwin wrote: > You could link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=19096 Done. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:16: On 2013/03/10 21:29:39, ddorwin wrote: > Should some of the existing Chunk Demuxer code be updated to use these? Done. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: std::string GenerateWebMCounterBlock(const uint8* iv, int iv_size); On 2013/03/10 21:29:39, ddorwin wrote: > This is not currently used outside the .cc file. Done. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_stream_pa... File media/webm/webm_stream_parser.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_stream_pa... media/webm/webm_stream_parser.cc:157: AVCodecContextToAudioDecoderConfig(stream->codec, false, &audio_config_); On 2013/03/10 21:29:39, ddorwin wrote: > Just call AVStreamToAudioDecoderConfig like the video path below? Done.
Please update the description now that it's ready to land. The main line should probably be updated to "...specified by src". LG, but I think xhwang was going to take a look too. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:74: if (audio_config_.is_encrypted() || video_config_.is_encrypted()) { On 2013/03/12 00:42:42, fgalligan1 wrote: > On 2013/03/10 21:29:39, ddorwin wrote: > > Shouldn't we only check the one for this stream type? Maybe set a local > variable > > in the switch statement at 52? > Done. But I think having the two configs in demuxer stream is confusing. Should > I add a todo about generalizing the configs? Agreed. Not sure what the best solution is. We can talk later. https://codereview.chromium.org/10829470/diff/26001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:85: if (stream_->codec->codec_id == CODEC_ID_VORBIS) { On 2013/03/12 00:42:42, fgalligan1 wrote: > On 2013/03/10 21:29:39, ddorwin wrote: > > I don't think codec matters. If we read enc_key_id, it's WebM. > > I think we can simplify this block to: > > encryption_key_id_.assign(enc_key_id); > > demuxer_->FireNeedKey(kWebMEncryptInitDataType, enc_key_id); > > The code I added to FFmpeg is not WebM specific. It will parse the EncKeyID > element and base64 encoded that data into enc_key_id. I removed the checks as I > was probably just being overly cautious. Done on the second part per the comment > from the h file. > In what cases would we do the wrong thing? Matroska files using enc_key_id? Some other container that decides to use enc_key_id? For the first case, I think it's probably fine since the goal is to tell the app how to interpret the data. We don't really have a way to deterministically determine it here as far as I can tell. https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/10829470/diff/26001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.cc:24: << "Got an encrypted block with not enough data " On 2013/03/12 00:42:42, fgalligan1 wrote: > On 2013/03/10 21:29:39, ddorwin wrote: > > I assume this is included in the prod binary. Should probably be short. > > Something like "Invalid data size: " > I copied this from media source. Should I change it? It's only non-D CHECK strings that (might) remain in prod binary. https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: CHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; How can this happen? Should it really be a check (with string) in opt builds? https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: ASSERT_FALSE(need_key_cb_.is_null()); Doh, I was wrong. I don't think this failure gets passed up. I think this should be CHECK(!...) https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:70: const uint8 encrypted_frame[] = { kEncryptedFrame. This should be with other constants. It should also be static (as should constants above) or in an anonymous namespace. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:82: frame_size = sizeof(encrypted_frame); This seems unexpected. DCHECK or something if you can't handle it. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:469: EXPECT_EQ(cluster->size(), result); Can we check is_encrypted or anything?
https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: CHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; On 2013/03/12 04:40:06, ddorwin wrote: > How can this happen? Should it really be a check (with string) in opt builds? This could happen is the encrypted Block is malformed. E.g. A zero length frame (missing the signal byte). Or an encrypted frame that has length less than 9 bytes (A corrupted IV). I added 2 tests to check this code in chunk demuxer. Should I just let this type of error propagate and let the decode fail? https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: ASSERT_FALSE(need_key_cb_.is_null()); On 2013/03/12 04:40:06, ddorwin wrote: > Doh, I was wrong. I don't think this failure gets passed up. I think this should > be CHECK(!...) Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:70: const uint8 encrypted_frame[] = { On 2013/03/12 04:40:06, ddorwin wrote: > kEncryptedFrame. > This should be with other constants. > It should also be static (as should constants above) or in an anonymous > namespace. Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:82: frame_size = sizeof(encrypted_frame); On 2013/03/12 04:40:06, ddorwin wrote: > This seems unexpected. DCHECK or something if you can't handle it. Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:469: EXPECT_EQ(cluster->size(), result); On 2013/03/12 04:40:06, ddorwin wrote: > Can we check is_encrypted or anything? Done.
This looks pretty good. Just a few comments. The comments are made on patch set 5. https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:85: need_key_cb); Can we actually add a test with an encrypted file and make sure need_key_cb is fired? https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: ASSERT_FALSE(need_key_cb_.is_null()); Is the callback is null, the call (line 69) will segfault anyways. So probably we don't need to check it explicitly here. https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:203: PipelineIntegrationTestBase::CreateFilterCollection( This function really is CreateFilterCollectionWithFFmpegDemuxer(...). Also, it looks like we need some clean-up on these overloaded versions of CreateFilterCollection() and Start(). Nothing to do in this CL. Just a note to myself for future cleanup. https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:214: need_key_cb), decryptor); indentation. "decryptor" should be aligned with "new FFmpegDemuxer..." https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser.cc:293: return false; It's a bit strange to check after set. Can we do something like: config = WebMCreateDecryptConfig(...) if (!config) return false; buffer->SetDecryptConfig(config); https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:71: 0x01, // Block is encrypted nit: 2 spaces before // https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.h (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:17: const char kWebMEncryptInitDataType[] = "video/webm"; static? https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: // signal byte, and if the frame is encrypted an IV, prepended to the frame. ".. is encrypted, an IV is prepended to the frame" ? https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: // signal byte, and if the frame is encrypted an IV, prepended to the frame. s/IV/initialization vector/ https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:22: // Leaving the initialization vector empty will tell the decryptor that the s/initialization vector/IV/ https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:25: // http://wiki.webmproject.org/encryption/webm-encryption-rfc Returns NULL if the |data| is invalid?
Thanks. Replies in 5 and 6. https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: CHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; On 2013/03/12 17:20:13, fgalligan1 wrote: > On 2013/03/12 04:40:06, ddorwin wrote: > > How can this happen? Should it really be a check (with string) in opt builds? > This could happen is the encrypted Block is malformed. E.g. A zero length frame > (missing the signal byte). Or an encrypted frame that has length less than 9 > bytes (A corrupted IV). I added 2 tests to check this code in chunk demuxer. > Should I just let this type of error propagate and let the decode fail? I think so. If "user input" (media) can cause it, we should not crash. https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:68: ASSERT_FALSE(need_key_cb_.is_null()); On 2013/03/12 17:28:57, xhwang wrote: > Is the callback is null, the call (line 69) will segfault anyways. So probably > we don't need to check it explicitly here. Since this is a test, I think we want it to be easy to catch problems. I agree that we would likely not do this is prod code. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.h (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:17: const char kWebMEncryptInitDataType[] = "video/webm"; On 2013/03/12 17:28:57, xhwang wrote: > static? This is a header. The definition should probably be in the .cc, though. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:70: static const uint8 kEncryptedFrame[] = { Please move up with other constants. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:185: static bool VerifyEncryptedFrame( IsEncryptedFrame(). The verification is at the call site. Note: We may just eliminate this - see last comment. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:186: const WebMClusterParser::BufferQueue& video_buffers) { Since this just verifies one frame, it should only receive one frame, meaning 187 should occur at the call site. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:477: EXPECT_TRUE(VerifyEncryptedFrame(parser_->video_buffers())); The implementation of this really just checks the data that was parsed, right? I think this should be something like: ASSERT_EQ(1, parser_->video_buffers().size()); ASSERT_TRUE(parser_->video_buffers()[0]->GetDecryptConfig()); EXPECT_EQ(8?, parser_->video_buffers()[0]->GetDecryptConfig()->iv().length()); You could maybe put all that in a VerifyFrameIsEncrypted().
https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: CHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; On 2013/03/12 19:31:01, ddorwin wrote: > On 2013/03/12 17:20:13, fgalligan1 wrote: > > On 2013/03/12 04:40:06, ddorwin wrote: > > > How can this happen? Should it really be a check (with string) in opt > builds? > > This could happen is the encrypted Block is malformed. E.g. A zero length > frame > > (missing the signal byte). Or an encrypted frame that has length less than 9 > > bytes (A corrupted IV). I added 2 tests to check this code in chunk demuxer. > > Should I just let this type of error propagate and let the decode fail? > > I think so. If "user input" (media) can cause it, we should not crash. Changed to DCHECK. Good? https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:85: need_key_cb); On 2013/03/12 17:28:57, xhwang wrote: > Can we actually add a test with an encrypted file and make sure need_key_cb is > fired? Added a test. Please take a look. https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:214: need_key_cb), decryptor); On 2013/03/12 17:28:57, xhwang wrote: > indentation. "decryptor" should be aligned with "new FFmpegDemuxer..." Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser.cc:293: return false; On 2013/03/12 17:28:57, xhwang wrote: > It's a bit strange to check after set. Can we do something like: > config = WebMCreateDecryptConfig(...) > if (!config) > return false; > buffer->SetDecryptConfig(config); Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:71: 0x01, // Block is encrypted On 2013/03/12 17:28:57, xhwang wrote: > nit: 2 spaces before // Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... File media/webm/webm_crypto_helpers.h (right): https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:17: const char kWebMEncryptInitDataType[] = "video/webm"; On 2013/03/12 19:31:01, ddorwin wrote: > On 2013/03/12 17:28:57, xhwang wrote: > > static? > > This is a header. The definition should probably be in the .cc, though. I tried to extern and move the definition to the .cc file but I couldn't resolve it in the chunk_demuxer_unitest.cc https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: // signal byte, and if the frame is encrypted an IV, prepended to the frame. On 2013/03/12 17:28:57, xhwang wrote: > ".. is encrypted, an IV is prepended to the frame" ? Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:21: // signal byte, and if the frame is encrypted an IV, prepended to the frame. On 2013/03/12 17:28:57, xhwang wrote: > s/IV/initialization vector/ Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:22: // Leaving the initialization vector empty will tell the decryptor that the On 2013/03/12 17:28:57, xhwang wrote: > s/initialization vector/IV/ Done. https://codereview.chromium.org/10829470/diff/35001/media/webm/webm_crypto_he... media/webm/webm_crypto_helpers.h:25: // http://wiki.webmproject.org/encryption/webm-encryption-rfc On 2013/03/12 17:28:57, xhwang wrote: > Returns NULL if the |data| is invalid? Done. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:70: static const uint8 kEncryptedFrame[] = { On 2013/03/12 19:31:02, ddorwin wrote: > Please move up with other constants. Done. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:185: static bool VerifyEncryptedFrame( On 2013/03/12 19:31:02, ddorwin wrote: > IsEncryptedFrame(). The verification is at the call site. > Note: We may just eliminate this - see last comment. Done. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:186: const WebMClusterParser::BufferQueue& video_buffers) { On 2013/03/12 19:31:02, ddorwin wrote: > Since this just verifies one frame, it should only receive one frame, meaning > 187 should occur at the call site. Done. https://codereview.chromium.org/10829470/diff/52001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser_unittest.cc:477: EXPECT_TRUE(VerifyEncryptedFrame(parser_->video_buffers())); On 2013/03/12 19:31:02, ddorwin wrote: > The implementation of this really just checks the data that was parsed, right? > > I think this should be something like: > ASSERT_EQ(1, parser_->video_buffers().size()); > ASSERT_TRUE(parser_->video_buffers()[0]->GetDecryptConfig()); > EXPECT_EQ(8?, parser_->video_buffers()[0]->GetDecryptConfig()->iv().length()); > > You could maybe put all that in a VerifyFrameIsEncrypted(). Done. PTAL
LGTM, though I suggest using mocks for the new test. https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:181: void NeedKeyCB(const std::string& type, MOCK_METHOD... https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:302: EXPECT_EQ(2, need_key_called_); This is what mocks do well. Since this tests already uses mocks, I think we should use them for this.
lgtm % nits and agree on ddorwins' comments about using mock for the test. https://codereview.chromium.org/10829470/diff/70001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/10829470/diff/70001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser.cc:292: if (!config.get()) nit: you can just do "if (!config)"
https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:181: void NeedKeyCB(const std::string& type, On 2013/03/13 18:07:24, ddorwin wrote: > MOCK_METHOD... Done. https://codereview.chromium.org/10829470/diff/70001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:302: EXPECT_EQ(2, need_key_called_); On 2013/03/13 18:07:24, ddorwin wrote: > This is what mocks do well. Since this tests already uses mocks, I think we > should use them for this. Done. https://codereview.chromium.org/10829470/diff/70001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/10829470/diff/70001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser.cc:292: if (!config.get()) On 2013/03/13 18:57:27, xhwang wrote: > nit: you can just do "if (!config)" Done.
https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: DCHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; Continuing from Patch Set 5: It's probably better to LOG(ERROR) like line 108 if this can happen in user/media data. Since it might end up in the binary, we should display a more helpful message "Malfromed encrypted block" or whatever. https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:134: void NeedKeyCB(const std::string& type, Why can't we use the mock method directly? For the NotNull check? xhwang, any ideas to address that? fgalligan, please at least add a comment.
https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:123: DCHECK(buffer->GetDecryptConfig()) << "Create DecryptConfig failed."; On 2013/03/13 19:16:09, ddorwin wrote: > Continuing from Patch Set 5: > It's probably better to LOG(ERROR) like line 108 if this can happen in > user/media data. Since it might end up in the binary, we should display a more > helpful message "Malfromed encrypted block" or whatever. Added LOG(ERROR). But kept the text about DecryptConfig creation failed. There is debug logging in WebMCreateDecryptConfig() of why it failed. https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:134: void NeedKeyCB(const std::string& type, On 2013/03/13 19:16:09, ddorwin wrote: > Why can't we use the mock method directly? For the NotNull check? xhwang, any > ideas to address that? > fgalligan, please at least add a comment. I added xhwang's todo and comment on why we can't use the mock method directly.
lgtm Thanks!
lgtm https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/10829470/diff/83002/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:134: void NeedKeyCB(const std::string& type, On 2013/03/13 19:36:52, fgalligan1 wrote: > On 2013/03/13 19:16:09, ddorwin wrote: > > Why can't we use the mock method directly? For the NotNull check? xhwang, any > > ideas to address that? > > fgalligan, please at least add a comment. > > I added xhwang's todo and comment on why we can't use the mock method directly. Yeah, it's unfortunate that mock method doesn't work with move-only classes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10829470/120001
Message was sent while issue was closed.
Change committed as 188228 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
