Add "type" in GenerateKeyRequest() and OnNeedKey().

Add "type" in GenerateKeyRequest() and OnNeedKey().

The "type" information could help the CDM to parse the initialization data correctly.

See for details: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19096

TBR=viettrungluu@chromium.org
BUG=none
TEST=none

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

[xhwang, 2012-10-26 22:59:14]

This is the brother CL of Tom's cl 11270057.

Some TODOs are added which needs to be resolved after I rebase this onto Tom's
(or vice versa).

PTAL!

[ddorwin, 2012-10-27 00:08:26]

LG. Just minor comments.

http://codereview.chromium.org/11313016/diff/1/media/base/stream_parser.h
File media/base/stream_parser.h (right):

http://codereview.chromium.org/11313016/diff/1/media/base/stream_parser.h#new...
media/base/stream_parser.h:62: // First parameter - The type of the
initialization data associated with the
Not exact, but close enough until we get more clarity in the spec.

http://codereview.chromium.org/11313016/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:137: const std::string& type,
Comment somewhere that ClearKey/AesDecryptor does not care and just returns it
in the message.

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser.cc
File media/mp4/mp4_stream_parser.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:23: static const char kMp4InitDataType[] =
"video/mp4";
// TODO: Figure this out appropriately once spec'd.

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser_un...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser_un...
media/mp4/mp4_stream_parser_unittest.cc:81: DVLOG(1) << "KeyNeededF: type=" <<
type
Shouldn't we just EXPECT_STREQ the type?

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

http://codereview.chromium.org/11313016/diff/1/media/webm/webm_stream_parser....
media/webm/webm_stream_parser.cc:22: static const char kWebMInitDataType[] =
"video/webm";
same

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/ppapi/clea...
File webkit/media/crypto/ppapi/clear_key_cdm.h (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/ppapi/clea...
webkit/media/crypto/ppapi/clear_key_cdm.h:48: // TODO(xhwang): Hook up "type".
Are you or Tom landing first? :)

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/proxy_decr...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:61: client->NeedKey("", "", "",
key_id_array.Pass(), key_id.size());
This is the decryptor (not demuxer) needkey, right?

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:806: init_data_type_,
This only works if needkey has been called. Ohterwise it will be an empty
string. I guess that's fine.
A better interim solution would be to figure out which stream parser was used
(even if needkey was never called).
This is fine for now, but we should see if we can do that.

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.h:368: std::string init_data_type_;
// Temporary for EME v0.1.
Or something like that.

[xhwang, 2012-10-27 00:50:14]

comments resolved, PTAL again.

http://codereview.chromium.org/11313016/diff/1/media/base/stream_parser.h
File media/base/stream_parser.h (right):

http://codereview.chromium.org/11313016/diff/1/media/base/stream_parser.h#new...
media/base/stream_parser.h:62: // First parameter - The type of the
initialization data associated with the
On 2012/10/27 00:08:26, ddorwin wrote:
> Not exact, but close enough until we get more clarity in the spec.

:) I choose vague language on purpose.

http://codereview.chromium.org/11313016/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:137: const std::string& type,
On 2012/10/27 00:08:26, ddorwin wrote:
> Comment somewhere that ClearKey/AesDecryptor does not care and just returns it
> in the message.

Done.

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser.cc
File media/mp4/mp4_stream_parser.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:23: static const char kMp4InitDataType[] =
"video/mp4";
On 2012/10/27 00:08:26, ddorwin wrote:
> // TODO: Figure this out appropriately once spec'd.

Done.

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser_un...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/1/media/mp4/mp4_stream_parser_un...
media/mp4/mp4_stream_parser_unittest.cc:81: DVLOG(1) << "KeyNeededF: type=" <<
type
On 2012/10/27 00:08:26, ddorwin wrote:
> Shouldn't we just EXPECT_STREQ the type?

Done.

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

http://codereview.chromium.org/11313016/diff/1/media/webm/webm_stream_parser....
media/webm/webm_stream_parser.cc:22: static const char kWebMInitDataType[] =
"video/webm";
On 2012/10/27 00:08:26, ddorwin wrote:
> same

Done.

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/ppapi/clea...
File webkit/media/crypto/ppapi/clear_key_cdm.h (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/ppapi/clea...
webkit/media/crypto/ppapi/clear_key_cdm.h:48: // TODO(xhwang): Hook up "type".
On 2012/10/27 00:08:26, ddorwin wrote:
> Are you or Tom landing first? :)

