Decryptors can report kNoKey to WebMediaPlayer

Decryptors can report kNoKey to WebMediaPlayer

Add callback so that kNoKey can get passed to blink in order
to generate the HTMLMediaElement.waitingforkey event.

BUG=337975
TEST=updated tests pass

Committed: https://crrev.com/74fc4f9447f3ea2e277350dccce4de67e5ee4ea4
Cr-Commit-Position: refs/heads/master@{#318787}

[jrummell, 2015-02-19 04:43:46]

jrummell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, ddorwin@chromium.org

[jrummell, 2015-02-19 04:43:47]

PTAL. This is mostly just passing the callback through various layers, down to
decrypting_audio/video_decoders which use it. Some of the tests have been
updated to check that the callback happens.

[ddorwin, 2015-02-19 05:33:17]

I reviewed through WMPI then some other files. I think we can simplify this a
lot - see decoder_selector.cc.

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:59: //  |waiting_for_encryption_key_cb| is called
whenever a new key needs to be
...whenever the key needed to decrypt the stream is not available.

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:65: const base::Closure&
waiting_for_encryption_key_cb) = 0;
s/encryption/decryption/

https://codereview.chromium.org/935243002/diff/1/media/base/audio_renderer.h
File media/base/audio_renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/audio_renderer.h#...
media/base/audio_renderer.h:43: // |waiting_for_encryption_key_cb| is executed
whenever the stream needs
dittos

https://codereview.chromium.org/935243002/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:108: //   |waiting_for_encryption_key_cb| will be executed
whenever a stream needs
dittos

https://codereview.chromium.org/935243002/diff/1/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/renderer.h#newcode46
media/base/renderer.h:46: // - |waiting_for_encryption_key_cb|: Executed
whenever a stream needs a
dittos

https://codereview.chromium.org/935243002/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/video_decoder.h#n...
media/base/video_decoder.h:56: // decoded by Decode().
|waiting_for_encryption_key_cb| is called for
dittos

https://codereview.chromium.org/935243002/diff/1/media/base/video_renderer.h
File media/base/video_renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/video_renderer.h#...
media/base/video_renderer.h:57: // |waiting_for_encryption_key_cb| is executed
whenever the stream needs
dittos

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:215: // Called when the decoders need a key in
order to decrypt the encrypted
s/the decoders/a decoder/?

ditto on the description.

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:217: void OnWaitingForEncryptionKey();
Decryption

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
media/filters/decoder_selector.cc:112: task_runner_, set_decryptor_ready_cb_));
Can we just pass waiting_for_encryption_key_cb_ here? Note how we're already
passing an EME-specific CB here instead of in Initialize().

Do any of the decoders other than DVD and DAD need this?

If not, then we don't need to modify any of the decryptors

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
media/filters/decoder_selector.cc:136: new DecryptingDemuxerStream(task_runner_,
set_decryptor_ready_cb_));
ditto

https://codereview.chromium.org/935243002/diff/1/media/test/pipeline_integrat...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/935243002/diff/1/media/test/pipeline_integrat...
media/test/pipeline_integration_test.cc:698: base::Closure());
Shouldn't we check this here - this is the EME function.

[jrummell, 2015-02-23 22:06:32]

Updated, with question in webmediaplayer_impl.cc.

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:59: //  |waiting_for_encryption_key_cb| is called
whenever a new key needs to be
On 2015/02/19 05:33:17, ddorwin wrote:
> ...whenever the key needed to decrypt the stream is not available.

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:65: const base::Closure&
waiting_for_encryption_key_cb) = 0;
On 2015/02/19 05:33:17, ddorwin wrote:
> s/encryption/decryption/

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/audio_renderer.h
File media/base/audio_renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/audio_renderer.h#...
media/base/audio_renderer.h:43: // |waiting_for_encryption_key_cb| is executed
whenever the stream needs
On 2015/02/19 05:33:17, ddorwin wrote:
> dittos

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:108: //   |waiting_for_encryption_key_cb| will be executed
whenever a stream needs
On 2015/02/19 05:33:17, ddorwin wrote:
> dittos

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/renderer.h#newcode46
media/base/renderer.h:46: // - |waiting_for_encryption_key_cb|: Executed
whenever a stream needs a
On 2015/02/19 05:33:17, ddorwin wrote:
> dittos

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/video_decoder.h#n...
media/base/video_decoder.h:56: // decoded by Decode().
|waiting_for_encryption_key_cb| is called for
On 2015/02/19 05:33:17, ddorwin wrote:
> dittos

Done.

https://codereview.chromium.org/935243002/diff/1/media/base/video_renderer.h
File media/base/video_renderer.h (right):

https://codereview.chromium.org/935243002/diff/1/media/base/video_renderer.h#...
media/base/video_renderer.h:57: // |waiting_for_encryption_key_cb| is executed
whenever the stream needs
On 2015/02/19 05:33:17, ddorwin wrote:
> dittos

Done.

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:215: // Called when the decoders need a key in
order to decrypt the encrypted
On 2015/02/19 05:33:17, ddorwin wrote:
> s/the decoders/a decoder/?
> 
> ditto on the description.

