Encrypted Media: Replace DecryptorClient with key event callbacks.

Encrypted Media: Replace DecryptorClient with key event callbacks.

- Removed DecryptorClient and use key event callbacks.
- Renamed Decryptor::KeyAddedCB to Decryptor::NewKeyCB.
- ProxyDecryptor implements key event callbacks and WebMediaPlayerProxy does not handle key events anymore.
- PpapiDecryptor implements key event callbacks as well so ContentDecryptorDelegate doesn't call ProxyDecryptor directly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174495

[xhwang, 2012-10-22 02:56:57]

Note: This is low priority. Don't review this if you have high priority task to
do :)

This is a prototype CL. Apparently it doesn't compile yet :)

Some points for discussion:

1) It's a little weird to have both key event callbacks (e.g. KeyErrorCB in
ProxyDecryptor) and DecryptorClient around. I can make WMPI implement
DecryptorClient directly, or remove DecryptorClient and have those key event
callbacks passed into AesDecryptor, PpapiDecryptor and PluginInstance directly.
The former seems to be abusing WMPI. The latter seems to be overkill to pass 4
callbacks to 3 classes. Compared to them, the current CL seems okay :) WDYT?

2) BindToLoop doesn't work with move-only types. Now in WMPI we have
OnDemuxerNeedKeyTrampoline to get around this issue. I can use this to make the
current CL work. But I have two question:
a) Can we make BindToLoop work w/ move-only types?
b) I feel the use of these move-only types has forced us to write many extra
code. For example, in the current implementation of both WebM and MP4 demuxer,
we create a new scoped_array for the init_data and do a memcpy so that we can
fire needkey. But we chose to use scoped_array to avoid memcpy in the first
place! I wonder again how big can the init data and key message be? Can we just
use std::string?

[Ami GONE FROM CHROMIUM, 2012-10-23 06:46:24]

> Note: This is low priority. Don't review this if you have high priority task
to do :)

This is a pretty good disclaimer for getting your CL completely ignored ;)
I haven't looked at the code in it yet but the discussion points caught my eye.

> Some points for discussion:
> 1) It's a little weird to have both key event callbacks (e.g. KeyErrorCB in
> ProxyDecryptor) and DecryptorClient around. I can make WMPI implement
> DecryptorClient directly, or remove DecryptorClient and have those key event
> callbacks passed into AesDecryptor, PpapiDecryptor and PluginInstance
directly.
> The former seems to be abusing WMPI. The latter seems to be overkill to pass 4
> callbacks to 3 classes. Compared to them, the current CL seems okay :) WDYT?

Personally I prefer CBs to Client interfaces and multiple inheritance.

> 2) BindToLoop doesn't work with move-only types. Now in WMPI we have
> OnDemuxerNeedKeyTrampoline to get around this issue. I can use this to make
the
> current CL work. But I have two question:
> a) Can we make BindToLoop work w/ move-only types?

Hah!  I fixed that ~2w ago but didn't realize it was hurting others, too:
https://codereview.chromium.org/11092054
You wanna review it for landing?

> b) I feel the use of these move-only types has forced us to write many extra
> code. For example, in the current implementation of both WebM and MP4 demuxer,
> we create a new scoped_array for the init_data and do a memcpy so that we can
> fire needkey. But we chose to use scoped_array to avoid memcpy in the first
> place! I wonder again how big can the init data and key message be? Can we
just
> use std::string?

Can you point out this code specifically?

[Ami GONE FROM CHROMIUM, 2012-10-23 06:52:21]

Hokay, now I've looked at the code, too :)

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

http://codereview.chromium.org/11226019/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:242: const OnKeyAddedCB& key_added_cb) {
I wish you hadn't done all this renaming in a prototype CL that's meant to
gather feedback on something else...

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

http://codereview.chromium.org/11226019/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:277: }
This collection of functions is a pretty strong smell that something's
suboptimal :)

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

