Passed is_encrypted parameter to the VDA initialization.

Passed is_encrypted parameter to the VDA initialization.

The VDA on Android (AVDA) requires this info to properly
implement CDM. This CL changes the method
VideoDecodeAccelerator::Initialize() to accept the structure
holding VDA initialization parameters. There are two parameters:
video codec profile and the is_encrypted flag. For Android both
parameters are propagated to GPU process.

BUG=542417
Committed: https://crrev.com/e32b7be080461db66d13fd54b4e312568efe03e9
Cr-Commit-Position: refs/heads/master@{#363127}

[Tima Vaisburd, 2015-12-01 01:41:53]

timav@chromium.org changed reviewers:
+ dcheng@chromium.org, liberato@chromium.org, sandersd@chromium.org,
xhwang@chromium.org

[Tima Vaisburd, 2015-12-01 01:41:54]

PTAL.

+sandersd@ as content/common/gpu/ owner
+dcheng@ as content/common/gpu/gpu_messages.h owner

https://codereview.chromium.org/1485043002/diff/1/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1485043002/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.h:185: const
media::VideoDecodeAccelerator::InitParams& params,
I'm reusing InitParams for a slightly different purpose here, these params are
passed by IPC.

[dcheng, 2015-12-01 02:03:13]

lgtm but one nit

https://codereview.chromium.org/1485043002/diff/1/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:59: bool is_encrypted = false);
The style guide forbids default args.

[xhwang, 2015-12-01 07:15:02]

looking good. just a few comments

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:68: // one of them
succeeds for given |profile|. Send the |init_done_msg| when
update this comment

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:447:
video_profile_ = params.profile;
In all non-android concrete VDAs, DCHECK that |is_encrypted is false.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT InitParams {
We use VideoDecoderConfig to initialize VideoDecoders. In theory, we can just
pass that to VDA::Initialize() as well. But in practice it seems the VDA only
needs a subset of VideoDecoderConfig. So this approach is fine. But to make it
more consistent with VideoDecoder, can we do the following in this CL?

s/InitParams/Config
s/params/config

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:59: bool is_encrypted = false);
no default parameters per style guide

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:61: bool is_encrypted;
You can use Non-Static Class Member Initializers here, e.g. 

bool is_encrypted = false;

[Pawel Osciak, 2015-12-01 07:16:06]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2015-12-01 07:16:07]

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:436: bool
V4L2SliceVideoDecodeAccelerator::Initialize(
Initialize() should fail for VDAs that don't support params.is_encrypted ==
true, here and in other VDAs.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:437: const
media::VideoDecodeAccelerator::InitParams& params,
Just "const InitParam& params" should work as well.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:439: DVLOGF(3)
<< "profile: " << profile;
s/profile/params.profile/

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:56: // Parameters required for the
Initialize().
Please document all parameters.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT InitParams {
Perhaps s/InitParam/StreamParameters/ or StreamConfig?

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:59: bool is_encrypted = false);
Default parameters are generally not allowed by coding style, please see
https://www.chromium.org/developers/coding-style.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:125: virtual bool Initialize(const
InitParams& params, Client* client) = 0;
Would it be possible to just indicate encrypted stream by calling SetCdm()? If
we don't call SetCdm() before Decode(), that would indicate a regular
unencrypted stream...

[liberato (no reviews please), 2015-12-01 14:59:35]

lgtm % nits.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.h:185: const
media::VideoDecodeAccelerator::InitParams& params,
nit - formatting.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:107: is_encrypted_
= params.is_encrypted;
might just want to keep a copy of the params as an avda member.  we'll have "foo
= params.foo" often if we add more config params.  fine as it is, too.