Done.

https://codereview.chromium.org/935243002/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:217: void OnWaitingForEncryptionKey();
On 2015/02/19 05:33:17, ddorwin wrote:
> Decryption

Done.

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
media/filters/decoder_selector.cc:112: task_runner_, set_decryptor_ready_cb_));
On 2015/02/19 05:33:17, ddorwin wrote:
> Can we just pass waiting_for_encryption_key_cb_ here? Note how we're already
> passing an EME-specific CB here instead of in Initialize().
> 
> Do any of the decoders other than DVD and DAD need this?
> 
> If not, then we don't need to modify any of the decryptors

Done.

https://codereview.chromium.org/935243002/diff/1/media/filters/decoder_select...
media/filters/decoder_selector.cc:136: new DecryptingDemuxerStream(task_runner_,
set_decryptor_ready_cb_));
On 2015/02/19 05:33:17, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/935243002/diff/1/media/test/pipeline_integrat...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/935243002/diff/1/media/test/pipeline_integrat...
media/test/pipeline_integration_test.cc:698: base::Closure());
On 2015/02/19 05:33:17, ddorwin wrote:
> Shouldn't we check this here - this is the EME function.

Done.

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:682: // client_->waitingForKey();
Forgot to update this. There are 2 options.

1) Call didBlockPlaybackWaitingForKey() immediately followed by 
didResumePlaybackBlockedForKey(). This will generate the event for every
audio/video stream that needs a key.

2) Rather than use base::Closure as the type, create one that passes an enum
(NeedKey, GotKey). Based on the enum call the appropriate method on client_
(plus add the necessary call with GotKey inside DVD/DAD). This would likely mean
that the event is only generated once. 

Thoughts?

[ddorwin, 2015-02-23 23:47:47]

lgtm % comments

https://codereview.chromium.org/935243002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:204:
base::Bind(&PipelineTest::OnWaitingForDecryptionKey,
nit: There might be an existing mechanism in GMock to check that something is
not called.

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:682: // client_->waitingForKey();
On 2015/02/23 22:06:32, jrummell wrote:
> Forgot to update this. There are 2 options.
> 
> 1) Call didBlockPlaybackWaitingForKey() immediately followed by 
> didResumePlaybackBlockedForKey(). This will generate the event for every
> audio/video stream that needs a key.
> 
> 2) Rather than use base::Closure as the type, create one that passes an enum
> (NeedKey, GotKey). Based on the enum call the appropriate method on client_
> (plus add the necessary call with GotKey inside DVD/DAD). This would likely
mean
> that the event is only generated once. 
> 
> Thoughts?

Let's start with the simple solution then re-evaluate the best way to implement
this.

https://codereview.chromium.org/935243002/diff/20001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:119: EXPECT_CALL(*this,
OnWaitingForDecryptionKey()).Times(0);
ditto

[DaleCurtis, 2015-02-24 00:32:55]

A couple of questions first:
- Did you guys consider some sort of EmeParams class which could be passed
around instead of adding a 2nd callback?
- Do you see yourself adding any other callbacks in the near future?

It's getting to the point where we might want to consider creating a
RendererParams object for passing this mass around.

[jrummell, 2015-02-24 01:19:40]

Updated to make the change in WebMediaPlayerImpl::OnWaitingForDecryptionKey().
Includes a rebase to get the latest blink changes.

> - Did you guys consider some sort of EmeParams class which could be passed
> around instead of adding a 2nd callback?

This CB goes to DAD/DVD, while OnEncryptedMediaInitData() is passed to the
demuxers to report when they detect encrypted content. I don't see any other EME
specific CB passed through pipeline/renderer.

> - Do you see yourself adding any other callbacks in the near future?

We need to figure out the proper way to get didResumePlaybackBlockedForKey()
called (see
https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...).
I don't think that fixing that properly will require another CB passed though
pipeline/renderer. Ideally it would be done in the blink code, but the EME
objects don't know what player they're connected to.

https://codereview.chromium.org/935243002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:204:
base::Bind(&PipelineTest::OnWaitingForDecryptionKey,
On 2015/02/23 23:47:47, ddorwin wrote:
> nit: There might be an existing mechanism in GMock to check that something is
> not called.

The wiki states that .Times(0) is the way to do it
(https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#Disallowing_Unexp...).

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/935243002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:682: // client_->waitingForKey();
On 2015/02/23 23:47:47, ddorwin wrote:
> On 2015/02/23 22:06:32, jrummell wrote:
> > Forgot to update this. There are 2 options.
> > 
> > 1) Call didBlockPlaybackWaitingForKey() immediately followed by 
> > didResumePlaybackBlockedForKey(). This will generate the event for every
> > audio/video stream that needs a key.
> > 
> > 2) Rather than use base::Closure as the type, create one that passes an enum
> > (NeedKey, GotKey). Based on the enum call the appropriate method on client_
> > (plus add the necessary call with GotKey inside DVD/DAD). This would likely
> mean
> > that the event is only generated once. 
> > 
> > Thoughts?
> 
> Let's start with the simple solution then re-evaluate the best way to
implement
> this.

