Add |texture_target_| and |pixel_format_| to media::PictureBuffer

Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2881553002
Cr-Commit-Position: refs/heads/master@{#473236}
Committed: https://chromium.googlesource.com/chromium/src/+/d264e4712c7c599d78ca2d615b692f1a9f68e325

[Chandan, 2017-05-11 09:56:19]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 09:57:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Chandan, 2017-05-11 10:50:57]

Description was changed from

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

Picture buffers can have different formats, which may be necessary
if the decoder has to change behavior in some circumstances.
This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.

BUG=614789
==========

to

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
==========

[commit-bot: I haz the power, 2017-05-11 11:35:22]

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

[commit-bot: I haz the power, 2017-05-11 11:35:22]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-11 11:40:06]

c.padhi@samsung.com changed reviewers:
+ jbauman@chromium.org

[Chandan, 2017-05-11 11:40:07]

PTAL. Thank you.

[Chandan, 2017-05-11 11:44:29]

c.padhi@samsung.com changed reviewers:
+ liberato@chromium.org, posciak@chromium.org, sandersd@chromium.org

[Chandan, 2017-05-11 11:44:29]

+ owners

[liberato (no reviews please), 2017-05-11 16:24:40]

out of curiousity, are you planning to actually switch picture buffer formats
mid-stream?  if so, in which decoder(s)?

thanks
-fl

https://codereview.chromium.org/2881553002/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2881553002/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:571:
vda_->AssignPictureBuffers(picture_buffers);
the picture buffer received by GPU clients will not have the texture target nor
format filled in properly; they're not sent via IPC in
gpu_video_decode_accelerator_host.cc (AssignPictureBuffers).

it works as written, since nothing expects that those fields exist currently. 
it's still a little unexpected.

to be fair, |size| isn't sent in the existing code, either.

would you mind adding all three to the IPC?  it's a very rare message, so the
extra IPC overhead to minimize surprises seems worth it to me.

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h#newco...
media/video/picture.h:70: uint32_t texture_target_;
this seems to be left uninitialized in some constructors.

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h#newco...
media/video/picture.h:71: VideoPixelFormat pixel_format_;
same here.

[Chandan, 2017-05-12 13:37:56]

Description was changed from

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
==========

to

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Chandan, 2017-05-12 13:38:14]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 13:38:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Chandan, 2017-05-12 13:47:51]

https://codereview.chromium.org/2881553002/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2881553002/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:571:
vda_->AssignPictureBuffers(picture_buffers);
On 2017/05/11 16:24:40, liberato wrote:
> the picture buffer received by GPU clients will not have the texture target
nor
> format filled in properly; they're not sent via IPC in
> gpu_video_decode_accelerator_host.cc (AssignPictureBuffers).
> 
> it works as written, since nothing expects that those fields exist currently. 
> it's still a little unexpected.
> 
> to be fair, |size| isn't sent in the existing code, either.
> 
> would you mind adding all three to the IPC?  it's a very rare message, so the
> extra IPC overhead to minimize surprises seems worth it to me.

Sure. I have added these fields to the IPC. However, they are unused on the
client side. Some work left there.
I feel we could have restricted this CL to only adding pixel_format and
texture_target to PictureBuffer.

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h#newco...
media/video/picture.h:70: uint32_t texture_target_;
On 2017/05/11 16:24:40, liberato wrote:
> this seems to be left uninitialized in some constructors.

Done.

https://codereview.chromium.org/2881553002/diff/1/media/video/picture.h#newco...
media/video/picture.h:71: VideoPixelFormat pixel_format_;
On 2017/05/11 16:24:40, liberato wrote:
> same here.

Done.

[Chandan, 2017-05-12 13:51:15]

On 2017/05/11 16:24:40, liberato wrote:
> out of curiousity, are you planning to actually switch picture buffer formats
> mid-stream?  if so, in which decoder(s)?
> 
> thanks
> -fl

Not really. I came across the bug 614789 and hence this attempt to make some
contribution from my side.
PTAL @PS2. Thank you.

[commit-bot: I haz the power, 2017-05-12 15:21:51]

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

[commit-bot: I haz the power, 2017-05-12 15:21:52]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-12 21:26:42]

thanks
-fl

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/common/me...
File media/gpu/ipc/common/media_messages.h (right):

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:83: std::vector<uint32_t>,                
        /* Texture target */