http://codereview.chromium.org/11226019/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.h:50: int init_data_size)> NeedKeyCB;
ISTM this CL went half-way to the real goal: delete DecryptorClient and put
these typedefs in Decryptor.  Maybe that's what you meant in one of your
discussion points?
Anyway, I think that'd be best.

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.cc (left):

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.cc:111: void
WebMediaPlayerProxy::KeyAdded(const std::string& key_system,
HELL YES

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.h (left):

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:44: public media::DecryptorClient {
I <3 THIS SO HARD!

[xhwang, 2012-10-23 07:23:32]

On 2012/10/23 06:46:24, Ami Fischman wrote:
> > Note: This is low priority. Don't review this if you have high priority task
> to do :)
> 
> This is a pretty good disclaimer for getting your CL completely ignored ;)
> I haven't looked at the code in it yet but the discussion points caught my
eye.
> 
> > Some points for discussion:
> > 1) It's a little weird to have both key event callbacks (e.g. KeyErrorCB in
> > ProxyDecryptor) and DecryptorClient around. I can make WMPI implement
> > DecryptorClient directly, or remove DecryptorClient and have those key event
> > callbacks passed into AesDecryptor, PpapiDecryptor and PluginInstance
> directly.
> > The former seems to be abusing WMPI. The latter seems to be overkill to pass
4
> > callbacks to 3 classes. Compared to them, the current CL seems okay :) WDYT?
> 
> Personally I prefer CBs to Client interfaces and multiple inheritance.

Multiple inheritance is temporary. ProxyDecryptor will soon not be a Decrypor.
I'am okay with either. I'll see what ddorwin/scherkus has to say.
> 
> > 2) BindToLoop doesn't work with move-only types. Now in WMPI we have
> > OnDemuxerNeedKeyTrampoline to get around this issue. I can use this to make
> the
> > current CL work. But I have two question:
> > a) Can we make BindToLoop work w/ move-only types?
> 
> Hah!  I fixed that ~2w ago but didn't realize it was hurting others, too:
> https://codereview.chromium.org/11092054
> You wanna review it for landing?

Will take a look!
> 
> > b) I feel the use of these move-only types has forced us to write many extra
> > code. For example, in the current implementation of both WebM and MP4
demuxer,
> > we create a new scoped_array for the init_data and do a memcpy so that we
can
> > fire needkey. But we chose to use scoped_array to avoid memcpy in the first
> > place! I wonder again how big can the init data and key message be? Can we
> just
> > use std::string?
> 
> Can you point out this code specifically?

http://code.google.com/searchframe#OAMlx_jo-ck/src/media/webm/webm_stream_par...

http://code.google.com/searchframe#OAMlx_jo-ck/src/media/mp4/mp4_stream_parse...

[xhwang, 2012-10-23 07:32:13]

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

http://codereview.chromium.org/11226019/diff/1/media/crypto/aes_decryptor.cc#...
media/crypto/aes_decryptor.cc:242: const OnKeyAddedCB& key_added_cb) {
On 2012/10/23 06:52:22, Ami Fischman wrote:
> I wish you hadn't done all this renaming in a prototype CL that's meant to
> gather feedback on something else...

Sorry for the noise. I realized that I need to do this since Decryptor and
DecryptorClient both define KeyAddedCB, and ProxyDecryptor inherits both :) 
Next time I'll make a prototype CL really prototyping :)

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

http://codereview.chromium.org/11226019/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:277: }
On 2012/10/23 06:52:22, Ami Fischman wrote:
> This collection of functions is a pretty strong smell that something's
> suboptimal :)

If we remove DecryptorClient and use callbacks exclusively, should
ProxyDecryptor implements the callbacks and pass those to Aes/PpapiDecryptor to
avoid jumping-layers? e.g. to avoid AesDecryptor/PpapiDecryptor to call WMPI
directly?

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

