Support encrypted audio stream in demuxer.

Support encrypted audio stream in demuxer.

BUG=123421
TEST=updated media_unittest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161465

[xhwang, 2012-10-10 00:24:43]

Pretty small change. PTAL!

[ddorwin, 2012-10-10 07:14:52]

lgtm % comments.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:204: std::string encryption_key_id;
const ref?

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:212: DVLOG(1) << "Unexpected track number " <<
track_num;
I know this was copied from below, but it seems like it should be a
DLOG(WARNING). It would be nice to see we are processing bad content.

[acolwell GONE FROM CHROMIUM, 2012-10-10 16:00:06]

Here are my initial comments. Also please add a test for this.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:212: DVLOG(1) << "Unexpected track number " <<
track_num;
On 2012/10/10 07:14:52, ddorwin wrote:
> I know this was copied from below, but it seems like it should be a
> DLOG(WARNING). It would be nice to see we are processing bad content.

I don't think this condition is any more important than the other DVLOG()
messages that report errors so I'd rather keep this consistent with the other
log messages. 

If there is a desire to have a wholesale change on this front then I think that
should be a different CL.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:244: data_offset += sizeof(iv);
nit:s/sizeof(iv)/sizeof(network_iv) since network_iv is really the one you are
extracting from the data buffer. It would also be nice to have this right under
the memcpy(). 

Also once you do this, you don't really need the iv variable anymore.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:252: video_encryption_key_id_,
s/video_encryption_key_id_/encryption_key_id ?

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
File media/webm/webm_cluster_parser_unittest.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser_unittest.cc:130: std::string(),
nit: use "" instead here and below.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser.cc
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser....
media/webm/webm_tracks_parser.cc:33: return
audio_content_encodings_client_->content_encodings()[0]->
If you only ever use the first encoding, why don't you just store the key_id
string instead of keeping the whole client around? Doing that would greatly
simplify this method since it would become a simple member accessor.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser....
media/webm/webm_tracks_parser.cc:42: return
video_content_encodings_client_->content_encodings()[0]->
ditto

[xhwang, 2012-10-11 00:49:41]

Resolved comments and changed more code to support audio in chunk demuxer. Also
updated ChunkDemuxerTest.

Sorry for rebase noise (if any)!

PTAL!

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:204: std::string encryption_key_id;
On 2012/10/10 07:14:52, ddorwin wrote:
> const ref?

Ref has to be initialized upon declaration. If we want to avoid this copy (of
16/32 bytes), I can do at line 215:

const std::string& encryption_key_id = track_num == audio_.track_num() ?
audio_encryption_key_id_ : video_encryption_key_id_;

which I feel isn't as clean as the current code. WDYT?

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:244: data_offset += sizeof(iv);
On 2012/10/10 16:00:06, acolwell wrote:
> nit:s/sizeof(iv)/sizeof(network_iv) since network_iv is really the one you are
> extracting from the data buffer. It would also be nice to have this right
under
> the memcpy(). 
> 
> Also once you do this, you don't really need the iv variable anymore.

Done.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:252: video_encryption_key_id_,
On 2012/10/10 16:00:06, acolwell wrote:
> s/video_encryption_key_id_/encryption_key_id ?

good catch. thanks!

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
File media/webm/webm_cluster_parser_unittest.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser_unittest.cc:130: std::string(),
On 2012/10/10 16:00:06, acolwell wrote:
> nit: use "" instead here and below.

Done.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser.cc
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser....
media/webm/webm_tracks_parser.cc:33: return
audio_content_encodings_client_->content_encodings()[0]->
On 2012/10/10 16:00:06, acolwell wrote:
> If you only ever use the first encoding, why don't you just store the key_id
> string instead of keeping the whole client around? Doing that would greatly
> simplify this method since it would become a simple member accessor.

Done.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_tracks_parser....
media/webm/webm_tracks_parser.cc:42: return
video_content_encodings_client_->content_encodings()[0]->
On 2012/10/10 16:00:06, acolwell wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2012-10-11 01:11:24]

LGTM % minor comments

http://codereview.chromium.org/11088047/diff/7001/media/base/audio_decoder_co...
File media/base/audio_decoder_config.h (right):

