Plumb decoded video pixel format from GPU process to renderer.

Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292
Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Committed: https://crrev.com/86afa7ffd8f64c9829381e0661f94423873b9478
Cr-Original-Commit-Position: refs/heads/master@{#396634}
Cr-Commit-Position: refs/heads/master@{#398196}

[jbauman, 2016-05-03 21:52:27]

Description was changed from

==========
Plumb pixel format from GPU.

BUG=
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292
==========

[jbauman, 2016-05-03 21:52:35]

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

[miu, 2016-05-05 20:35:36]

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

[miu, 2016-05-05 20:35:37]

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

https://codereview.chromium.org/1942123002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:140: static scoped_refptr<VideoFrame>
WrapNativeTextures(
If you're going to add this, please delete WrapNativeTexture() and
WrapYUV420NativeTextures() since they would now be redundant.

[jbauman, 2016-05-12 19:25:19]

PTAL.

On 2016/05/05 20:35:37, miu wrote:
> https://codereview.chromium.org/1942123002/diff/1/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
https://codereview.chromium.org/1942123002/diff/1/media/base/video_frame.h#ne...
> media/base/video_frame.h:140: static scoped_refptr<VideoFrame>
> WrapNativeTextures(
> If you're going to add this, please delete WrapNativeTexture() and
> WrapYUV420NativeTextures() since they would now be redundant.

VideoFrame changes were moved to another patch (and committed).

[miu, 2016-05-12 20:03:02]

lgtm % minor things:

https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:182: return
picture_buffer_format_;
Order-of-operations consideration: Should we DCHECK_NE(PIXEL_FORMAT_UNKNOWN,
picture_buffer_format_) to ensure this isn't called before the format is known?
Or, is it normal for the caller to call GetOutputFormat() and expects an
"UNKNOWN" result before picture buffers are provided?

https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:229:
DCHECK(CalledOnValidThread());
Robustness consideration: Should we DCHECK() that the dimensions and format do
not change? (Could this method be called more than once?)

https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.h:99: VideoPixelFormat
picture_buffer_format_;
This should have default initialization (PIXEL_FORMAT_UNKNOWN in ctor
initializer list?).

[jbauman, 2016-05-13 01:10:50]

On 2016/05/12 20:03:02, miu wrote:
> lgtm % minor things:
> 
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:182: return
> picture_buffer_format_;
> Order-of-operations consideration: Should we DCHECK_NE(PIXEL_FORMAT_UNKNOWN,
> picture_buffer_format_) to ensure this isn't called before the format is
known?
> Or, is it normal for the caller to call GetOutputFormat() and expects an
> "UNKNOWN" result before picture buffers are provided?

UNKNOWN is (for the moment at least) an allowed pixel format to be returned. It
means that the consumer has to pick either XRGB or ARGB, depending on the
expected opacity of the frame. There's also not much point in a consumer calling
GetOutputFormat before OnProvidePictureBuffers is called.
> 
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:229:
> DCHECK(CalledOnValidThread());
> Robustness consideration: Should we DCHECK() that the dimensions and format do
> not change? (Could this method be called more than once?)
> 
Done for format. Dimensions are allowed to change.

>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> File media/gpu/ipc/client/gpu_video_decode_accelerator_host.h (right):
> 
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> media/gpu/ipc/client/gpu_video_decode_accelerator_host.h:99: VideoPixelFormat
> picture_buffer_format_;
> This should have default initialization (PIXEL_FORMAT_UNKNOWN in ctor
> initializer list?).

Done.

[miu, 2016-05-13 22:39:29]

On 2016/05/13 01:10:50, jbauman wrote:
> On 2016/05/12 20:03:02, miu wrote:
> > lgtm % minor things:
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:182: return
> > picture_buffer_format_;
> > Order-of-operations consideration: Should we DCHECK_NE(PIXEL_FORMAT_UNKNOWN,
> > picture_buffer_format_) to ensure this isn't called before the format is
> known?
> > Or, is it normal for the caller to call GetOutputFormat() and expects an
> > "UNKNOWN" result before picture buffers are provided?
> 
> UNKNOWN is (for the moment at least) an allowed pixel format to be returned.
It
> means that the consumer has to pick either XRGB or ARGB, depending on the
> expected opacity of the frame. There's also not much point in a consumer
calling
> GetOutputFormat before OnProvidePictureBuffers is called.
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:229:
> > DCHECK(CalledOnValidThread());
> > Robustness consideration: Should we DCHECK() that the dimensions and format
do
> > not change? (Could this method be called more than once?)
> > 
> Done for format. Dimensions are allowed to change.
> 
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > File media/gpu/ipc/client/gpu_video_decode_accelerator_host.h (right):
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > media/gpu/ipc/client/gpu_video_decode_accelerator_host.h:99:
VideoPixelFormat
> > picture_buffer_format_;
> > This should have default initialization (PIXEL_FORMAT_UNKNOWN in ctor
> > initializer list?).
> 
> Done.

Looks like all the code related to my latest comments makes them "N/A." So,
still lgtm.

[jbauman, 2016-05-13 22:42:45]

On 2016/05/13 22:39:29, miu wrote:
> On 2016/05/13 01:10:50, jbauman wrote:
> > On 2016/05/12 20:03:02, miu wrote:
> > > lgtm % minor things:
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > > File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > > media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:182: return
> > > picture_buffer_format_;
> > > Order-of-operations consideration: Should we
DCHECK_NE(PIXEL_FORMAT_UNKNOWN,
> > > picture_buffer_format_) to ensure this isn't called before the format is
> > known?
> > > Or, is it normal for the caller to call GetOutputFormat() and expects an
> > > "UNKNOWN" result before picture buffers are provided?
> > 
> > UNKNOWN is (for the moment at least) an allowed pixel format to be returned.
> It
> > means that the consumer has to pick either XRGB or ARGB, depending on the
> > expected opacity of the frame. There's also not much point in a consumer
> calling
> > GetOutputFormat before OnProvidePictureBuffers is called.
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > > media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:229:
> > > DCHECK(CalledOnValidThread());
> > > Robustness consideration: Should we DCHECK() that the dimensions and
format
> do
> > > not change? (Could this method be called more than once?)
> > > 
> > Done for format. Dimensions are allowed to change.
> > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > > File media/gpu/ipc/client/gpu_video_decode_accelerator_host.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/20001/media/gpu/ipc/client/gp...
> > > media/gpu/ipc/client/gpu_video_decode_accelerator_host.h:99:
> VideoPixelFormat
> > > picture_buffer_format_;
> > > This should have default initialization (PIXEL_FORMAT_UNKNOWN in ctor
> > > initializer list?).
> > 
> > Done.
> 
> Looks like all the code related to my latest comments makes them "N/A." So,
> still lgtm.

Yeah, posciak@ recommended removing GetOutputFormat() and instead adding the
format as an argument to ProvidePictureBuffers.

[jbauman, 2016-05-17 20:52:19]

posciak@: PTAL

[Pawel Osciak, 2016-05-18 07:32:30]

posciak@chromium.org changed reviewers:
+ liberato@chromium.org, sandersd@chromium.org

[Pawel Osciak, 2016-05-18 07:32:32]

Also adding a few other VDA owners.

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
We may have more than one set of buffers with different formats if PPB() is
called more than once (that's why we do += count to available_pictures_ above).
I'm wondering if we should store format information in PictureBuffer itself...

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.h:199: VideoPixelFormat pixel_format_;
Please add documentation.

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
media/gpu/dxva_video_decode_accelerator_win.cc:1885: share_nv12_textures_ ?
PIXEL_FORMAT_NV12 : PIXEL_FORMAT_UNKNOWN,
Should we report RGBA instead of UNKNOWN in general? Is this specified by DXVA
API? Similarly in other VDAs, perhaps their owners could help us confirm this...

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/v4l2_slice_v...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/v4l2_slice_v...
media/gpu/v4l2_slice_video_decode_accelerator.cc:757:
DCHECK(output_format_fourcc_);
This should not be needed, we'd fail S_FMT for 0.

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.cc:681: PIXEL_FORMAT_UNKNOWN, 1,
requested_pic_size_,
In ToT we have a new kOutputPictureFormat constant in this file, which we should
be using here.

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/video_decode...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/gpu/video_decode...
media/gpu/video_decode_accelerator_unittest.cc:656: if (pixel_format ==
media::PIXEL_FORMAT_UNKNOWN)
This is only needed inside the if() clause in l.670, so should probably be
there.

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

https://codereview.chromium.org/1942123002/diff/100001/media/video/video_deco...
media/video/video_decode_accelerator.h:164: // If format is
PIXEL_FORMAT_UNKNOWN, picture buffers can be treated as
I'm wondering if perhaps this would reflect the current state better:

|format| indicates what format the decoded frames will be produced in by the
VDA, or PIXEL_FORMAT_UNKNOWN if the underlying platform handles this
transparently.

[liberato (no reviews please), 2016-05-18 14:49:27]

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
On 2016/05/18 07:32:31, Pawel Osciak wrote:
> We may have more than one set of buffers with different formats if PPB() is
> called more than once (that's why we do += count to available_pictures_
above).
> I'm wondering if we should store format information in PictureBuffer itself...

+1. the renderer factory memorizes RGBA right now, i think.  would be better to
choose what the PB actually requests.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[jbauman, 2016-05-19 17:52:18]

On 2016/05/18 07:32:32, Pawel Osciak wrote:
> Also adding a few other VDA owners.
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> File media/filters/gpu_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
> We may have more than one set of buffers with different formats if PPB() is
> called more than once (that's why we do += count to available_pictures_
above).
> I'm wondering if we should store format information in PictureBuffer itself...

I've added a DCHECK that this doesn't change. I think it should be like
decoder_texture_target_ in that respect.
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> File media/filters/gpu_video_decoder.h (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> media/filters/gpu_video_decoder.h:199: VideoPixelFormat pixel_format_;
> Please add documentation.
> 
Done.

>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> File media/gpu/dxva_video_decode_accelerator_win.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> media/gpu/dxva_video_decode_accelerator_win.cc:1885: share_nv12_textures_ ?
> PIXEL_FORMAT_NV12 : PIXEL_FORMAT_UNKNOWN,
> Should we report RGBA instead of UNKNOWN in general? Is this specified by DXVA
> API? Similarly in other VDAs, perhaps their owners could help us confirm
this...

For now I think it makes sense to leave it as PIXEL_FORMAT_UNKNOWN, so the
client side can help out by knowing whether the video's opaque.
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/v4l2_slice_v...
> File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/v4l2_slice_v...
> media/gpu/v4l2_slice_video_decode_accelerator.cc:757:
> DCHECK(output_format_fourcc_);
> This should not be needed, we'd fail S_FMT for 0.

Removed.
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/vaapi_video_...
> File media/gpu/vaapi_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/vaapi_video_...
> media/gpu/vaapi_video_decode_accelerator.cc:681: PIXEL_FORMAT_UNKNOWN, 1,
> requested_pic_size_,
> In ToT we have a new kOutputPictureFormat constant in this file, which we
should
> be using here.

Done.
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/video_decode...
> File media/gpu/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/video_decode...
> media/gpu/video_decode_accelerator_unittest.cc:656: if (pixel_format ==
> media::PIXEL_FORMAT_UNKNOWN)
> This is only needed inside the if() clause in l.670, so should probably be
> there.
> 
Done.

>
https://codereview.chromium.org/1942123002/diff/100001/media/video/video_deco...
> File media/video/video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/1942123002/diff/100001/media/video/video_deco...
> media/video/video_decode_accelerator.h:164: // If format is
> PIXEL_FORMAT_UNKNOWN, picture buffers can be treated as
> I'm wondering if perhaps this would reflect the current state better:
> 
> |format| indicates what format the decoded frames will be produced in by the
> VDA, or PIXEL_FORMAT_UNKNOWN if the underlying platform handles this
> transparently.

Done.

[Pawel Osciak, 2016-05-20 10:16:06]

On 2016/05/19 17:52:18, jbauman wrote:
> On 2016/05/18 07:32:32, Pawel Osciak wrote:
> > Also adding a few other VDA owners.
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > File media/filters/gpu_video_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
> > We may have more than one set of buffers with different formats if PPB() is
> > called more than once (that's why we do += count to available_pictures_
> above).
> > I'm wondering if we should store format information in PictureBuffer
itself...
> 
> I've added a DCHECK that this doesn't change. I think it should be like
> decoder_texture_target_ in that respect.

Having multiple PB sets in flight is a common case, could we at have an if() at
least, perhaps also with a TODO/crbug please?

> 
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> > File media/gpu/dxva_video_decode_accelerator_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> > media/gpu/dxva_video_decode_accelerator_win.cc:1885: share_nv12_textures_ ?
> > PIXEL_FORMAT_NV12 : PIXEL_FORMAT_UNKNOWN,
> > Should we report RGBA instead of UNKNOWN in general? Is this specified by
DXVA
> > API? Similarly in other VDAs, perhaps their owners could help us confirm
> this...
> 
> For now I think it makes sense to leave it as PIXEL_FORMAT_UNKNOWN, so the
> client side can help out by knowing whether the video's opaque.

Good point, agreed.

[liberato (no reviews please), 2016-05-20 14:26:09]

On 2016/05/20 10:16:06, Pawel Osciak wrote:
> On 2016/05/19 17:52:18, jbauman wrote:
> > On 2016/05/18 07:32:32, Pawel Osciak wrote:
> > > Also adding a few other VDA owners.
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > File media/filters/gpu_video_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
> > > We may have more than one set of buffers with different formats if PPB()
is
> > > called more than once (that's why we do += count to available_pictures_
> > above).
> > > I'm wondering if we should store format information in PictureBuffer
> itself...
> > 
> > I've added a DCHECK that this doesn't change. I think it should be like
> > decoder_texture_target_ in that respect.
> 
> Having multiple PB sets in flight is a common case, could we at have an if()
at
> least, perhaps also with a TODO/crbug please?

decoder_texture_target_: SGTM.  as an aside, decoder_texture_target_ should also
be per PB.  for example, when Android API limitations require
AndroidVideoDecodeAccelerator to replace the codec mid-playback, it could switch
from external to 2D textures, and then back, to make the codec change seamless
to the user.  maybe include decoder_texture_target_ in the TODO/crbug, since it
makes sense to do them together.

avda and avda dependencies lgtm.

[sandersd (OOO until July 31), 2016-05-20 18:26:08]

media/ lgtm

[jbauman, 2016-05-20 22:50:34]

On 2016/05/20 10:16:06, Pawel Osciak wrote:
> On 2016/05/19 17:52:18, jbauman wrote:
> > On 2016/05/18 07:32:32, Pawel Osciak wrote:
> > > Also adding a few other VDA owners.
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > File media/filters/gpu_video_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
> > > We may have more than one set of buffers with different formats if PPB()
is
> > > called more than once (that's why we do += count to available_pictures_
> > above).
> > > I'm wondering if we should store format information in PictureBuffer
> itself...
> > 
> > I've added a DCHECK that this doesn't change. I think it should be like
> > decoder_texture_target_ in that respect.
> 
> Having multiple PB sets in flight is a common case, could we at have an if()
at
> least, perhaps also with a TODO/crbug please?

Hmm, what should I have an if() for?
> 
> > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> > > File media/gpu/dxva_video_decode_accelerator_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/gpu/dxva_video_d...
> > > media/gpu/dxva_video_decode_accelerator_win.cc:1885: share_nv12_textures_
?
> > > PIXEL_FORMAT_NV12 : PIXEL_FORMAT_UNKNOWN,
> > > Should we report RGBA instead of UNKNOWN in general? Is this specified by
> DXVA
> > > API? Similarly in other VDAs, perhaps their owners could help us confirm
> > this...
> > 
> > For now I think it makes sense to leave it as PIXEL_FORMAT_UNKNOWN, so the
> > client side can help out by knowing whether the video's opaque.
> 
> Good point, agreed.

[jbauman, 2016-05-26 01:27:45]

PTAL.

On 2016/05/20 10:16:06, Pawel Osciak wrote:
> On 2016/05/19 17:52:18, jbauman wrote:
> > On 2016/05/18 07:32:32, Pawel Osciak wrote:
> > > Also adding a few other VDA owners.
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > File media/filters/gpu_video_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942123002/diff/100001/media/filters/gpu_vide...
> > > media/filters/gpu_video_decoder.cc:489: pixel_format_ = format;
> > > We may have more than one set of buffers with different formats if PPB()
is
> > > called more than once (that's why we do += count to available_pictures_
> > above).
> > > I'm wondering if we should store format information in PictureBuffer
> itself...
> > 
> > I've added a DCHECK that this doesn't change. I think it should be like
> > decoder_texture_target_ in that respect.
> 
> Having multiple PB sets in flight is a common case, could we at have an if()
at
> least, perhaps also with a TODO/crbug please?
> 
Done.

[Pawel Osciak, 2016-05-26 08:54:07]

Thank you. lgtm with some remaining comments please.

https://chromiumcodereview.appspot.com/1942123002/diff/200001/content/rendere...
File content/renderer/media/rtc_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1942123002/diff/200001/content/rendere...
content/renderer/media/rtc_video_decoder.cc:356: pixel_format_ =
media::PIXEL_FORMAT_ARGB;
We should probably refuse format change in this class as well.

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/filters/g...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/filters/g...
media/filters/gpu_video_decoder.cc:488: if ((pixel_format_ !=
PIXEL_FORMAT_UNKNOWN) && (pixel_format_ != format)) {
We should probably check for this at the beginning of the method, before
creating textures.

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/gpu/ipc/c...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/gpu/ipc/c...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.h:66: void
OnProvidePictureBuffer(VideoPixelFormat format,
Nit: other classes have this as second argument, perhaps we should keep that for
consistency.

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/gpu/video...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1942123002/diff/200001/media/gpu/video...
media/gpu/video_decode_accelerator_unittest.cc:654: requested_num_of_buffers +=
kExtraPictureBuffers;
We should refuse format change here as well.

[jbauman, 2016-05-27 01:10:52]

jbauman@chromium.org changed reviewers:
+ nasko@chromium.org

[jbauman, 2016-05-27 01:10:53]

nasko@: content/ and media/gpu/ipc/common OWNERS review, please.

[nasko, 2016-05-27 23:02:18]

IPC and content/ LGTM

[jbauman, 2016-05-27 23:03:14]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-05-27 23:03:14]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
sandersd@chromium.org, liberato@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1942123002/#ps210001
(title: " ")

[commit-bot: I haz the power, 2016-05-27 23:03:35]

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

[commit-bot: I haz the power, 2016-05-28 00:02:18]

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

[commit-bot: I haz the power, 2016-05-28 00:02:19]

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

[jbauman, 2016-05-28 01:28:55]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2016-05-28 01:29:11]

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

[commit-bot: I haz the power, 2016-05-28 04:10:21]

Description was changed from

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292
==========

[commit-bot: I haz the power, 2016-05-28 04:10:23]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-05-28 04:11:31]

Description was changed from

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

[commit-bot: I haz the power, 2016-05-28 04:11:33]

Patchset 12 (id:??) landed as
https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}

[Pawel Osciak, 2016-05-31 09:23:17]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/2019333004/ by posciak@chromium.org.

The reason for reverting is: Causing CrOS PFQs to fail due to crbug.com/615993..

[jbauman, 2016-06-06 22:28:38]

Description was changed from

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

[jbauman, 2016-06-07 01:44:16]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-06-07 01:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
posciak@chromium.org, sandersd@chromium.org, miu@chromium.org,
nasko@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1942123002/#ps230001
(title: "fix test on bots")

[commit-bot: I haz the power, 2016-06-07 01:44:37]

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

[commit-bot: I haz the power, 2016-06-07 02:15:02]

Description was changed from

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

[commit-bot: I haz the power, 2016-06-07 02:15:03]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2016-06-07 02:16:41]

Description was changed from

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Cr-Commit-Position: refs/heads/master@{#396634}
==========

to

==========
Plumb decoded video pixel format from GPU process to renderer.

This allows the renderer to correctly interpret NV12 and other pixel formats.

BUG=574292

Committed: https://crrev.com/1ace3b00d9dad163255b8e73deaad53c0a43a79a
Committed: https://crrev.com/86afa7ffd8f64c9829381e0661f94423873b9478
Cr-Original-Commit-Position: refs/heads/master@{#396634}
Cr-Commit-Position: refs/heads/master@{#398196}
==========

[commit-bot: I haz the power, 2016-06-07 02:16:43]

Patchset 13 (id:??) landed as
https://crrev.com/86afa7ffd8f64c9829381e0661f94423873b9478
Cr-Commit-Position: refs/heads/master@{#398196}