http://codereview.chromium.org/11226019/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.h:50: int init_data_size)> NeedKeyCB;
On 2012/10/23 06:52:22, Ami Fischman wrote:
> ISTM this CL went half-way to the real goal: delete DecryptorClient and put
> these typedefs in Decryptor.  Maybe that's what you meant in one of your
> discussion points?
> Anyway, I think that'd be best.

Agreed that if we choose the pure callback path then these CBs should be defined
somewhere else (e.g. Decryptor).

[Ami GONE FROM CHROMIUM, 2012-10-23 07:33:32]

>
> > Can you point out this code specifically?

http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> media/webm/webm_stream_parser.**cc&exact_package=chromium&q=**
>
NeedKey&type=cs&l=476<http://code.google.com/searchframe#OAMlx_jo-ck/src/media/webm/webm_stream_parser.cc&exact_package=chromium&q=NeedKey&type=cs&l=476>
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> media/mp4/mp4_stream_parser.**cc&exact_package=chromium&q=**
>
NeedKey&type=cs&l=286<http://code.google.com/searchframe#OAMlx_jo-ck/src/media/mp4/mp4_stream_parser.cc&exact_package=chromium&q=NeedKey&type=cs&l=286>


Is there some inherent reasons the carriers of the data can't have a
release_foo() method that returns the scoped_array that'd allow avoiding
the copy?

[Ami GONE FROM CHROMIUM, 2012-10-23 07:42:54]

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

http://codereview.chromium.org/11226019/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:277: }
On 2012/10/23 07:32:13, xhwang wrote:
> On 2012/10/23 06:52:22, Ami Fischman wrote:
> > This collection of functions is a pretty strong smell that something's
> > suboptimal :)
> 
> If we remove DecryptorClient and use callbacks exclusively, should
> ProxyDecryptor implements the callbacks and pass those to Aes/PpapiDecryptor
to
> avoid jumping-layers? e.g. to avoid AesDecryptor/PpapiDecryptor to call WMPI
> directly? 

I /think/ so...
But it's late and I'm not as familiar with this code as I could be.

[xhwang, 2012-10-23 07:46:39]

On 2012/10/23 07:33:32, Ami Fischman wrote:
> >
> > > Can you point out this code specifically?
> 
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> > media/webm/webm_stream_parser.**cc&exact_package=chromium&q=**
> >
>
NeedKey&type=cs&l=476<http://code.google.com/searchframe#OAMlx_jo-ck/src/media/webm/webm_stream_parser.cc&exact_package=chromium&q=NeedKey&type=cs&l=476>
> > http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> > media/mp4/mp4_stream_parser.**cc&exact_package=chromium&q=**
> >
>
NeedKey&type=cs&l=286<http://code.google.com/searchframe#OAMlx_jo-ck/src/media/mp4/mp4_stream_parser.cc&exact_package=chromium&q=NeedKey&type=cs&l=286>
> 
> 
> Is there some inherent reasons the carriers of the data can't have a
> release_foo() method that returns the scoped_array that'd allow avoiding
> the copy?

For WebM, the initdata is a key_id, which is typically 16 or 32 bytes long. So
we don't really care to copy it. std::string was chosen so that we avoid a lot
of hassles of scoped_ptr.

For MP4, the internal type for init_data is std::vector<uint8>. I don't know
what's the reason but it seems std::vector<> is used in that file for most MP4
boxes.
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/mp4/box_definitions....

[Ami GONE FROM CHROMIUM, 2012-10-23 07:57:38]

Probably worth talking w/ strobe@ about the length of the actual expected
init_data segments here and whether the machinations of avoiding copies are
warranted.

[scherkus (not reviewing), 2012-10-23 21:10:39]

Seems like fischman@ has this review down.