i don't think that target, format, size need to be vectors.

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:484:
buffers.push_back(PictureBuffer(buffer_ids[i], texture_dimensions_,
i had forgotten that |texture_dimensions_| is cached here.  if you'd like to
mirror this approach for format and target, rather than sending via IPC, that
works too.  sorry about the confusion.

if you choose to keep IPC, then |texture_dimensions_| can be removed from
GpuVideoDecoder entirely, i think.

[Chandan, 2017-05-16 10:47:39]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 10:47:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 14:26:00]

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

[commit-bot: I haz the power, 2017-05-16 14:26:01]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-16 14:29:30]

PTAL.

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/common/me...
File media/gpu/ipc/common/media_messages.h (right):

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:83: std::vector<uint32_t>,                
        /* Texture target */
On 2017/05/12 21:26:42, liberato wrote:
> i don't think that target, format, size need to be vectors.

Right. Done.

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2881553002/diff/20001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:484:
buffers.push_back(PictureBuffer(buffer_ids[i], texture_dimensions_,
On 2017/05/12 21:26:42, liberato wrote:
> i had forgotten that |texture_dimensions_| is cached here.  if you'd like to
> mirror this approach for format and target, rather than sending via IPC, that
> works too.  sorry about the confusion.
> 
> if you choose to keep IPC, then |texture_dimensions_| can be removed from
> GpuVideoDecoder entirely, i think.

Done.

[liberato (no reviews please), 2017-05-16 17:25:25]

thanks for taking the time with this.  looks good % one last change.

also, you'll need to add somebody from chrome security for the ipc changes. 
ipc/SECURITY_OWNERS (or 'git cl owners') has a list.

thanks
-fl

https://codereview.chromium.org/2881553002/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2881553002/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:481:
buffers.push_back(PictureBuffer(buffer_ids[i], dimensions,
i think that this should pass in |texture_target| and |format|.  i think that
only this call and the associated unit test use that constructor, so it looks
like a small change.

[Chandan, 2017-05-17 06:45:45]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 06:46:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 08:44:42]

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

[commit-bot: I haz the power, 2017-05-17 08:44:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Chandan, 2017-05-17 12:07:05]

c.padhi@samsung.com changed reviewers:
+ dcheng@chromium.org

[Chandan, 2017-05-17 12:07:06]

PTAL. Thank you.
dcheng@, please review IPC changes.

https://codereview.chromium.org/2881553002/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2881553002/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:481:
buffers.push_back(PictureBuffer(buffer_ids[i], dimensions,
On 2017/05/16 17:25:24, liberato wrote:
> i think that this should pass in |texture_target| and |format|.  i think that
> only this call and the associated unit test use that constructor, so it looks
> like a small change.

Done.

[liberato (no reviews please), 2017-05-17 17:08:57]

lgtm.

thanks
-fl

[jbauman, 2017-05-17 23:40:50]

lgtm

[sandersd (OOO until July 31), 2017-05-17 23:51:50]

lgtm

[Chandan, 2017-05-18 11:52:57]

Thanks for the review.

PTAL.
@Pawel: content/renderer/media/gpu/rtc_video_decoder.h|cc
@dcheng: media/gpu/ipc/common/media_messages.h

[dcheng, 2017-05-18 14:15:41]

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

https://codereview.chromium.org/2881553002/diff/80001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:139: texture_target =
buffers[0].texture_target();
Is it possible (via something malicious) that the buffers don't all have
matching attributes? In particular, size. This code can send a message from the
renderer to the GPU right? Does the GPU service check all these parameters for
correctness?

[Chandan, 2017-05-18 19:38:53]

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

https://codereview.chromium.org/2881553002/diff/80001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:139: texture_target =
buffers[0].texture_target();
On 2017/05/18 14:15:41, dcheng (in AEST) wrote:
> Is it possible (via something malicious) that the buffers don't all have
> matching attributes? In particular, size. This code can send a message from
the
> renderer to the GPU right? Does the GPU service check all these parameters for
> correctness?

Can you pls elaborate a bit on 'matching attributes'?
I assume you are referring to GPU process by 'GPU'. Then, yes.
These extra parameters are passed on to the underlying decoder. 
However, these are unused currently. Their correctness will be checked may be
when we actually switch picture buffer formats mid-stream.
liberato@, your views on this?

[liberato (no reviews please), 2017-05-18 20:07:01]

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

https://codereview.chromium.org/2881553002/diff/80001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:139: texture_target =
buffers[0].texture_target();
On 2017/05/18 19:38:53, Chandan wrote:
> On 2017/05/18 14:15:41, dcheng (in AEST) wrote:
> > Is it possible (via something malicious) that the buffers don't all have
> > matching attributes? In particular, size. This code can send a message from
> the
> > renderer to the GPU right? Does the GPU service check all these parameters
for
> > correctness?
> 
> Can you pls elaborate a bit on 'matching attributes'?
> I assume you are referring to GPU process by 'GPU'. Then, yes.
> These extra parameters are passed on to the underlying decoder. 
> However, these are unused currently. Their correctness will be checked may be
> when we actually switch picture buffer formats mid-stream.
> liberato@, your views on this?

hrm, it's a good point that i hadn't considerd.

i suppose that one could remove it from IPC and fill it in on the GPU side,
based on the most recently requested parameters.  that's what size was doing
before, i think.

if one does that, then we avoid the security issue but still get buffers with
correct metadata on the gpu side.

[Chandan, 2017-05-18 20:36:37]

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

https://codereview.chromium.org/2881553002/diff/80001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:139: texture_target =
buffers[0].texture_target();
On 2017/05/18 20:07:01, liberato wrote:
> On 2017/05/18 19:38:53, Chandan wrote:
> > On 2017/05/18 14:15:41, dcheng (in AEST) wrote:
> > > Is it possible (via something malicious) that the buffers don't all have
> > > matching attributes? In particular, size. This code can send a message
from
> > the
> > > renderer to the GPU right? Does the GPU service check all these parameters
> for
> > > correctness?
> > 
> > Can you pls elaborate a bit on 'matching attributes'?
> > I assume you are referring to GPU process by 'GPU'. Then, yes.
> > These extra parameters are passed on to the underlying decoder. 
> > However, these are unused currently. Their correctness will be checked may
be
> > when we actually switch picture buffer formats mid-stream.
> > liberato@, your views on this?
> 
> hrm, it's a good point that i hadn't considerd.
> 
> i suppose that one could remove it from IPC and fill it in on the GPU side,
> based on the most recently requested parameters.  that's what size was doing
> before, i think.
> 
> if one does that, then we avoid the security issue but still get buffers with
> correct metadata on the gpu side.

Ok. I will remove these 3 extra params from IPC then and cache format, target
similar to what was done for size?

[Chandan, 2017-05-19 10:44:13]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 10:44:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 12:52:36]

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

[commit-bot: I haz the power, 2017-05-19 12:52:37]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-19 12:53:22]

PTAL @PS6. Thank you.

[dcheng, 2017-05-19 14:08:44]

It looks like this change has no IPC reviews anymore, so I'll recuse myself; let
me know if there's something you need me to look at still.

[liberato (no reviews please), 2017-05-19 17:24:32]

lgtm.  thanks for doing this.

-fl

[Chandan, 2017-05-19 17:36:50]

The CQ bit was checked by c.padhi@samsung.com

[Chandan, 2017-05-19 17:36:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2881553002/#ps100001
(title: "removed IPC changes")

[commit-bot: I haz the power, 2017-05-19 17:37:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 17:43:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495215410892760,
"parent_rev": "94a4ca46bbe956a4ae033e7e207c584abe1e3645", "commit_rev":
"d264e4712c7c599d78ca2d615b692f1a9f68e325"}

[commit-bot: I haz the power, 2017-05-19 17:43:51]

Description was changed from

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add |texture_target_| and |pixel_format_| to media::PictureBuffer

This CL moves |decoder_texture_target_| and |pixel_format_| from
{Gpu,RTC}VideoDecoder to media::PictureBuffer.
This will allow different picture buffers to have different formats.

BUG=614789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2881553002
Cr-Commit-Position: refs/heads/master@{#473236}
Committed:
https://chromium.googlesource.com/chromium/src/+/d264e4712c7c599d78ca2d615b69...
==========

[commit-bot: I haz the power, 2017-05-19 17:43:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d264e4712c7c599d78ca2d615b69...