https://codereview.chromium.org/1485043002/diff/60001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:178: VideoDecodeAccelerator::InitParams
init_params(config.profile(),
perhaps add a constructor to InitParams that takes the VideoDecoderConfig and
extracts what it wants.

[Tima Vaisburd, 2015-12-02 02:25:07]

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.h:185: const
media::VideoDecodeAccelerator::InitParams& params,
On 2015/12/01 14:59:35, liberato wrote:
> nit - formatting.

This is the result of git cl format, perhaps because the line was too long.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:107: is_encrypted_
= params.is_encrypted;
On 2015/12/01 14:59:35, liberato wrote:
> might just want to keep a copy of the params as an avda member.  we'll have
"foo
> = params.foo" often if we add more config params.  fine as it is, too.

|codec_| already existed. The idea was that a particular VDA extracts what it
needs, the InitParams (now StreamParameters) might have more members with some
potentially irrelevant.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:68: // one of them
succeeds for given |profile|. Send the |init_done_msg| when
On 2015/12/01 07:15:02, xhwang wrote:
> update this comment

Done.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:436: bool
V4L2SliceVideoDecodeAccelerator::Initialize(
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> Initialize() should fail for VDAs that don't support params.is_encrypted ==
> true, here and in other VDAs.

Done.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:437: const
media::VideoDecodeAccelerator::InitParams& params,
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> Just "const InitParam& params" should work as well.

Done.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:439: DVLOGF(3)
<< "profile: " << profile;
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> s/profile/params.profile/

Done.

https://codereview.chromium.org/1485043002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:447:
video_profile_ = params.profile;
On 2015/12/01 07:15:02, xhwang wrote:
> In all non-android concrete VDAs, DCHECK that |is_encrypted is false.

Returned |false| instead as recommended by Pawel Osciak

https://codereview.chromium.org/1485043002/diff/60001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1485043002/diff/60001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:178: VideoDecodeAccelerator::InitParams
init_params(config.profile(),
On 2015/12/01 14:59:35, liberato wrote:
> perhaps add a constructor to InitParams that takes the VideoDecoderConfig and
> extracts what it wants.

Done.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:56: // Parameters required for the
Initialize().
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> Please document all parameters.

Done.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT InitParams {
On 2015/12/01 07:15:02, xhwang wrote:
> We use VideoDecoderConfig to initialize VideoDecoders. In theory, we can just
> pass that to VDA::Initialize() as well. But in practice it seems the VDA only
> needs a subset of VideoDecoderConfig. So this approach is fine. But to make it
> more consistent with VideoDecoder, can we do the following in this CL?
> 
> s/InitParams/Config
> s/params/config

Did s/InitParams/StreamParameters/

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT InitParams {
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> Perhaps s/InitParam/StreamParameters/ or StreamConfig?

Chose StreamParameters to distinguish from VideoDecoderConfig which is now an
argument to one of StreamParameters constructors.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:59: bool is_encrypted = false);
On 2015/12/01 07:15:02, xhwang wrote:
> no default parameters per style guide

Done.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:59: bool is_encrypted = false);
On 2015/12/01 07:15:02, xhwang wrote:
> no default parameters per style guide

Done.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:61: bool is_encrypted;
On 2015/12/01 07:15:02, xhwang wrote:
> You can use Non-Static Class Member Initializers here, e.g. 
> 
> bool is_encrypted = false;

Since we have several constructors now I thought it would be clearer to have the
initialization solely in constructors.

https://codereview.chromium.org/1485043002/diff/60001/media/video/video_decod...
media/video/video_decode_accelerator.h:125: virtual bool Initialize(const
InitParams& params, Client* client) = 0;
On 2015/12/01 07:16:07, Pawel Osciak wrote:
> Would it be possible to just indicate encrypted stream by calling SetCdm()? If
> we don't call SetCdm() before Decode(), that would indicate a regular
> unencrypted stream...

On Android the Initialize() call might come before SetCdm(). For unencrypted
streams I need to configure MediaCodec inside Initialize(), and for the
encrypted streams I need to postpone this configuration till MediaCrypto object
is created, which happens after SetCdm(). In other words, in the Initialize() I
need to know, it seems, whether SetCdm() will be called or not. This is the
reason I pass the is_encrypted flag.

[xhwang, 2015-12-02 06:48:19]

https://codereview.chromium.org/1485043002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:638: DLOG(ERROR) <<
"Encrypted streams are not supported for this VDA";
Here and below, how about

NOTREACHED() << "Encrypted.....";

I like the "return false" below. But we shouldn't get here in the first place.

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.cc:13: :
profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}
OOC, where do we need this default ctor?

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.cc:15:
VideoDecodeAccelerator::StreamParameters::StreamParameters(VideoCodecProfile p)
no meaningless abbreviations:
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
IMHO "Parameters" doesn't add any value to the name.  Also, I don't see why we
want to distinguish this from VideoDecoderConfig. If not for the sake of
additional IPC work, I'd like to send VideoDecoderConfig directly to
VDA::Initialize() to avoid reinventing a new type for the initialization config.
VDA::Config seems a perfect tradeoff to me...

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:66: bool is_encrypted;
Having multiple constructors is actually the perfect case to use non-static
class member initializers. That way, you  have consistent default values and
only overwrite members with non-default values in the initialization list.

Just in case it's not clear, I meant having default values here for both
|profile| and |is_encrypted|.

Imagine what if you have a typo in the following code. It's hard to find it out.

VideoDecodeAccelerator::StreamParameters::StreamParameters()
  : profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}

VideoDecodeAccelerator::StreamParameters::StreamParameters(VideoCodecProfile p)
   : profile(p), is_encrypted(true) {}

[liberato (no reviews please), 2015-12-02 16:18:50]

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 06:48:18, xhwang wrote:
> IMHO "Parameters" doesn't add any value to the name.  Also, I don't see why we
> want to distinguish this from VideoDecoderConfig. If not for the sake of
> additional IPC work, I'd like to send VideoDecoderConfig directly to
> VDA::Initialize() to avoid reinventing a new type for the initialization
config.
> VDA::Config seems a perfect tradeoff to me...

i don't think that VDA::Config will always be a subset of VDC.  we've had
several discussions lately (e.g., full screen surface ID) that suggest it won't
be.  since updating all the VDA implementations is no fun, i believe that we
definitely need some VDA-specific config type, rather than sending the VDC
verbatim. +1 xhwang's VDA::Config name.

i also really don't like 'shadow types', but the IPC cost prevented me from
objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it did
yesterday; VDC is lightweight and initialization is infrequent.

how about this, which is what i would favor if IPC weren't involved:

class VideoDecodeAccelerator::Config {
  VideoDecoderConfig decoder_config_;
  // other stuff as it shows up
}

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:66: bool is_encrypted;
On 2015/12/02 06:48:18, xhwang wrote:
> Having multiple constructors is actually the perfect case to use non-static
> class member initializers. That way, you  have consistent default values and
> only overwrite members with non-default values in the initialization list.
> 
> Just in case it's not clear, I meant having default values here for both
> |profile| and |is_encrypted|.
> 
> Imagine what if you have a typo in the following code. It's hard to find it
out.
> 
> VideoDecodeAccelerator::StreamParameters::StreamParameters()
>   : profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}
> 
> VideoDecodeAccelerator::StreamParameters::StreamParameters(VideoCodecProfile
p)
>    : profile(p), is_encrypted(true) {}

