Implement "Key Presence" step in "Encrypted Block Encounted" algorithm in EME.

Implement "Key Presence" step in "Encrypted Block Encounted" algorithm in EME.

See related spec here: http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media.html#algorithms-enrypted-block

If the AesDecryptor cannot find the decryption for the encrypted buffer, it returns a kNoKey.

In ProxyDecryptor, if the input cannot be decrypted due to kNoKey, the ProxyDecryptor will cache the encrypted buffers and callbacks and fire a needkey event. When some key is added (AddKey called) later, it will try to decrypt these buffers again. The callbacks will be fired on the same thread where Decrypt() was called originally.

BUG=125401, 125753
TEST=media_unittest, media layout_tests

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

[xhwang, 2012-07-25 22:40:02]

Hello scherkus and ddorwin,

This CL replaces the original "kTryAgain" CL to implement the "resume playback
after key added" logic ("Key Presence" step in "Encrypted Block Encounted"
algorithm in EME spec).

My local demo is working. I'll work on unittests shortly. I'd like to upload
this CL first in case I am in the wrong direction.

PTAL!

xhwang

[scherkus (not reviewing), 2012-07-26 00:13:50]

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

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:111: std::swap(pending_decrypt_closures_,
closures_to_run);
so if I queue a bunch of closures requiring key A but I call add key with key B
we're going to fire off all of those closures again?

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:123: DVLOG(1) << "CancelKeyRequest()";
do we need to commit these?

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:142: if (!decryptor) {
when would this happen?

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:158: base::Unretained(this),
MessageLoop::current(), encrypted, decrypt_cb));
this should be MessageLoopProxy

[xhwang, 2012-07-26 00:53:00]

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

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:111: std::swap(pending_decrypt_closures_,
closures_to_run);
On 2012/07/26 00:13:50, scherkus wrote:
> so if I queue a bunch of closures requiring key A but I call add key with key
B
> we're going to fire off all of those closures again?

This is a feature not a bug. According to ddorwin@, we'll remove key_id or
init_data in AddKey. So the only way to tell if some key added here applies to
some cached buffer is to call decryptor_->Decrypt(). Fortunately this should
only happen during initialization phase, and we will only have one buffer cached
per stream.

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:123: DVLOG(1) << "CancelKeyRequest()";
On 2012/07/26 00:13:50, scherkus wrote:
> do we need to commit these?

I am trying to add logs to calls from WebKit to help debugging. Let me know if
it's bad.

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:142: if (!decryptor) {
On 2012/07/26 00:13:50, scherkus wrote:
> when would this happen?

This could happen when the encrypted media file is loaded but GenerateKeyRequest
has not been called. In this case the decryptor will hold the decrypt_cb and
wait for GKR() and AddKey() to be called then resume callback.

http://codereview.chromium.org/10822026/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:158: base::Unretained(this),
MessageLoop::current(), encrypted, decrypt_cb));
On 2012/07/26 00:13:50, scherkus wrote:
> this should be MessageLoopProxy

Done.

[scherkus (not reviewing), 2012-07-26 02:06:38]

based on our discussion I forget -- where you planning on writing tests in this
CL?

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:88: DCHECK(client_);
dcheck not needed

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.h:92: base::Lock
pending_decrypt_closures_lock_;
do you need separate locks?

[xhwang, 2012-07-26 22:16:03]

Hello scherkus and ddorwin,

I added unit tests too. PTAL!

xhwang

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:88: DCHECK(client_);
On 2012/07/26 02:06:38, scherkus wrote:
> dcheck not needed

Done.

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.h:92: base::Lock
pending_decrypt_closures_lock_;
On 2012/07/26 02:06:38, scherkus wrote:
> do you need separate locks?

I was trying to minimize the time of lock blocking. One lock will work for sure
and is actually simpler. Am I doing pre-mature optimization?

[scherkus (not reviewing), 2012-07-28 23:07:19]

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/4001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.h:92: base::Lock
pending_decrypt_closures_lock_;
On 2012/07/26 22:16:04, xhwang wrote:
> On 2012/07/26 02:06:38, scherkus wrote:
> > do you need separate locks?
> 
> I was trying to minimize the time of lock blocking. One lock will work for
sure
> and is actually simpler. Am I doing pre-mature optimization?