I believe the original motivation for DecryptorClient was that there was a mix
of one-time and permanent callbacks. Even then I still tend to prefer callbacks
(even better: minimize the API surface / # of CBs!)

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.h (left):

http://codereview.chromium.org/11226019/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.h:44: public media::DecryptorClient {
On 2012/10/23 06:52:22, Ami Fischman wrote:
> I <3 THIS SO HARD!

+1

[xhwang, 2012-12-19 04:16:21]

Hi, after 1 month and 4 week, I am updating this CL again. As suggested by the
reviewers, I am removing the DecryptorClient and replace it with callbacks. This
CL is not complete yet. I still need work on the tests. Right now please ignore
all tests. Please review other files from a design perspective and ignore
details.

ddorwin: we are not firing NeedKey from decryptors right now. I guess current
implementation of applications are not expecting them either. Do you know which
direction we are going on this? I am thinking of not passing NeedKeyCB to
AesDecryptor and even PpapiDecrytpor.

xhwang

[Ami GONE FROM CHROMIUM, 2012-12-19 19:01:07]

On 2012/12/19 04:16:21, xhwang wrote:
> Please review other files from a design perspective and ignore details.

Done; looks fine.

[xhwang, 2012-12-20 00:06:01]

This CL is ready for a full review now. PTAL!

https://codereview.chromium.org/11226019/diff/15001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15001/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
Rename this since we are using KeyAddedCB for the new callback to fire KeyAdded
events (see line 222). I am open to suggestions for a better name. A lot of
changes in this CL is due to this renaming (which should really be in a separate
CL). Sorry for the noise.

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (left):

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decrypto...
media/crypto/aes_decryptor.h:32: // must be valid throughout the lifetime of the
AesDecryptor.
Not needed anymore since those callbacks are on weakptr.

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decrypto...
media/crypto/aes_decryptor.h:32: const NeedKeyCB& need_key_cb);
We are not firing NeedKey from AesDecryptor now (which is not spec compliant).
So we can potentially remove the 4th parameter.

ddorwin: what's the plan for firing NeedKey when the decryptor doesn't have a
key to decrypt? Or shall I fix AesDecryptor (and other decryptors) to make them
spec compliant?

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:34: class FakeEncryptedMedia {
moved from old code, renamed, and removed "virtual". no functionality change.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/ppapi...
File webkit/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/ppapi...
webkit/media/crypto/ppapi_decryptor.cc:260: void PpapiDecryptor::KeyAdded(const
std::string& key_system,
Now PpapiDecryptor is implementing these callbacks and no layer jumping
(ContentDecryptorDelegate calling WebMediaPlayerProxy callbacks directly)
anymore.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
File webkit/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.cc:168: base::Bind(&ProxyDecryptor::NeedKey,
weak_ptr_factory_.GetWeakPtr())));
This assumes NeedKey is always fired on the main thread (where ProxyDecryptor is
created). This may not be true if we decide to fire NeedKey in Decrypt() call in
AesDecryptor.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:179:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnKeyAdded),
key events are all fired on the main thread. Force post here so that we don't
reenter WMPI.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1093: const GURL
default_url_gurl(default_url);
This conversion was done in WMPP. Moved here since WMPP doesn't handle key
events anymore.

[scherkus (not reviewing), 2012-12-20 15:18:55]

lgtm w/ nits+q's

https://codereview.chromium.org/11226019/diff/15001/media/filters/decrypting_...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/11226019/diff/15001/media/filters/decrypting_...
media/filters/decrypting_video_decoder_unittest.cc:222: IsNullCallback()))
indent

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:51: // DecryptorClient
implementation.
class doesn't exist anymore

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:198: encrypted_media_->NeedKey("",
"", type, init_data.Pass(), init_data_size);
nit: this pointer could get replaced with a NeedKeyCB

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:179:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnKeyAdded),
On 2012/12/20 00:06:01, xhwang wrote:
> key events are all fired on the main thread. Force post here so that we don't
> reenter WMPI.

OOC is reentrancy in WMPI bad?

[xhwang, 2012-12-20 18:06:31]

scherkus's comments resolved.

https://codereview.chromium.org/11226019/diff/15001/media/filters/decrypting_...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/11226019/diff/15001/media/filters/decrypting_...
media/filters/decrypting_video_decoder_unittest.cc:222: IsNullCallback()))
On 2012/12/20 15:18:55, scherkus wrote:
> indent