+1

[Tima Vaisburd, 2015-12-02 18:02:15]

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.cc:13: :
profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}
On 2015/12/02 06:48:18, xhwang wrote:
> OOC, where do we need this default ctor?

I think it is required by auto generated gpu messages, since this structure is
also directly used in IPC.

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 16:18:50, liberato wrote:
> On 2015/12/02 06:48:18, xhwang wrote:
> > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see why
we
> > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > VDA::Initialize() to avoid reinventing a new type for the initialization
> config.
> > VDA::Config seems a perfect tradeoff to me...
> 
> i don't think that VDA::Config will always be a subset of VDC.  we've had
> several discussions lately (e.g., full screen surface ID) that suggest it
won't
> be.  since updating all the VDA implementations is no fun, i believe that we
> definitely need some VDA-specific config type, rather than sending the VDC
> verbatim. +1 xhwang's VDA::Config name.
> 
> i also really don't like 'shadow types', but the IPC cost prevented me from
> objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it did
> yesterday; VDC is lightweight and initialization is infrequent.
> 
> how about this, which is what i would favor if IPC weren't involved:
> 
> class VideoDecodeAccelerator::Config {
>   VideoDecoderConfig decoder_config_;
>   // other stuff as it shows up
> }

(1) Not every method calling Initialize() has VDC easily available, what to do
in rtc_video_decoder.cc ?
(2) Do you prefer passing the whole VDC over IPC even if we use only 2 fields,
namely, profile and is_encrypted? Or shall there be another structure dedicated
just for IPC?

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:66: bool is_encrypted;
On 2015/12/02 06:48:18, xhwang wrote:
> Having multiple constructors is actually the perfect case to use non-static
> class member initializers. That way, you  have consistent default values and
> only overwrite members with non-default values in the initialization list.
> 
> Just in case it's not clear, I meant having default values here for both
> |profile| and |is_encrypted|.
> 
> Imagine what if you have a typo in the following code. It's hard to find it
out.
> 
> VideoDecodeAccelerator::StreamParameters::StreamParameters()
>   : profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}
> 
> VideoDecodeAccelerator::StreamParameters::StreamParameters(VideoCodecProfile
p)
>    : profile(p), is_encrypted(true) {}

Acknowledged.

[liberato (no reviews please), 2015-12-02 18:29:47]

yet another beautiful theory spoiled by facts.

-fl

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> On 2015/12/02 16:18:50, liberato wrote:
> > On 2015/12/02 06:48:18, xhwang wrote:
> > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see
why
> we
> > > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > VDA::Initialize() to avoid reinventing a new type for the initialization
> > config.
> > > VDA::Config seems a perfect tradeoff to me...
> > 
> > i don't think that VDA::Config will always be a subset of VDC.  we've had
> > several discussions lately (e.g., full screen surface ID) that suggest it
> won't
> > be.  since updating all the VDA implementations is no fun, i believe that we
> > definitely need some VDA-specific config type, rather than sending the VDC
> > verbatim. +1 xhwang's VDA::Config name.
> > 
> > i also really don't like 'shadow types', but the IPC cost prevented me from
> > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it
did
> > yesterday; VDC is lightweight and initialization is infrequent.
> > 
> > how about this, which is what i would favor if IPC weren't involved:
> > 
> > class VideoDecodeAccelerator::Config {
> >   VideoDecoderConfig decoder_config_;
> >   // other stuff as it shows up
> > }
> 
> (1) Not every method calling Initialize() has VDC easily available, what to do
> in rtc_video_decoder.cc ?
> (2) Do you prefer passing the whole VDC over IPC even if we use only 2 fields,
> namely, profile and is_encrypted? Or shall there be another structure
dedicated
> just for IPC? 