Yes.

There are three situations where a lock is required:
  1) GKR()
  2) AddKey()
  3) Decrypt()

1 and 2 are one-off operations as opposed to 3 which is a more frequent
operation. I'm doubtful you'll ever see so much contention between these 3
operations that it necessitates an extra lock.

As you note having a single lock-per-object is also simpler and easier for
others to comprehend.

http://codereview.chromium.org/10822026/diff/14001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10822026/diff/14001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:8: #include <deque>
were these changes needed due to including m_l_p instead of m_l.h?

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:146:
pending_decrypt_closures_.push_back(base::Bind(
to refresh my memory -- what's our long term plan to get rid of the pending
decrypt stuff here?

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:41: // This is only for testing purpose.
instead of a comment + setter that looks like it's not for testing, how about:
  1) Drop the comment
  2) Change fn to set_decryptor_for_testing()

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:97: base::Lock
pending_decrypt_closures_lock_;
see my other comment -- we don't need this

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:107: };
DISALLLOW etc

[scherkus (not reviewing), 2012-07-28 23:08:03]

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:197: StopMessageLoop();
this method name is misleading as it only posts a quit closure but doesn't
actually stop the message loop

[xhwang, 2012-07-30 19:58:09]

Resolved most comments from scherkus@. PTAL!

Sorry, I accidentally rebased :/. Hopefully this will not make the reviewing too
hard.

xhwang

http://codereview.chromium.org/10822026/diff/14001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10822026/diff/14001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:8: #include <deque>
On 2012/07/28 23:07:19, scherkus wrote:
> were these changes needed due to including m_l_p instead of m_l.h?

No, it's because of the "include-what-you-use" rule: deque is used on line 167
and swap is used on line 269.

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:146:
pending_decrypt_closures_.push_back(base::Bind(
On 2012/07/28 23:07:19, scherkus wrote:
> to refresh my memory -- what's our long term plan to get rid of the pending
> decrypt stuff here?

Pending decrypt stuff here will stay. It's part of the EME spec and this is the
best solution/implementation of that so far.

In the future, we'll call Decrypt() directly from the VideoRendererBase instead
of from the VideoDecoder. But that is orthogonal to this change.

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:41: // This is only for testing purpose.
On 2012/07/28 23:07:19, scherkus wrote:
> instead of a comment + setter that looks like it's not for testing, how about:
>   1) Drop the comment
>   2) Change fn to set_decryptor_for_testing()

Done.

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:97: base::Lock
pending_decrypt_closures_lock_;
On 2012/07/28 23:07:19, scherkus wrote:
> see my other comment -- we don't need this

Done.

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:107: };
On 2012/07/28 23:07:19, scherkus wrote:
> DISALLLOW etc

Done.

http://codereview.chromium.org/10822026/diff/14001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:197: StopMessageLoop();
On 2012/07/28 23:08:03, scherkus wrote:
> this method name is misleading as it only posts a quit closure but doesn't
> actually stop the message loop

Renamed to ScheduleMessageLoopToStop().

[xhwang, 2012-07-30 20:48:25]

+acolwell for changes in media/filters/chunk*, PTAL!

http://codereview.chromium.org/10822026/diff/14004/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/14004/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:673: MessageLoop message_loop_;
In the original test, since we don't have a message loop, MessageLoop::Current()
returns NULL. The test still works because it compares pointers directly (i.e.
if (NULL != NULL) ...):
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/chunk_demuxe...