Done.

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:51: // DecryptorClient
implementation.
On 2012/12/20 15:18:55, scherkus wrote:
> class doesn't exist anymore

Done.

https://codereview.chromium.org/11226019/diff/15001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:198: encrypted_media_->NeedKey("",
"", type, init_data.Pass(), init_data_size);
On 2012/12/20 15:18:55, scherkus wrote:
> nit: this pointer could get replaced with a NeedKeyCB

Done.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:179:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnKeyAdded),
On 2012/12/20 15:18:55, scherkus wrote:
> On 2012/12/20 00:06:01, xhwang wrote:
> > key events are all fired on the main thread. Force post here so that we
don't
> > reenter WMPI.
> 
> OOC is reentrancy in WMPI bad?

For this particular case, I don't see any obvious problem. But I'd like to avoid
reentrancy in general to avoid super long callstack. Not sure if I am
paranoid...

[Ami GONE FROM CHROMIUM, 2012-12-20 18:33:55]

I'll defer to scherkus@ for review of the details.
(unless someone wants me to do a real review now)

https://codereview.chromium.org/11226019/diff/11001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/11001/media/base/decryptor.h#ne...
media/base/decryptor.h:219: // Key events callbacks.
Point to
http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media...
?

[xhwang, 2012-12-20 19:27:51]

ddorwin: PTAL at the CL!

https://codereview.chromium.org/11226019/diff/11001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/11001/media/base/decryptor.h#ne...
media/base/decryptor.h:219: // Key events callbacks.
On 2012/12/20 18:33:55, Ami Fischman wrote:
> Point to
>
http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media...
> ?

Done.

[ddorwin, 2012-12-21 04:19:06]

Reviewed PS6, but 3 replies in PS3.

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decrypto...
media/crypto/aes_decryptor.h:32: const NeedKeyCB& need_key_cb);
On 2012/12/20 00:06:01, xhwang wrote:
> We are not firing NeedKey from AesDecryptor now (which is not spec compliant).
> So we can potentially remove the 4th parameter.
> 
> ddorwin: what's the plan for firing NeedKey when the decryptor doesn't have a
> key to decrypt? Or shall I fix AesDecryptor (and other decryptors) to make
them
> spec compliant?

TBD, but I don't think we'll make any changes to the v0.1 implementation. Do
whatever makes sense in the code.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
File webkit/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.cc:168: base::Bind(&ProxyDecryptor::NeedKey,
weak_ptr_factory_.GetWeakPtr())));
On 2012/12/20 00:06:01, xhwang wrote:
> This assumes NeedKey is always fired on the main thread (where ProxyDecryptor
is
> created). This may not be true if we decide to fire NeedKey in Decrypt() call
in
> AesDecryptor.

Can we DCHECK this assumption?

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:179:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnKeyAdded),
On 2012/12/20 18:06:31, xhwang wrote:
> On 2012/12/20 15:18:55, scherkus wrote:
> > On 2012/12/20 00:06:01, xhwang wrote:
> > > key events are all fired on the main thread. Force post here so that we
> don't
> > > reenter WMPI.
> > 
> > OOC is reentrancy in WMPI bad?
> 
> For this particular case, I don't see any obvious problem. But I'd like to
avoid
> reentrancy in general to avoid super long callstack. Not sure if I am
> paranoid...

I think we wan to avoid firing events in WK on the same call stack as a JS call.
For example, WK -> GKR ->KeyMessage -> WK.