http://codereview.chromium.org/11088047/diff/7001/media/base/audio_decoder_co...
media/base/audio_decoder_config.h:88: // Note that in a potentially encrypted
video stream, individual buffers
s/video/audio/

http://codereview.chromium.org/11088047/diff/7001/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/11088047/diff/7001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:223: // TODO(xhwang): Simplify this!
Probably have test data files that contain
+1

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_stream_pars...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:378: FireNeedKey(need_key_cb_,
tracks_parser.audio_encryption_key_id());
nit: Why are you using a static function here instead of a member function?

[ddorwin, 2012-10-11 05:18:21]

LG. I'll let acolwell finish close out the review. I didn't review
chunk_demuxer_unittest.cc.

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/11088047/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:204: std::string encryption_key_id;
On 2012/10/11 00:49:41, xhwang wrote:
> On 2012/10/10 07:14:52, ddorwin wrote:
> > const ref?
> 
> Ref has to be initialized upon declaration. If we want to avoid this copy (of
> 16/32 bytes), I can do at line 215:
> 
> const std::string& encryption_key_id = track_num == audio_.track_num() ?
> audio_encryption_key_id_ : video_encryption_key_id_;
> 
> which I feel isn't as clean as the current code. WDYT?

Oh, duh. Nevermind.

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:74: if (track_type_ == kWebMTrackTypeVideo) {
Strange that video is processed first here. It seems to be the opposite order
most places and in the header file.

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:79:
content_encodings()[0]->encryption_key_id();
Why is it the first encoding? Maybe we should comment this.
I'm guessing because we only support one type of encoding. Is there a way to
DCHECK the type of the encoding?

[xhwang, 2012-10-11 16:36:10]

Resolved comments and nits. PTAL!

fgalligan@: please double check if the checks added in webm_tracks_parser.cc
makes sense or not as per the WebM encryption RFC.

Thanks!

http://codereview.chromium.org/11088047/diff/7001/media/base/audio_decoder_co...
File media/base/audio_decoder_config.h (right):

http://codereview.chromium.org/11088047/diff/7001/media/base/audio_decoder_co...
media/base/audio_decoder_config.h:88: // Note that in a potentially encrypted
video stream, individual buffers
On 2012/10/11 01:11:24, acolwell wrote:
> s/video/audio/

Done.

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_stream_pars...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_stream_pars...
media/webm/webm_stream_parser.cc:378: FireNeedKey(need_key_cb_,
tracks_parser.audio_encryption_key_id());
On 2012/10/11 01:11:24, acolwell wrote:
> nit: Why are you using a static function here instead of a member function? 

Done.

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:74: if (track_type_ == kWebMTrackTypeVideo) {
On 2012/10/11 05:18:22, ddorwin wrote:
> Strange that video is processed first here. It seems to be the opposite order
> most places and in the header file.

Done.

http://codereview.chromium.org/11088047/diff/7001/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:79:
content_encodings()[0]->encryption_key_id();
On 2012/10/11 05:18:22, ddorwin wrote:
> Why is it the first encoding? Maybe we should comment this.
> I'm guessing because we only support one type of encoding. Is there a way to
> DCHECK the type of the encoding?

Added more comments and more checks. fgalligan@: please double check if this is
correct as per WebM encryption RFC.

[acolwell GONE FROM CHROMIUM, 2012-10-11 16:50:33]

http://codereview.chromium.org/11088047/diff/10/media/webm/webm_tracks_parser.cc
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/11088047/diff/10/media/webm/webm_tracks_parser...
media/webm/webm_tracks_parser.cc:86: 1u);
This will cause files w/ multiple content encodings to crash in debug builds and
get accepted in release builds. This doesn't seem right. I think we should
either always allow them and simply add a DVLOG() to indicate that we are
picking the first one when multiple encodings exist or explicitly reject them as
invalid content.

[xhwang, 2012-10-11 17:12:31]

Comments resolved. Now we are back to the original behavior with comments added.
PTAL!

[acolwell GONE FROM CHROMIUM, 2012-10-11 17:36:35]

On 2012/10/11 17:12:31, xhwang wrote:
> Comments resolved. Now we are back to the original behavior with comments
added.
> PTAL!

looks good to me. ;)

[commit-bot: I haz the power, 2012-10-11 22:58:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11088047/12001

[commit-bot: I haz the power, 2012-10-12 01:04:35]

Change committed as 161465