But now we switch to MessageLoopProxy, this is caught by an assertion:
T* scoped_refptr<T>::operator->() const [with T = base::MessageLoopProxy]:
Assertion `ptr_ != __null' failed.

It makes sense for me to create a MessageLoop here so that we don't rely on the
"if (NULL != NULL)" logic. With this, all tests pass now.

[xhwang, 2012-07-30 22:50:01]

FYI, I still need to figure out how to make the tear down process working
correctly, e.g. the ProxyDecryptor should fire all pending DecryptCBs before
destruction.

[xhwang, 2012-07-31 16:11:14]

On 2012/07/30 22:50:01, xhwang wrote:
> FYI, I still need to figure out how to make the tear down process working
> correctly, e.g. the ProxyDecryptor should fire all pending DecryptCBs before
> destruction.

Hi scherkus and ddorwin,

I added a Stop() method in Decryptor so that pending decrypt callbacks can be
aborted during pipeline tearing down. PTAL!

xhwang

[xhwang, 2012-08-01 16:25:07]

On 2012/07/31 16:11:14, xhwang wrote:
> On 2012/07/30 22:50:01, xhwang wrote:
> > FYI, I still need to figure out how to make the tear down process working
> > correctly, e.g. the ProxyDecryptor should fire all pending DecryptCBs before
> > destruction.
> 
> Hi scherkus and ddorwin,
> 
> I added a Stop() method in Decryptor so that pending decrypt callbacks can be
> aborted during pipeline tearing down. PTAL!
> 
> xhwang

Hello scherkus,

Updated as discussed yesterday. Please review.

Xiaohan

[scherkus (not reviewing), 2012-08-02 18:05:31]

lgtm w/ nits

http://codereview.chromium.org/10822026/diff/17005/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10822026/diff/17005/media/base/decryptor.h#new...
media/base/decryptor.h:81: // Stops the decryptor and fires any pending
DecryptCB immediately.
fires the cb with what result?

http://codereview.chromium.org/10822026/diff/17005/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/10822026/diff/17005/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:231: // Tell the ProxyDecryptor to abort
any pending decryptions.
we don't know this is a ProxyDecyptor

I also don't think this comment adds much so I would just get rid of it

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:237: decryptor_->Decrypt(encrypted,
base::Bind(&ProxyDecryptor::OnBufferDecrypted,
nit: technically this is mixing style: params on next line would be aligned w/
first arg at base::Bind(

can you drop the &ProxyDecryptor::OBD to next line as done on line 159-161?

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:92: bool is_shutting_down_;
nit: use stopped_ instead

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:159:
.WillOnce(InvokeWithoutArgs(
nit: instead of InvokeWithoutArgs you can have

ACTION_P(StopMessageLoop, message_loop) {
  message_loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
}

Then you can do:
.WillOnce(StopMessageLoop(&message_loop_))

[xhwang, 2012-08-03 20:08:10]

http://codereview.chromium.org/10822026/diff/17005/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10822026/diff/17005/media/base/decryptor.h#new...
media/base/decryptor.h:81: // Stops the decryptor and fires any pending
DecryptCB immediately.
On 2012/08/02 18:05:32, scherkus wrote:
> fires the cb with what result?

Done.

http://codereview.chromium.org/10822026/diff/17005/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/10822026/diff/17005/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:231: // Tell the ProxyDecryptor to abort
any pending decryptions.
On 2012/08/02 18:05:32, scherkus wrote:
> we don't know this is a ProxyDecyptor
> 
> I also don't think this comment adds much so I would just get rid of it

Done.

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:237: decryptor_->Decrypt(encrypted,
base::Bind(&ProxyDecryptor::OnBufferDecrypted,
On 2012/08/02 18:05:32, scherkus wrote:
> nit: technically this is mixing style: params on next line would be aligned w/
> first arg at base::Bind(
> 
> can you drop the &ProxyDecryptor::OBD to next line as done on line 159-161?

Done.

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.h (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.h:92: bool is_shutting_down_;
On 2012/08/02 18:05:32, scherkus wrote:
> nit: use stopped_ instead

Done.

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor_unittest.cc (right):

http://codereview.chromium.org/10822026/diff/17005/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor_unittest.cc:159:
.WillOnce(InvokeWithoutArgs(
On 2012/08/02 18:05:32, scherkus wrote:
> nit: instead of InvokeWithoutArgs you can have
> 
> ACTION_P(StopMessageLoop, message_loop) {
>   message_loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
> }
> 
> Then you can do:
> .WillOnce(StopMessageLoop(&message_loop_))

Done.

[xhwang, 2012-08-03 20:09:36]

piman@, could you please do an OWNERS review on this? It's just adding a test.

content/content_tests.gypi

[piman, 2012-08-03 20:11:46]

lgtm

[commit-bot: I haz the power, 2012-08-03 20:12:42]

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

[commit-bot: I haz the power, 2012-08-03 22:39:31]

Change committed as 149952