(1) good question -- i don't know.  "whole VDC" doesn't bother me, though, in
this case.  but if not every use of VDA has one, well, that's almost the same
problem that makes me not like "VDCLite" - manually thunking one config
structure into another can go wrong easily.

(2) ignoring (1) for a moment, i'd favor all of VDC.  otherwise, we'll end up
adding "just one more field" to the interim structure, and/or forgetting to
initialize it elsewhere.

on balance, creating VDA::Config with just |profile_| and |is_encrypted_| seems
like the best of a bad set of options.  might as well provide only the fields
that have meaningful data, and centralize the thunk from VDC in the VDA::C
constructor.

[xhwang, 2015-12-02 18:58:35]

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 18:29:47, liberato wrote:
> On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> > On 2015/12/02 16:18:50, liberato wrote:
> > > On 2015/12/02 06:48:18, xhwang wrote:
> > > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see
> why
> > we
> > > > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > > VDA::Initialize() to avoid reinventing a new type for the initialization
> > > config.
> > > > VDA::Config seems a perfect tradeoff to me...
> > > 
> > > i don't think that VDA::Config will always be a subset of VDC.  we've had
> > > several discussions lately (e.g., full screen surface ID) that suggest it
> > won't
> > > be.  since updating all the VDA implementations is no fun, i believe that
we
> > > definitely need some VDA-specific config type, rather than sending the VDC
> > > verbatim. +1 xhwang's VDA::Config name.
> > > 
> > > i also really don't like 'shadow types', but the IPC cost prevented me
from
> > > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it
> did
> > > yesterday; VDC is lightweight and initialization is infrequent.
> > > 
> > > how about this, which is what i would favor if IPC weren't involved:
> > > 
> > > class VideoDecodeAccelerator::Config {
> > >   VideoDecoderConfig decoder_config_;
> > >   // other stuff as it shows up
> > > }
> > 
> > (1) Not every method calling Initialize() has VDC easily available, what to
do
> > in rtc_video_decoder.cc ?
> > (2) Do you prefer passing the whole VDC over IPC even if we use only 2
fields,
> > namely, profile and is_encrypted? Or shall there be another structure
> dedicated
> > just for IPC? 
> 
> (1) good question -- i don't know.  "whole VDC" doesn't bother me, though, in
> this case.  but if not every use of VDA has one, well, that's almost the same
> problem that makes me not like "VDCLite" - manually thunking one config
> structure into another can go wrong easily.
> 
> (2) ignoring (1) for a moment, i'd favor all of VDC.  otherwise, we'll end up
> adding "just one more field" to the interim structure, and/or forgetting to
> initialize it elsewhere.
> 
> on balance, creating VDA::Config with just |profile_| and |is_encrypted_|
seems
> like the best of a bad set of options.  might as well provide only the fields
> that have meaningful data, and centralize the thunk from VDC in the VDA::C
> constructor.

I am okay with just renaming the current StreamParameters to Config, which
contains only |profile_| and |is_encrypted_|.

[Tima Vaisburd, 2015-12-02 22:56:58]

Please take another look.

https://codereview.chromium.org/1485043002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:638: DLOG(ERROR) <<
"Encrypted streams are not supported for this VDA";
On 2015/12/02 06:48:18, xhwang wrote:
> Here and below, how about
> 
> NOTREACHED() << "Encrypted.....";
> 
> I like the "return false" below. But we shouldn't get here in the first place.

Done.

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 18:58:34, xhwang wrote:
> On 2015/12/02 18:29:47, liberato wrote:
> > On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> > > On 2015/12/02 16:18:50, liberato wrote:
> > > > On 2015/12/02 06:48:18, xhwang wrote:
> > > > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't
see
> > why
> > > we
> > > > > want to distinguish this from VideoDecoderConfig. If not for the sake
of
> > > > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > > > VDA::Initialize() to avoid reinventing a new type for the
initialization
> > > > config.
> > > > > VDA::Config seems a perfect tradeoff to me...
> > > > 
> > > > i don't think that VDA::Config will always be a subset of VDC.  we've
had
> > > > several discussions lately (e.g., full screen surface ID) that suggest
it
> > > won't
> > > > be.  since updating all the VDA implementations is no fun, i believe
that
> we
> > > > definitely need some VDA-specific config type, rather than sending the
VDC
> > > > verbatim. +1 xhwang's VDA::Config name.
> > > > 
> > > > i also really don't like 'shadow types', but the IPC cost prevented me
> from
> > > > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as
it
> > did
> > > > yesterday; VDC is lightweight and initialization is infrequent.
> > > > 
> > > > how about this, which is what i would favor if IPC weren't involved:
> > > > 
> > > > class VideoDecodeAccelerator::Config {
> > > >   VideoDecoderConfig decoder_config_;
> > > >   // other stuff as it shows up
> > > > }
> > > 
> > > (1) Not every method calling Initialize() has VDC easily available, what
to
> do
> > > in rtc_video_decoder.cc ?
> > > (2) Do you prefer passing the whole VDC over IPC even if we use only 2
> fields,
> > > namely, profile and is_encrypted? Or shall there be another structure
> > dedicated
> > > just for IPC? 
> > 
> > (1) good question -- i don't know.  "whole VDC" doesn't bother me, though,
in
> > this case.  but if not every use of VDA has one, well, that's almost the
same
> > problem that makes me not like "VDCLite" - manually thunking one config
> > structure into another can go wrong easily.
> > 
> > (2) ignoring (1) for a moment, i'd favor all of VDC.  otherwise, we'll end
up
> > adding "just one more field" to the interim structure, and/or forgetting to
> > initialize it elsewhere.
> > 
> > on balance, creating VDA::Config with just |profile_| and |is_encrypted_|
> seems
> > like the best of a bad set of options.  might as well provide only the
fields
> > that have meaningful data, and centralize the thunk from VDC in the VDA::C
> > constructor.
> 
> I am okay with just renaming the current StreamParameters to Config, which
> contains only |profile_| and |is_encrypted_|.

