media: Support SetCdm() on VideoDecodeAccelerator interface.

media: Support SetCdm() on VideoDecodeAccelerator interface.

This is needed to support encrypted stream handling in VideoDecodeAccelerator
(VDA) implmentations on certain platforms (e.g. Android).

BUG=545099
Committed: https://crrev.com/5acd08322f6402c527b3197e973b727370584797
Cr-Commit-Position: refs/heads/master@{#359696}

[xhwang, 2015-11-11 22:23:16]

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

[xhwang, 2015-11-11 22:23:16]

xhwang@chromium.org changed required reviewers:
+ liberato@chromium.org

[xhwang, 2015-11-11 22:23:16]

This is pure plumbing code getting SetCdm() call through. I'll hook it up in
GpuVideoDecoder in the next CL after
https://chromiumcodereview.appspot.com/1423163004/ lands.

liberato: PTAL
sandersd/timav: FYI

[xhwang, 2015-11-11 22:26:05]

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

[xhwang, 2015-11-11 22:26:05]

ccameron: Please OWNERS review content/common/gpu/* changes.

[xhwang, 2015-11-11 22:29:44]

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

[xhwang, 2015-11-11 22:29:45]

dcheng: Please OWNERS review changes in content/common/gpu/gpu_messages.h.

[liberato (no reviews please), 2015-11-11 23:29:10]

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/g...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/g...
content/common/gpu/gpu_messages.h:744:
IPC_MESSAGE_ROUTED1(AcceleratedVideoDecoderHostMsg_CdmSet,
if you're feeling sufficiently obsessive-compulsive, then please reorder to
match gpu_video_decode_accelerator, which itself doesn't match
video_decode_accelerator.h .  these were out of order before, though.

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
content/common/gpu/media/gpu_video_decode_accelerator.h:54: void
NotifyCdmSet(bool success) override;
was broken before, but would you mind reordering these to match
video_decode_accelerator.h?  NotifyError is in the wrong spot.

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
media/video/video_decode_accelerator.h:86: // SetCdm completion callback.
might want to describe default behavior, here and SetCdm.

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
media/video/video_decode_accelerator.h:149: // followed by
Client::NotifyCdmSet() being called with the setting result.
right now, AVDA::Initialize, which is called very early, configures the codec
without a cdm.  to add one later, we'll have to destroy the codec, or defer
initialization.  how do you plan on handling this?

also, is it okay to call this at any time, or are there restrictions (e.g.
before the first call to Decode())?

[dcheng, 2015-11-12 18:48:43]

https://codereview.chromium.org/1438063002/diff/1/content/common/gpu/gpu_mess...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1438063002/diff/1/content/common/gpu/gpu_mess...
content/common/gpu/gpu_messages.h:744:
IPC_MESSAGE_ROUTED1(AcceleratedVideoDecoderHostMsg_CdmSet,
I think this could be named more intuitively (because naively, you might read
this as setting the cdm too).

Maybe SetCdmDone to match Reset/ResetDone?

https://codereview.chromium.org/1438063002/diff/1/content/common/gpu/media/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1438063002/diff/1/content/common/gpu/media/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:554:
DCHECK(video_decode_accelerator_.get());
Nit: no .get() when testing a smart pointer

[xhwang, 2015-11-13 01:16:55]

Comments addressed. PTAL again!

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/g...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/g...
content/common/gpu/gpu_messages.h:744:
IPC_MESSAGE_ROUTED1(AcceleratedVideoDecoderHostMsg_CdmSet,
On 2015/11/12 18:48:43, dcheng wrote:
> I think this could be named more intuitively (because naively, you might read
> this as setting the cdm too).
> 
> Maybe SetCdmDone to match Reset/ResetDone?

Renamed to CdmAttached which is already used in various places:

https://code.google.com/p/chromium/codesearch#search/&q=CdmAttached&sq=packag...

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/g...
content/common/gpu/gpu_messages.h:744:
IPC_MESSAGE_ROUTED1(AcceleratedVideoDecoderHostMsg_CdmSet,
On 2015/11/11 23:29:10, liberato wrote:
> if you're feeling sufficiently obsessive-compulsive, then please reorder to
> match gpu_video_decode_accelerator, which itself doesn't match
> video_decode_accelerator.h .  these were out of order before, though.

I fixed gpu_vda, which is in a mess.

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
content/common/gpu/media/gpu_video_decode_accelerator.cc:554:
DCHECK(video_decode_accelerator_.get());
On 2015/11/12 18:48:43, dcheng wrote:
> Nit: no .get() when testing a smart pointer

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/content/common/gpu/m...
content/common/gpu/media/gpu_video_decode_accelerator.h:54: void
NotifyCdmSet(bool success) override;
On 2015/11/11 23:29:10, liberato wrote:
> was broken before, but would you mind reordering these to match
> video_decode_accelerator.h?  NotifyError is in the wrong spot.

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
media/video/video_decode_accelerator.h:86: // SetCdm completion callback.
On 2015/11/11 23:29:10, liberato wrote:
> might want to describe default behavior, here and SetCdm.

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/1/media/video/video_de...
media/video/video_decode_accelerator.h:149: // followed by
Client::NotifyCdmSet() being called with the setting result.
On 2015/11/11 23:29:10, liberato wrote:
> right now, AVDA::Initialize, which is called very early, configures the codec
> without a cdm.  to add one later, we'll have to destroy the codec, or defer
> initialization.  how do you plan on handling this?
> 
> also, is it okay to call this at any time, or are there restrictions (e.g.
> before the first call to Decode())?

As discussed offline, there's no way to finish codec configuration synchronously
(Initialize is sync) on Android when EME is involved. So Initialize will return
immediately, but the real initialization will be delayed until the SetCdm(). The
real initialization result is returned in NotifyCdmAttached() call.

Updated the comments to clarify these.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:155:
gpu::VideoDecodeAcceleratorSupportedProfiles
here and below, non-cdm methods are moved to fix the order, not code change.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/20001/media/video/vide...
media/video/video_decode_accelerator.h:66: virtual void NotifyCdmAttached(bool
success);
Moved up since this is part of the initialization process.

[dcheng, 2015-11-13 19:54:53]

ipc changes lgtm

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:683: int32) /* CDM ID */
Nit: let's use int32_t for this and the IPC message handler since we're (slowly)
trying to transition off base/basictypes.h

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:155:
gpu::VideoDecodeAcceleratorSupportedProfiles
On 2015/11/13 at 01:16:55, xhwang wrote:
> here and below, non-cdm methods are moved to fix the order, not code change.

In the future, maybe just do this in a separate patch (either before or after is
fine, just makes the review easier)

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:306: if (filter_.get())
{
No .get() (unless you end up moving this code in a separate patch, then just
leave this alone I guess)

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:319: if (filter_.get()
&& io_task_runner_->BelongsToCurrentThread())
Ditto

https://codereview.chromium.org/1438063002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:489:
DCHECK(video_decode_accelerator_.get());
No .get()

[xhwang, 2015-11-13 20:06:22]

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/gpu_messages.h:683: int32) /* CDM ID */
On 2015/11/13 19:54:52, dcheng wrote:
> Nit: let's use int32_t for this and the IPC message handler since we're
(slowly)
> trying to transition off base/basictypes.h

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:155:
gpu::VideoDecodeAcceleratorSupportedProfiles
On 2015/11/13 19:54:53, dcheng wrote:
> On 2015/11/13 at 01:16:55, xhwang wrote:
> > here and below, non-cdm methods are moved to fix the order, not code change.
> 
> In the future, maybe just do this in a separate patch (either before or after
is
> fine, just makes the review easier)

Acknowledged.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:306: if (filter_.get())
{
On 2015/11/13 19:54:53, dcheng wrote:
> No .get() (unless you end up moving this code in a separate patch, then just
> leave this alone I guess)

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:319: if (filter_.get()
&& io_task_runner_->BelongsToCurrentThread())
On 2015/11/13 19:54:53, dcheng wrote:
> Ditto

Done.

https://chromiumcodereview.appspot.com/1438063002/diff/20001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:489:
DCHECK(video_decode_accelerator_.get());
On 2015/11/13 19:54:53, dcheng wrote:
> No .get()

Done.

[xhwang, 2015-11-13 20:06:54]

liberato: PTAL again!

ccameron: Please OWNERS review content/common/gpu/* changes.

[liberato (no reviews please), 2015-11-13 22:51:54]

lgtm.

[ccameron, 2015-11-13 22:59:44]

owner lgtm stamp

[xhwang, 2015-11-13 23:36:31]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-11-13 23:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1438063002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-11-13 23:38:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438063002/40001

[commit-bot: I haz the power, 2015-11-13 23:57:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-13 23:58:42]

Patchset 3 (id:??) landed as
https://crrev.com/5acd08322f6402c527b3197e973b727370584797
Cr-Commit-Position: refs/heads/master@{#359696}