Move EME code out of WebMediaPlayerImpl.

Move EME code out of WebMediaPlayerImpl.

This change moves all the encrypted media code out of WebMediaPlayerImpl
and into a EncryptedMediaSupportImpl class. This removes the encrypted
media related content/ depencencies from WebMediaPlayerImpl so that it
will be easier to move WebMediaPlayerImpl to a location where it can
be reused by Mojo.

Committed: https://crrev.com/e2ea5183f7f5b2c562ed84258cc5bca4847b768b
Cr-Commit-Position: refs/heads/master@{#291776}

[acolwell GONE FROM CHROMIUM, 2014-08-22 19:57:57]

Please take a look.

Based on these changes it looks like we should really extract the EME methods
from the WebMediaPlayer and WebMediaPlayerClient interfaces since it appears
that most of the functionality doesn't even involve WMPI. EncryptedMediaSupport
would only need 3 virtual methods if we did that. In the interest of getting
WMPI moved quickly, I've opted to not do that refactoring right now.

[xhwang, 2014-08-22 20:52:52]

Didn't review the full impl of the new EMSI yet (assuming it's mostly moved from
WMPI). I just have some high level comments/questions.

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support.h (right):

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:22: class
EncryptedMediaSupport {
This seems to be an interface with only one implementation. In that case can we
drop that interface?

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:25:
blink::WebMediaPlayerClient* client);
Can this return NULL? It seems so based on the new code in WMPI. We should
document this in that case.

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:30: virtual
blink::WebMediaPlayer::MediaKeyException generateKeyRequest(
This is Chromium code, use Chromium style function name like GenerateKeyRequest?

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:646: if
(!encrypted_media_support_)
Here and below, this can only happen when EncryptedMediaSupport::create()
failed. Is this supposed to happen at all (see comments above about more doc on
the create() function)?

When create() returns NULL, does it mean that on this platform EME is not
supported? If so, shall return MediaKeyExceptionKeySystemNotSupported, or
MediaKeyExceptionKeySystemInvalidAccess?

[ddorwin, 2014-08-22 21:08:48]

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:254:
'renderer/media/crypto/encrypted_media_support.cc',
This filename is not super meaningful in media/crypto/. Maybe it should mention
[W]MP or something.

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:764:
'renderer/media/crypto/encrypted_media_support_impl.cc',
Is the intent that Android will have its own?

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support.h (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:30: virtual
blink::WebMediaPlayer::MediaKeyException generateKeyRequest(
Since we're changing things and this class is used for both cases, maybe we
should name these prefixed....().

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:56: virtual
media::Demuxer::NeedKeyCB CreateNeedKeyCB() = 0;
Should there be some documentation that the CB is bound?

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:58: virtual void
OnDecryptError() = 0;
This seems like something that the support impl would call on WMPI. How about
OnPipelineDecryptError with a comment that this is mostly informative and the
error has already been handled.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support_impl.cc (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support_impl.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Is this far too different from WMPI.cc to get any useful diff (even with a low
--similarity)?

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:646: if
(!encrypted_media_support_)
Is this possible given line 190?
Ditto below.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_params.h (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_params.h:16: // Holds parameters for
constructing WebMediaPlayerImpl without having
empty space was correct

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_params.h:41: }  // namespace media
content

[acolwell GONE FROM CHROMIUM, 2014-08-22 21:09:31]

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support.h (right):

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:22: class
EncryptedMediaSupport {
On 2014/08/22 20:52:51, xhwang wrote:
> This seems to be an interface with only one implementation. In that case can
we
> drop that interface?
I need the interface so that when I move WMPI, I don't need to pull in all of
EMSI's dependencies with it. When WMPI moves to media/blink so will this
interface, but EMSI will stay in content/renderer/media. Since this interface is
abstract I can still use WMPI in Mojo w/o enabling encrypted media support. Also
one can imagine creating a new implementation of this interface that integrates
with the Mojo world.

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:25:
blink::WebMediaPlayerClient* client);
On 2014/08/22 20:52:51, xhwang wrote:
> Can this return NULL? It seems so based on the new code in WMPI. We should
> document this in that case.
Yes. Done.

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:30: virtual
blink::WebMediaPlayer::MediaKeyException generateKeyRequest(
On 2014/08/22 20:52:51, xhwang wrote:
> This is Chromium code, use Chromium style function name like
GenerateKeyRequest?

Done.

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:646: if
(!encrypted_media_support_)
On 2014/08/22 20:52:51, xhwang wrote:
> Here and below, this can only happen when EncryptedMediaSupport::create()
> failed. Is this supposed to happen at all (see comments above about more doc
on
> the create() function)?
Yes.

> 
> When create() returns NULL, does it mean that on this platform EME is not
> supported? If so, shall return MediaKeyExceptionKeySystemNotSupported, or
> MediaKeyExceptionKeySystemInvalidAccess?
Yes. I changed it to MediaKeyExceptionKeySystemNotSupported.

[acolwell GONE FROM CHROMIUM, 2014-08-22 23:20:23]

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:254:
'renderer/media/crypto/encrypted_media_support.cc',
On 2014/08/22 21:08:47, ddorwin wrote:
> This filename is not super meaningful in media/crypto/. Maybe it should
mention
> [W]MP or something.
Changed to encrypted_media_player_support based on offline discussion.

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:764:
'renderer/media/crypto/encrypted_media_support_impl.cc',
On 2014/08/22 21:08:47, ddorwin wrote:
> Is the intent that Android will have its own?
No, but this code doesn't build under Android because of the "#error Browser
side CDM in WMPI for prefixed EME API not supported yet." in the code. I took
quick look at WMPA and it looks like most of the code is the same so it could
probably use most of the code in here eventually.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support.h (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:30: virtual
blink::WebMediaPlayer::MediaKeyException generateKeyRequest(
On 2014/08/22 21:08:47, ddorwin wrote:
> Since we're changing things and this class is used for both cases, maybe we
> should name these prefixed....().
Kept names as-is but added comment based on offline conversation.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:56: virtual
media::Demuxer::NeedKeyCB CreateNeedKeyCB() = 0;
On 2014/08/22 21:08:48, ddorwin wrote:
> Should there be some documentation that the CB is bound?
I don't understand what you mean here. I added a comment indication what this
callback is used for.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support.h:58: virtual void
OnDecryptError() = 0;
On 2014/08/22 21:08:48, ddorwin wrote:
> This seems like something that the support impl would call on WMPI. How about
> OnPipelineDecryptError with a comment that this is mostly informative and the
> error has already been handled.
Done.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_support_impl.cc (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_support_impl.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/08/22 21:08:48, ddorwin wrote:
> Is this far too different from WMPI.cc to get any useful diff (even with a low
> --similarity)?
I don't know what you mean. Most of this code is simply copy and pasted with a
s/WebMediaPlayerImpl/EncryptedMediaPlayerImpl/g

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:646: if
(!encrypted_media_support_)
On 2014/08/22 21:08:48, ddorwin wrote:
> Is this possible given line 190?
> Ditto below.
Yes. That was the idea, but now that I think of it, perhaps it would be better
to just force other embedders to create a fake impl that always returns errors.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_params.h (right):

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_params.h:16: // Holds parameters for
constructing WebMediaPlayerImpl without having
On 2014/08/22 21:08:48, ddorwin wrote:
> empty space was correct
Done. I think format was being... helpful.

https://codereview.chromium.org/501473003/diff/60001/content/renderer/media/w...
content/renderer/media/webmediaplayer_params.h:41: }  // namespace media
On 2014/08/22 21:08:48, ddorwin wrote:
> content

Done.

[xhwang, 2014-08-23 00:31:57]

lgtm

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
File content/renderer/media/crypto/encrypted_media_player_support.h (right):

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support.h:26: static
scoped_ptr<EncryptedMediaPlayerSupport> create(
s/create/Create

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support.h:59:
blink::WebContentDecryptionModule* cdm) = 0;
For your info, later on when prefixed EME is deprecated, we can let Pipeline or
Rendeer own this class to help solve https://crbug.com/401264.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:190:
encrypted_media_support_(EncryptedMediaPlayerSupport::create(client)) {
DCHECK(encrypted_media_support_)?

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:736:
encrypted_media_support_->OnPipelineDecryptError();
PIPELINE_ERROR_DECRYPT is pretty much deprecated. Feel free to drop this UMA
altogether.

+ddorwin to confirm.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:847:
encrypted_media_support_->CreateNeedKeyCB();
FYI, we should be able to pass the need key cb in Demuxer::Initialize(). When we
do that, we can probably just pass WMPI::OnNeedKey() to the Pipeline.

[xhwang, 2014-08-23 00:33:18]

I was landing https://codereview.chromium.org/418143005/ and I just realized
that you need to rebase if that lands first. Sorry for the trouble.

[ddorwin, 2014-08-25 17:10:15]

LGTM. I didn't review encrypted_media_player_support* in detail.

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:764:
'renderer/media/crypto/encrypted_media_support_impl.cc',
On 2014/08/22 23:20:22, acolwell wrote:
> On 2014/08/22 21:08:47, ddorwin wrote:
> > Is the intent that Android will have its own?
> No, but this code doesn't build under Android because of the "#error Browser
> side CDM in WMPI for prefixed EME API not supported yet." in the code. I took
> quick look at WMPA and it looks like most of the code is the same so it could
> probably use most of the code in here eventually.

Won't you need to address WMPA before moving code to media/blink? Is that the
next step?

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:736:
encrypted_media_support_->OnPipelineDecryptError();
On 2014/08/23 00:31:57, xhwang wrote:
> PIPELINE_ERROR_DECRYPT is pretty much deprecated. Feel free to drop this UMA
> altogether.
> 
> +ddorwin to confirm.

MEDIA_ERR_DECRYPT is dead, but I don't know if this code is doing anything else
useful (i.e. tracking CDM-related failures). I'm fine with removing it if it's
unused, but that should probably be a larger CL to rip it out of the pipeline.
This isn't much extra code in this CL.

[acolwell GONE FROM CHROMIUM, 2014-08-25 18:10:43]

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/501473003/diff/60001/content/content_renderer...
content/content_renderer.gypi:764:
'renderer/media/crypto/encrypted_media_support_impl.cc',
On 2014/08/25 17:10:14, ddorwin wrote:
> On 2014/08/22 23:20:22, acolwell wrote:
> > On 2014/08/22 21:08:47, ddorwin wrote:
> > > Is the intent that Android will have its own?
> > No, but this code doesn't build under Android because of the "#error Browser
> > side CDM in WMPI for prefixed EME API not supported yet." in the code. I
took
> > quick look at WMPA and it looks like most of the code is the same so it
could
> > probably use most of the code in here eventually.
> 
> Won't you need to address WMPA before moving code to media/blink? Is that the
> next step?

I don't plan on moving WMPA to media/blink yet and I don't believe I'll need to
resolve this before. We'll see when I get my WMPI move patch uploaded and sent
to the try bots.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
File content/renderer/media/crypto/encrypted_media_player_support.h (right):

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support.h:26: static
scoped_ptr<EncryptedMediaPlayerSupport> create(
On 2014/08/23 00:31:57, xhwang wrote:
> s/create/Create

Done.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support.h:59:
blink::WebContentDecryptionModule* cdm) = 0;
On 2014/08/23 00:31:57, xhwang wrote:
> For your info, later on when prefixed EME is deprecated, we can let Pipeline
or
> Rendeer own this class to help solve https://crbug.com/401264.

Acknowledged.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:190:
encrypted_media_support_(EncryptedMediaPlayerSupport::create(client)) {
On 2014/08/23 00:31:57, xhwang wrote:
> DCHECK(encrypted_media_support_)?

Done.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:736:
encrypted_media_support_->OnPipelineDecryptError();
On 2014/08/25 17:10:14, ddorwin wrote:
> On 2014/08/23 00:31:57, xhwang wrote:
> > PIPELINE_ERROR_DECRYPT is pretty much deprecated. Feel free to drop this UMA
> > altogether.
> > 
> > +ddorwin to confirm.
> 
> MEDIA_ERR_DECRYPT is dead, but I don't know if this code is doing anything
else
> useful (i.e. tracking CDM-related failures). I'm fine with removing it if it's
> unused, but that should probably be a larger CL to rip it out of the pipeline.
> This isn't much extra code in this CL.

It looks like the VideoRendererImpl still generates this error. For now I'll
leave the code as is and let you guys follow up with removing the UMA and
pipeline error if you don't think it is helpful anymore.

https://codereview.chromium.org/501473003/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:847:
encrypted_media_support_->CreateNeedKeyCB();
On 2014/08/23 00:31:57, xhwang wrote:
> FYI, we should be able to pass the need key cb in Demuxer::Initialize(). When
we
> do that, we can probably just pass WMPI::OnNeedKey() to the Pipeline.

Acknowledged.

[acolwell GONE FROM CHROMIUM, 2014-08-25 20:41:43]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-25 20:43:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/501473003/140001

[commit-bot: I haz the power, 2014-08-25 22:05:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[commit-bot: I haz the power, 2014-08-25 23:01:29]

Committed patchset #8 (140001) as ece90a6a020e867855318ec21b7d9f6bd1a10dce

[commit-bot: I haz the power, 2014-09-10 02:38:08]

Patchset 8 (id:??) landed as
https://crrev.com/e2ea5183f7f5b2c562ed84258cc5bca4847b768b
Cr-Commit-Position: refs/heads/master@{#291776}