Done.

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 18:29:47, liberato wrote:
> On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> > On 2015/12/02 16:18:50, liberato wrote:
> > > On 2015/12/02 06:48:18, xhwang wrote:
> > > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see
> why
> > we
> > > > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > > VDA::Initialize() to avoid reinventing a new type for the initialization
> > > config.
> > > > VDA::Config seems a perfect tradeoff to me...
> > > 
> > > i don't think that VDA::Config will always be a subset of VDC.  we've had
> > > several discussions lately (e.g., full screen surface ID) that suggest it
> > won't
> > > be.  since updating all the VDA implementations is no fun, i believe that
we
> > > definitely need some VDA-specific config type, rather than sending the VDC
> > > verbatim. +1 xhwang's VDA::Config name.
> > > 
> > > i also really don't like 'shadow types', but the IPC cost prevented me
from
> > > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it
> did
> > > yesterday; VDC is lightweight and initialization is infrequent.
> > > 
> > > how about this, which is what i would favor if IPC weren't involved:
> > > 
> > > class VideoDecodeAccelerator::Config {
> > >   VideoDecoderConfig decoder_config_;
> > >   // other stuff as it shows up
> > > }
> > 
> > (1) Not every method calling Initialize() has VDC easily available, what to
do
> > in rtc_video_decoder.cc ?
> > (2) Do you prefer passing the whole VDC over IPC even if we use only 2
fields,
> > namely, profile and is_encrypted? Or shall there be another structure
> dedicated
> > just for IPC? 
> 
> (1) good question -- i don't know.  "whole VDC" doesn't bother me, though, in
> this case.  but if not every use of VDA has one, well, that's almost the same
> problem that makes me not like "VDCLite" - manually thunking one config
> structure into another can go wrong easily.
> 
> (2) ignoring (1) for a moment, i'd favor all of VDC.  otherwise, we'll end up
> adding "just one more field" to the interim structure, and/or forgetting to
> initialize it elsewhere.
> 
> on balance, creating VDA::Config with just |profile_| and |is_encrypted_|
seems
> like the best of a bad set of options.  might as well provide only the fields
> that have meaningful data, and centralize the thunk from VDC in the VDA::C
> constructor.

This is what I ended up doing in this PS, although maybe we can make fake
VideoDecoderConfig in this case, similar to this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:66: bool is_encrypted;
On 2015/12/02 16:18:50, liberato wrote:
> On 2015/12/02 06:48:18, xhwang wrote:
> > Having multiple constructors is actually the perfect case to use non-static
> > class member initializers. That way, you  have consistent default values and
> > only overwrite members with non-default values in the initialization list.
> > 
> > Just in case it's not clear, I meant having default values here for both
> > |profile| and |is_encrypted|.
> > 
> > Imagine what if you have a typo in the following code. It's hard to find it
> out.
> > 
> > VideoDecodeAccelerator::StreamParameters::StreamParameters()
> >   : profile(VIDEO_CODEC_PROFILE_UNKNOWN), is_encrypted(false) {}
> > 
> > VideoDecodeAccelerator::StreamParameters::StreamParameters(VideoCodecProfile
> p)
> >    : profile(p), is_encrypted(true) {}
> 
> +1

Done.

[Tima Vaisburd, 2015-12-02 23:00:36]

timav@chromium.org changed reviewers:
+ bbudge@chromium.org

[Tima Vaisburd, 2015-12-02 23:00:37]

Adding bbudge@ as content/renderer/pepper/ owner

[xhwang, 2015-12-02 23:04:30]

lgtm % one tiny nit

https://codereview.chromium.org/1485043002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/100001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.h:68: // one of them
succeeds for given |params.profile|. Send the |init_done_msg|
update this comment