P.S.
Is someone working on renaming this macro to "POST"? It always confuses me.

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:93: // Indicates that a new key has been added to the
Decryptor.
Is it really "new"? It could be called when we get an updated license for an
existing key id, right?

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
Should CB's have verb names? (I realize the ones from the spec below do not.)

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:221: //
http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media...
s/tip/eme-v0.1b/. These don't exist (in the same form) in the tip.

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:238: scoped_array<uint8> init_data,
Odd that message is a string but init_data isn't. I guess that's another cleanup
we should do.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:34: class FakeEncryptedMedia {
FakeEncryptedMediaSink or some similar noun?

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:61: NOTIMPLEMENTED();
Do we not expect this? Should it be:
FAIL() << "Unexpected Key Error";

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:72: current_key_system_ = key_system;
Do we really need this member var given line 68?

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:80: current_key_system_ = key_system;
Is this ever not clear key?

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:81: current_session_id_ = session_id;
Should we wipe out a session id if KeyMessage was already called for some
reason? I guess we're really using this as a local variable for limited
scenarios.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:87:
DCHECK(current_session_id_.empty());
Lots of DCHECKs here and below. Should these be EXPECT so they run in opt
builds?

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/ppapi...
File webkit/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/ppapi...
webkit/media/crypto/ppapi_decryptor.h:81: // Callbacks for firing key events.
Should we mention who calls these?

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.h:30: const media::KeyAddedCB& key_added_cb,
nit: Technically, these should come before the pointers.

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.h:77: // Callbacks for firing key events.
nit: I would then move these up above 73.

https://codereview.chromium.org/11226019/diff/15002/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:636: if
(key_added_cb_.is_null())
In what scenarios can this occur? Should there be a DCHECK, DLOG, return value,
or some other indication of failure?

[xhwang, 2012-12-21 05:50:06]

PTAL again!

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15001/media/crypto/aes_decrypto...
media/crypto/aes_decryptor.h:32: const NeedKeyCB& need_key_cb);
On 2012/12/21 04:19:06, ddorwin wrote:
> On 2012/12/20 00:06:01, xhwang wrote:
> > We are not firing NeedKey from AesDecryptor now (which is not spec
compliant).
> > So we can potentially remove the 4th parameter.
> > 
> > ddorwin: what's the plan for firing NeedKey when the decryptor doesn't have
a
> > key to decrypt? Or shall I fix AesDecryptor (and other decryptors) to make
> them
> > spec compliant?
> 
> TBD, but I don't think we'll make any changes to the v0.1 implementation. Do
> whatever makes sense in the code.

ok, I'll keep this as is then.

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
File webkit/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.cc:168: base::Bind(&ProxyDecryptor::NeedKey,
weak_ptr_factory_.GetWeakPtr())));
On 2012/12/21 04:19:06, ddorwin wrote:
> On 2012/12/20 00:06:01, xhwang wrote:
> > This assumes NeedKey is always fired on the main thread (where
ProxyDecryptor
> is
> > created). This may not be true if we decide to fire NeedKey in Decrypt()
call
> in
> > AesDecryptor.
> 
> Can we DCHECK this assumption?

If this assumption is violated, weak_ptr_factory_.GetWeakPtr() will fire (it has
internal checks).

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11226019/diff/15001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:179:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnKeyAdded),
On 2012/12/21 04:19:06, ddorwin wrote:
> On 2012/12/20 18:06:31, xhwang wrote:
> > On 2012/12/20 15:18:55, scherkus wrote:
> > > On 2012/12/20 00:06:01, xhwang wrote:
> > > > key events are all fired on the main thread. Force post here so that we
> > don't
> > > > reenter WMPI.
> > > 
> > > OOC is reentrancy in WMPI bad?
> > 
> > For this particular case, I don't see any obvious problem. But I'd like to
> avoid
> > reentrancy in general to avoid super long callstack. Not sure if I am
> > paranoid...
> 
> I think we wan to avoid firing events in WK on the same call stack as a JS
call.
> For example, WK -> GKR ->KeyMessage -> WK.
> 
> P.S.
> Is someone working on renaming this macro to "POST"? It always confuses me.

bug filed, let's discuss and get consensus first:
http://code.google.com/p/chromium/issues/detail?id=167240

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:93: // Indicates that a new key has been added to the
Decryptor.
On 2012/12/21 04:19:06, ddorwin wrote:
> Is it really "new"? It could be called when we get an updated license for an
> existing key id, right?