Not sure. Updated comment in case my CL lands first.

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/proxy_decr...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:61: client->NeedKey("", "", "",
key_id_array.Pass(), key_id.size());
On 2012/10/27 00:08:26, ddorwin wrote:
> This is the decryptor (not demuxer) needkey, right?

Yes. In the future we should let the decryptors initialize NeedKey directly
instead letting ProxyDecryptor do that.

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:806: init_data_type_,
On 2012/10/27 00:08:26, ddorwin wrote:
> This only works if needkey has been called. Ohterwise it will be an empty
> string. I guess that's fine.
> A better interim solution would be to figure out which stream parser was used
> (even if needkey was never called).
> This is fine for now, but we should see if we can do that.

This is by design. If the application calls GKR w/o waiting for the needkey to
be fired, it's assuming the CDM knows how to parse the init data. Anyway, I
don't think it's worth passing the demuxer stream info out to WMPI only to
support this use case, especially because this is only a temp hack.

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/11313016/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.h:368: std::string init_data_type_;
On 2012/10/27 00:08:26, ddorwin wrote:
> // Temporary for EME v0.1.
> Or something like that.

Done.

[ddorwin, 2012-10-27 01:47:17]

lgtm % nits. Thanks.

http://codereview.chromium.org/11313016/diff/12003/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/crypto/aes_decryptor...
media/crypto/aes_decryptor.cc:142: // For now, the AesDecryptor does not care
about |key_system| and |type| and
nit: Replace the last "and" with a period or semicolon.

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:84: EXPECT_EQ(type, kMp4InitDataType);
swap params

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:85: DVLOG(1) << "KeyNeededF: " <<
init_data_size;
Any idea why we log but don't check this?

http://codereview.chromium.org/11313016/diff/12003/media/webm/webm_stream_par...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/webm/webm_stream_par...
media/webm/webm_stream_parser.cc:480: need_key_cb_.Run(kWebMInitDataType,
key_id_array.Pass(), key_id_size);
Is there no test for the WebM parser like there is for MP4? Or do we just have
the not empty check in chunk_demuxer_unittest.cc? Is webm_parser_unittest.cc –
the equivalent of the mp4 test?

[xhwang, 2012-10-27 03:22:59]

http://codereview.chromium.org/11313016/diff/12003/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/crypto/aes_decryptor...
media/crypto/aes_decryptor.cc:142: // For now, the AesDecryptor does not care
about |key_system| and |type| and
On 2012/10/27 01:47:17, ddorwin wrote:
> nit: Replace the last "and" with a period or semicolon.

Done.

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:84: EXPECT_EQ(type, kMp4InitDataType);
On 2012/10/27 01:47:17, ddorwin wrote:
> swap params

Done.

http://codereview.chromium.org/11313016/diff/12003/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:85: DVLOG(1) << "KeyNeededF: " <<
init_data_size;
On 2012/10/27 01:47:17, ddorwin wrote:
> Any idea why we log but don't check this?

Done.

http://codereview.chromium.org/11313016/diff/12003/media/webm/webm_stream_par...
File media/webm/webm_stream_parser.cc (right):

http://codereview.chromium.org/11313016/diff/12003/media/webm/webm_stream_par...
media/webm/webm_stream_parser.cc:480: need_key_cb_.Run(kWebMInitDataType,
key_id_array.Pass(), key_id_size);
On 2012/10/27 01:47:17, ddorwin wrote:
> Is there no test for the WebM parser like there is for MP4? Or do we just have
> the not empty check in chunk_demuxer_unittest.cc? Is webm_parser_unittest.cc –
> the equivalent of the mp4 test?

It's pretty much tested in chunk_demuxer_unittests where we actually use webm
for testing (for historical reasons). Added check for "video/webm" there.

[ddorwin, 2012-10-27 03:26:00]

lgtm

http://codereview.chromium.org/11313016/diff/10034/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/10034/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:60: << ", dur=" <<
duration.InMilliseconds();
nit: space replacement, but still off

[Tom Finegan, 2012-10-27 05:13:29]

lgtm

I'll rebase http://codereview.chromium.org/11270057 on this CL.

[commit-bot: I haz the power, 2012-10-27 05:31:48]

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

[xhwang, 2012-10-27 05:32:51]

http://codereview.chromium.org/11313016/diff/10034/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser_unittest.cc (right):

http://codereview.chromium.org/11313016/diff/10034/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser_unittest.cc:60: << ", dur=" <<
duration.InMilliseconds();
On 2012/10/27 03:26:00, ddorwin wrote:
> nit: space replacement, but still off

On my local linux bot this looks aligned (no tab). I am so confused. CQ anyway.

[commit-bot: I haz the power, 2012-10-27 07:23:15]

Change committed as 164522