[sandersd (OOO until July 31), 2015-12-02 23:33:28]

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT StreamParameters
{
On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> On 2015/12/02 16:18:50, liberato wrote:
> > On 2015/12/02 06:48:18, xhwang wrote:
> > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see
why
> we
> > > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > VDA::Initialize() to avoid reinventing a new type for the initialization
> > config.
> > > VDA::Config seems a perfect tradeoff to me...
> > 
> > i don't think that VDA::Config will always be a subset of VDC.  we've had
> > several discussions lately (e.g., full screen surface ID) that suggest it
> won't
> > be.  since updating all the VDA implementations is no fun, i believe that we
> > definitely need some VDA-specific config type, rather than sending the VDC
> > verbatim. +1 xhwang's VDA::Config name.
> > 
> > i also really don't like 'shadow types', but the IPC cost prevented me from
> > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it
did
> > yesterday; VDC is lightweight and initialization is infrequent.
> > 
> > how about this, which is what i would favor if IPC weren't involved:
> > 
> > class VideoDecodeAccelerator::Config {
> >   VideoDecoderConfig decoder_config_;
> >   // other stuff as it shows up
> > }
> 
> (1) Not every method calling Initialize() has VDC easily available, what to do
> in rtc_video_decoder.cc ?
> (2) Do you prefer passing the whole VDC over IPC even if we use only 2 fields,
> namely, profile and is_encrypted? Or shall there be another structure
dedicated
> just for IPC? 

I wanted to add my late $0.02 here because this interface is slowly mutating
into VDAv2. In VDAv2, I plan to use VideoDecoderConfig. Note that the data
passed over IPC can still be only a subset.

It will be overall easier for me if that's what happened in this CL ;-)

[Tima Vaisburd, 2015-12-02 23:42:20]

On 2015/12/02 23:33:28, sandersd wrote:
>
https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
> File media/video/video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
> media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT
StreamParameters
> {
> On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> > On 2015/12/02 16:18:50, liberato wrote:
> > > On 2015/12/02 06:48:18, xhwang wrote:
> > > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't see
> why
> > we
> > > > want to distinguish this from VideoDecoderConfig. If not for the sake of
> > > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > > VDA::Initialize() to avoid reinventing a new type for the initialization
> > > config.
> > > > VDA::Config seems a perfect tradeoff to me...
> > > 
> > > i don't think that VDA::Config will always be a subset of VDC.  we've had
> > > several discussions lately (e.g., full screen surface ID) that suggest it
> > won't
> > > be.  since updating all the VDA implementations is no fun, i believe that
we
> > > definitely need some VDA-specific config type, rather than sending the VDC
> > > verbatim. +1 xhwang's VDA::Config name.
> > > 
> > > i also really don't like 'shadow types', but the IPC cost prevented me
from
> > > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as it
> did
> > > yesterday; VDC is lightweight and initialization is infrequent.
> > > 
> > > how about this, which is what i would favor if IPC weren't involved:
> > > 
> > > class VideoDecodeAccelerator::Config {
> > >   VideoDecoderConfig decoder_config_;
> > >   // other stuff as it shows up
> > > }
> > 
> > (1) Not every method calling Initialize() has VDC easily available, what to
do
> > in rtc_video_decoder.cc ?
> > (2) Do you prefer passing the whole VDC over IPC even if we use only 2
fields,
> > namely, profile and is_encrypted? Or shall there be another structure
> dedicated
> > just for IPC? 
> 
> I wanted to add my late $0.02 here because this interface is slowly mutating
> into VDAv2. In VDAv2, I plan to use VideoDecoderConfig. Note that the data
> passed over IPC can still be only a subset.
> 
> It will be overall easier for me if that's what happened in this CL ;-)

Okay, I'm going to implement liberato@'s suggestion, i.e.

class VideoDecodeAccelerator::Config {
  VideoDecoderConfig vdc_;
};

Do stop me if you do not agree. I would also pass all the VDC params over IPC.

[xhwang, 2015-12-02 23:59:24]

On 2015/12/02 23:42:20, Tima Vaisburd wrote:
> On 2015/12/02 23:33:28, sandersd wrote:
> >
>
https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
> > File media/video/video_decode_accelerator.h (right):
> > 
> >
>
https://codereview.chromium.org/1485043002/diff/80001/media/video/video_decod...
> > media/video/video_decode_accelerator.h:57: struct MEDIA_EXPORT
> StreamParameters
> > {
> > On 2015/12/02 18:02:15, Tima Vaisburd wrote:
> > > On 2015/12/02 16:18:50, liberato wrote:
> > > > On 2015/12/02 06:48:18, xhwang wrote:
> > > > > IMHO "Parameters" doesn't add any value to the name.  Also, I don't
see
> > why
> > > we
> > > > > want to distinguish this from VideoDecoderConfig. If not for the sake
of
> > > > > additional IPC work, I'd like to send VideoDecoderConfig directly to
> > > > > VDA::Initialize() to avoid reinventing a new type for the
initialization
> > > > config.
> > > > > VDA::Config seems a perfect tradeoff to me...
> > > > 
> > > > i don't think that VDA::Config will always be a subset of VDC.  we've
had
> > > > several discussions lately (e.g., full screen surface ID) that suggest
it
> > > won't
> > > > be.  since updating all the VDA implementations is no fun, i believe
that
> we
> > > > definitely need some VDA-specific config type, rather than sending the
VDC
> > > > verbatim. +1 xhwang's VDA::Config name.
> > > > 
> > > > i also really don't like 'shadow types', but the IPC cost prevented me
> from
> > > > objecting yesterday.  this morning, the IPC cost doesn't seem so bad as
it
> > did
> > > > yesterday; VDC is lightweight and initialization is infrequent.
> > > > 
> > > > how about this, which is what i would favor if IPC weren't involved:
> > > > 
> > > > class VideoDecodeAccelerator::Config {
> > > >   VideoDecoderConfig decoder_config_;
> > > >   // other stuff as it shows up
> > > > }
> > > 
> > > (1) Not every method calling Initialize() has VDC easily available, what
to
> do
> > > in rtc_video_decoder.cc ?
> > > (2) Do you prefer passing the whole VDC over IPC even if we use only 2
> fields,
> > > namely, profile and is_encrypted? Or shall there be another structure
> > dedicated
> > > just for IPC? 
> > 
> > I wanted to add my late $0.02 here because this interface is slowly mutating
> > into VDAv2. In VDAv2, I plan to use VideoDecoderConfig. Note that the data
> > passed over IPC can still be only a subset.
> > 
> > It will be overall easier for me if that's what happened in this CL ;-)
> 
> Okay, I'm going to implement liberato@'s suggestion, i.e.
> 
> class VideoDecodeAccelerator::Config {
>   VideoDecoderConfig vdc_;
> };
> 
> Do stop me if you do not agree. I would also pass all the VDC params over IPC.

Dan: In VDAv2, we'll probably use Mojo instead of IPC to send VDC. And we
already have code to do that [1]. Given that, I don't feel we should waste time
implementing IPC support for VDC. We could pass VDC to VDA::Initialize() but
only send a subset of VDC over IPC, but I don't see how to implement that
cleanly without hacks. For example, how do we construct VDC in the GPU process
with only the subset of values? Also, that doesn't seem to belong to this CL
(which is really just trying to send is_encrypted over).

Can we land this current CL as is, and then revisit this issue when we really
need VDC in VDA::Initialize()?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/medi...

[sandersd (OOO until July 31), 2015-12-03 00:10:03]

> Dan: In VDAv2, we'll probably use Mojo instead of IPC to send VDC. And we
> already have code to do that [1]. Given that, I don't feel we should waste
time
> implementing IPC support for VDC. We could pass VDC to VDA::Initialize() but
> only send a subset of VDC over IPC, but I don't see how to implement that
> cleanly without hacks. For example, how do we construct VDC in the GPU process
> with only the subset of values? Also, that doesn't seem to belong to this CL
> (which is really just trying to send is_encrypted over).
> 
> Can we land this current CL as is, and then revisit this issue when we really
> need VDC in VDA::Initialize()?

I'm fine with that outcome too; I just wanted to mention that there are good
reasons to use VDC and that that is likely the goal state.

[Tima Vaisburd, 2015-12-03 00:16:56]

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

[Tima Vaisburd, 2015-12-03 00:16:57]

Added ccameron@ as content/common/gpu/ owner

[bbudge, 2015-12-03 02:49:26]

content/renderer/pepper LGTM

[ccameron, 2015-12-03 19:14:01]

content/common/gpu/ LGTM

[Tima Vaisburd, 2015-12-03 19:17:45]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-12-03 19:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
liberato@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1485043002/#ps120001
(title: "Fixed compilation and a comment")

[commit-bot: I haz the power, 2015-12-03 19:19:51]

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

[commit-bot: I haz the power, 2015-12-03 21:42:53]

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

[commit-bot: I haz the power, 2015-12-03 21:42:54]

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

[Tima Vaisburd, 2015-12-03 22:00:16]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-12-03 22:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
ccameron@chromium.org, dcheng@chromium.org, liberato@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1485043002/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2015-12-03 22:00:34]

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

[commit-bot: I haz the power, 2015-12-04 00:08:42]

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

[commit-bot: I haz the power, 2015-12-04 00:08:43]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[Tima Vaisburd, 2015-12-04 00:09:57]

The CQ bit was checked by timav@chromium.org

[Pawel Osciak, 2015-12-04 00:11:07]

The CQ bit was unchecked by posciak@chromium.org

[Pawel Osciak, 2015-12-04 00:48:59]

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:24: #include
"media/base/video_decoder_config.h"
Perhaps this include would no longer be required?

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/dxva_video_decode_accelerator_win.cc:638: NOTREACHED()
<< "Encrypted streams are not supported for this VDA";
Should this be a NOTREACHED, or just a DVLOG() if we are falling back to
software decoder in these cases? Would this be a normal case or a result of a
bug to call this with is_encrypted=true? If the latter, would we know if VDA
supported encrypted content before attempting to Initialize() in order to avoid
calling it?

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:370: LOG(ERROR) << "HW
video decode not available for profile " << config.profile;
This may also happen if is_encrypted is not supported, so we should probably
update this. Maybe even have a Config::ToString() (up to you though)?

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.h:68: // one of them
succeeds for given |config.profile|. Send the |init_done_msg|
We should probably say for given config, as we may fail even if config.profile
is supported, but is_encrypted is not, for example.

https://codereview.chromium.org/1485043002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/renderer/peppe...
content/renderer/pepper/video_decoder_shim.cc:877: NOTREACHED() << "Encrypted
streams are not supported for this VDA";
s/for this VDA//

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
media/video/video_decode_accelerator.h:56: // Congig structure contains
parameters required for the VDA initialization.
s/Congig/Config/

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
media/video/video_decode_accelerator.h:127: //  |params| are the initialization
parameters.
Please update this documentation.

[xhwang, 2015-12-04 01:11:50]

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/dxva_video_decode_accelerator_win.cc:638: NOTREACHED()
<< "Encrypted streams are not supported for this VDA";
On 2015/12/04 00:48:58, Pawel Osciak wrote:
> Should this be a NOTREACHED, or just a DVLOG() if we are falling back to
> software decoder in these cases? Would this be a normal case or a result of a
> bug to call this with is_encrypted=true? If the latter, would we know if VDA
> supported encrypted content before attempting to Initialize() in order to
avoid
> calling it?

Currently we are only initializing VDA on Android for encrypted stream. This is
to prevent unnecessary delay and complexity on other platforms. We can always
change this in the future when we have more VDA supporting encrypted streams.

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...

[Tima Vaisburd, 2015-12-04 01:36:51]

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:24: #include
"media/base/video_decoder_config.h"
On 2015/12/04 00:48:58, Pawel Osciak wrote:
> Perhaps this include would no longer be required?

Done.

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:370: LOG(ERROR) << "HW
video decode not available for profile " << config.profile;
On 2015/12/04 00:48:58, Pawel Osciak wrote:
> This may also happen if is_encrypted is not supported, so we should probably
> update this. Maybe even have a Config::ToString() (up to you though)?

Added " with encryption" inline.

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/140001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.h:68: // one of them
succeeds for given |config.profile|. Send the |init_done_msg|
On 2015/12/04 00:48:58, Pawel Osciak wrote:
> We should probably say for given config, as we may fail even if config.profile
> is supported, but is_encrypted is not, for example.

Done.

https://codereview.chromium.org/1485043002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/1485043002/diff/140001/content/renderer/peppe...
content/renderer/pepper/video_decoder_shim.cc:877: NOTREACHED() << "Encrypted
streams are not supported for this VDA";
On 2015/12/04 00:48:58, Pawel Osciak wrote:
> s/for this VDA//

Done.

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
media/video/video_decode_accelerator.h:56: // Congig structure contains
parameters required for the VDA initialization.
On 2015/12/04 00:48:59, Pawel Osciak wrote:
> s/Congig/Config/

Done.

https://codereview.chromium.org/1485043002/diff/140001/media/video/video_deco...
media/video/video_decode_accelerator.h:127: //  |params| are the initialization
parameters.
On 2015/12/04 00:48:59, Pawel Osciak wrote:
> Please update this documentation.

Done.

[Pawel Osciak, 2015-12-04 01:43:10]

lgtm

[Tima Vaisburd, 2015-12-04 01:49:54]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-12-04 01:49:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
ccameron@chromium.org, dcheng@chromium.org, liberato@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1485043002/#ps160001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-12-04 01:54:53]

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

[commit-bot: I haz the power, 2015-12-04 04:11:41]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-04 04:13:03]

Description was changed from

==========
Passed is_encrypted parameter to the VDA initialization.

The VDA on Android (AVDA) requires this info to properly
implement CDM. This CL changes the method
VideoDecodeAccelerator::Initialize() to accept the structure
holding VDA initialization parameters. There are two parameters:
video codec profile and the is_encrypted flag. For Android both
parameters are propagated to GPU process.

BUG=542417
==========

to

==========
Passed is_encrypted parameter to the VDA initialization.

The VDA on Android (AVDA) requires this info to properly
implement CDM. This CL changes the method
VideoDecodeAccelerator::Initialize() to accept the structure
holding VDA initialization parameters. There are two parameters:
video codec profile and the is_encrypted flag. For Android both
parameters are propagated to GPU process.

BUG=542417

Committed: https://crrev.com/e32b7be080461db66d13fd54b4e312568efe03e9
Cr-Commit-Position: refs/heads/master@{#363127}
==========

[commit-bot: I haz the power, 2015-12-04 04:13:04]

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