I added "new" here only to make is consistent with the callback name. In some
sense, an updated license is a new license... WDYT?

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
On 2012/12/21 04:19:06, ddorwin wrote:
> Should CB's have verb names? (I realize the ones from the spec below do not.)

At least this is not a rule in media code. We hace PipelineStatusCB,
BufferingStateCB, etc.

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:221: //
http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media...
On 2012/12/21 04:19:06, ddorwin wrote:
> s/tip/eme-v0.1b/. These don't exist (in the same form) in the tip.

Done.

https://codereview.chromium.org/11226019/diff/15002/media/base/decryptor.h#ne...
media/base/decryptor.h:238: scoped_array<uint8> init_data,
On 2012/12/21 04:19:06, ddorwin wrote:
> Odd that message is a string but init_data isn't. I guess that's another
cleanup
> we should do.

Agreed. I talked to dmichael the other day. We all agreed that vector<unit8>
should be best for binary arrays. But we have had too many uses of std::string
for binary arrays (following crypto's convention), sigh...

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:34: class FakeEncryptedMedia {
On 2012/12/21 04:19:06, ddorwin wrote:
> FakeEncryptedMediaSink or some similar noun?

The current name is consistent with MockMediaSource (feel we should rename it to
FakeMediaSource though).

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:61: NOTIMPLEMENTED();
On 2012/12/21 04:19:06, ddorwin wrote:
> Do we not expect this? Should it be:
> FAIL() << "Unexpected Key Error";

Done.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:72: current_key_system_ = key_system;
On 2012/12/21 04:19:06, ddorwin wrote:
> Do we really need this member var given line 68?

This could be triggered by line 88, where the current_key_system_ is empty().

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:80: current_key_system_ = key_system;
On 2012/12/21 04:19:06, ddorwin wrote:
> Is this ever not clear key?

The demuxer doesn't know about key system, so |key_system| could be empty.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:81: current_session_id_ = session_id;
On 2012/12/21 04:19:06, ddorwin wrote:
> Should we wipe out a session id if KeyMessage was already called for some
> reason? I guess we're really using this as a local variable for limited
> scenarios.

Similar to key_system, session_id may not be set in NeedKey call. We need
GenerateKeyRequest call (see line 88) to trigger a KeyMessage where we can set
the session id (see line 73).

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:87:
DCHECK(current_session_id_.empty());
On 2012/12/21 04:19:06, ddorwin wrote:
> Lots of DCHECKs here and below. Should these be EXPECT so they run in opt
> builds?

Replace this DCHECK with EXPECT_TRUE. If we need to replace other DCHECKs, we
may do that in another CL.

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/ppapi...
File webkit/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/ppapi...
webkit/media/crypto/ppapi_decryptor.h:81: // Callbacks for firing key events.
On 2012/12/21 04:19:06, ddorwin wrote:
> Should we mention who calls these?

Done.

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.h:30: const media::KeyAddedCB& key_added_cb,
On 2012/12/21 04:19:06, ddorwin wrote:
> nit: Technically, these should come before the pointers.

I thought about that.. The rule is input before output; use ref for input and
use pointer for output. But these two pointers are not really output, they look
more like input to me. Probably we should pass them as ref, but they can't be
const-ref... On the other hand, even though the callbacks are const-ref, they
are really for passing some outputs (e.g. ReadCB, DecryptCB). I feel this is why
we have a lot of functions in media that put callbacks after input parameters.
WDYT?

https://codereview.chromium.org/11226019/diff/15002/webkit/media/crypto/proxy...
webkit/media/crypto/proxy_decryptor.h:77: // Callbacks for firing key events.
On 2012/12/21 04:19:06, ddorwin wrote:
> nit: I would then move these up above 73.

see above...

https://codereview.chromium.org/11226019/diff/15002/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11226019/diff/15002/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:636: if
(key_added_cb_.is_null())
On 2012/12/21 04:19:06, ddorwin wrote:
> In what scenarios can this occur? Should there be a DCHECK, DLOG, return
value,
> or some other indication of failure?

hmm, I guess this comes from my paranoid about not trusting the plugin/CDM. This
is to guard the case where the plugin/CDM fires key events before we
SetKeyEventCallbacks(). For example, if some other people choose to implement
another plugin that calls the PPB interface before GKR is called...

[scherkus (not reviewing), 2012-12-21 15:21:24]

still lgtm

[dmichael (off chromium), 2012-12-21 17:33:57]

webkit/plugins/ppapi lgtm

[ddorwin, 2012-12-21 20:05:30]

LGTM. We can fix the name later in a smaller CL.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:72: current_key_system_ = key_system;
On 2012/12/21 05:50:07, xhwang wrote:
> On 2012/12/21 04:19:06, ddorwin wrote:
> > Do we really need this member var given line 68?
> 
> This could be triggered by line 88, where the current_key_system_ is empty().

I guess we are using current_key_system_ as a bool for whether we have requested
a request. Maybe that's the problem.

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:81: current_session_id_ = session_id;
On 2012/12/21 05:50:07, xhwang wrote:
> On 2012/12/21 04:19:06, ddorwin wrote:
> > Should we wipe out a session id if KeyMessage was already called for some
> > reason? I guess we're really using this as a local variable for limited
> > scenarios.
> 
> Similar to key_system, session_id may not be set in NeedKey call. We need
> GenerateKeyRequest call (see line 88) to trigger a KeyMessage where we can set
> the session id (see line 73).

My point was that this line _might_ replace the session_id set by the key
message, possibly with an empty string. This isn't really a problem since we
probably only deal with one session, but that's not called out and the code is
incorrect for the general case.

https://codereview.chromium.org/11226019/diff/29002/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/29002/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
I think it's not a new key. :)
My question really was about the new name, not the comment.

[xhwang, 2012-12-22 00:46:33]

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11226019/diff/15002/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:81: current_session_id_ = session_id;
On 2012/12/21 20:05:30, ddorwin wrote:
> On 2012/12/21 05:50:07, xhwang wrote:
> > On 2012/12/21 04:19:06, ddorwin wrote:
> > > Should we wipe out a session id if KeyMessage was already called for some
> > > reason? I guess we're really using this as a local variable for limited
> > > scenarios.
> > 
> > Similar to key_system, session_id may not be set in NeedKey call. We need
> > GenerateKeyRequest call (see line 88) to trigger a KeyMessage where we can
set
> > the session id (see line 73).
> 
> My point was that this line _might_ replace the session_id set by the key
> message, possibly with an empty string. This isn't really a problem since we
> probably only deal with one session, but that's not called out and the code is
> incorrect for the general case.

Hmm, I'll think about this more and change it to a more acceptable way. But that
will be in a separate CL.

https://codereview.chromium.org/11226019/diff/29002/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/29002/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
On 2012/12/21 20:05:30, ddorwin wrote:
> I think it's not a new key. :)
> My question really was about the new name, not the comment.

I agree this name needs to be improved. But I don't have a better name for now.
Let's do that in a later CL.

[commit-bot: I haz the power, 2012-12-22 01:00:10]

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

[commit-bot: I haz the power, 2012-12-22 01:04:19]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-22 01:22:47]

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

[tfarina, 2013-01-26 16:05:23]

https://codereview.chromium.org/11226019/diff/25007/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/25007/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
base::Closure?

[xhwang, 2013-01-30 04:25:07]

https://codereview.chromium.org/11226019/diff/25007/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/11226019/diff/25007/media/base/decryptor.h#ne...
media/base/decryptor.h:94: typedef base::Callback<void()> NewKeyCB;
On 2013/01/26 16:05:23, tfarina wrote:
> base::Closure?

You are right. Thanks for pointing this out. I'll update it.