Done.

[ddorwin, 2015-02-24 01:36:52]

https://codereview.chromium.org/935243002/diff/40001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/935243002/diff/40001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:684: // TODO(jrummell):
didResumePlaybackBlockedForKey() should only be called
Bug? We should probably add a separate one and close the one you are fixing.

[DaleCurtis, 2015-02-24 23:24:54]

We're at the max parameters for a mock object method, so if any more are added
after this we will definitely have to switch to a struct.

I'm fine with landing this for now, but we should probably come up with a
cleaner path in a future CL if EME needs anything else.

You'll also need to fix the mojo instantiations of the renderer code.

[jrummell, 2015-02-25 19:06:40]

Updated with mojo changes.

https://codereview.chromium.org/935243002/diff/40001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/935243002/diff/40001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:684: // TODO(jrummell):
didResumePlaybackBlockedForKey() should only be called
On 2015/02/24 01:36:52, ddorwin wrote:
> Bug? We should probably add a separate one and close the one you are fixing.

Done.

[DaleCurtis, 2015-02-26 01:16:18]

dalecurtis@chromium.org changed reviewers:
+ xhwang@chromium.org

[DaleCurtis, 2015-02-26 01:16:19]

lgtm, unless xhwang@ has anything to add :)

[xhwang, 2015-02-26 18:55:53]

I have similar concerns as Dale: we are probably passing too many callbacks
around... Can we expand PipelineStatus to provide pipeline/player related info,
e.g. PIPELINE_INFO_NOKEY? Then we can reuse PipelineStatusCB in a lot of places
in this CL. We still need |waiting_for_decryption_key_cb| in Decrypting*
classes, but that should be fine.

[xhwang, 2015-02-28 01:34:44]

On 2015/02/26 18:55:53, xhwang wrote:
> I have similar concerns as Dale: we are probably passing too many callbacks
> around... Can we expand PipelineStatus to provide pipeline/player related
info,
> e.g. PIPELINE_INFO_NOKEY? Then we can reuse PipelineStatusCB in a lot of
places
> in this CL. We still need |waiting_for_decryption_key_cb| in Decrypting*
> classes, but that should be fine.

Per offline discussion, in our code, we have a lot of places where if the
PipelineStatus isn't PIPELINE_OK, we reset the error cb and destroy everything.
So the change I proposed won't be trivial. Long term I think that's still the
correct thing to do. But not in this CL :)

Also for the context: https://code.google.com/p/chromium/issues/detail?id=126070

With that, the current CL lgtm.

[jrummell, 2015-02-28 01:43:00]

Thanks for the reviews. I opened http://crbug.com/462820 in case anybody wants
to try to simplify the Pipeline/Renderer interface.

[jrummell, 2015-02-28 01:43:07]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-02-28 01:43:08]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/935243002/#ps60001 (title:
"mojo")

[commit-bot: I haz the power, 2015-02-28 01:44:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935243002/60001

[commit-bot: I haz the power, 2015-02-28 01:48:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-28 01:48:45]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jrummell, 2015-02-28 02:18:23]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-02-28 02:18:24]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
ddorwin@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/935243002/#ps80001 (title:
"rebase")

[commit-bot: I haz the power, 2015-02-28 02:18:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935243002/80001

[commit-bot: I haz the power, 2015-02-28 03:05:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-28 03:05:06]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[jrummell, 2015-03-02 21:50:39]

Updated with Android changes. xhwang@, can you take a look at the changes in
content/renderer/media/android/ ?

[xhwang, 2015-03-02 22:04:14]

lgtm % comment.

https://codereview.chromium.org/935243002/diff/100001/content/renderer/media/...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/935243002/diff/100001/content/renderer/media/...
content/renderer/media/android/media_source_delegate.cc:541:
waiting_for_decryption_key_cb_));
This fixes the decrypting-only part (where AesDecryptor is used). You also need
to fix the decrypt-and-decode part where ProxyMediaKeys and IPC are involved.
You can do that in a separate CL though.

[jrummell, 2015-03-02 22:25:18]

Thanks

https://codereview.chromium.org/935243002/diff/100001/content/renderer/media/...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/935243002/diff/100001/content/renderer/media/...
content/renderer/media/android/media_source_delegate.cc:541:
waiting_for_decryption_key_cb_));
On 2015/03/02 22:04:14, xhwang wrote:
> This fixes the decrypting-only part (where AesDecryptor is used). You also
need
> to fix the decrypt-and-decode part where ProxyMediaKeys and IPC are involved.
> You can do that in a separate CL though.

Acknowledged.

[jrummell, 2015-03-02 22:32:26]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-03-02 22:32:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
ddorwin@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/935243002/#ps100001
(title: "Android changes")

[commit-bot: I haz the power, 2015-03-02 22:40:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935243002/100001

[commit-bot: I haz the power, 2015-03-02 22:48:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-02 22:49:29]

Patchset 6 (id:??) landed as
https://crrev.com/74fc4f9447f3ea2e277350dccce4de67e5ee4ea4
Cr-Commit-Position: refs/heads/master@{#318787}