Avoid planar YUV resources when one component EGL images are not supported

Avoid planar YUV resources when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060, 632461
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[Tobias Sargeant, 2016-03-03 21:38:38]

Description was changed from

==========
Avoid sharing planar YUV when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
==========

to

==========
Avoid sharing planar YUV when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Tobias Sargeant, 2016-03-03 21:38:39]

Description was changed from

==========
Avoid sharing planar YUV when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Avoid sharing planar YUV when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Tobias Sargeant, 2016-03-03 21:41:13]

Description was changed from

==========
Avoid sharing planar YUV when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Avoid planar YUV resources when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Tobias Sargeant, 2016-03-03 21:41:44]

tobiasjs@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org, sievers@chromium.org

[no sievers, 2016-03-08 01:14:49]

https://codereview.chromium.org/1761893003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1761893003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3146:
caps.avoid_one_component_egl_images =
nit: The client doesn't know anything about EGL images being used, but we should
rather say that we are forcing RGBA8 mailbox textures or whatever. (Note that we
also don't support mipmaps, and only support GL_TEXTURE_2D).

[Tobias Sargeant, 2016-03-08 12:06:38]

https://codereview.chromium.org/1761893003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1761893003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3146:
caps.avoid_one_component_egl_images =
On 2016/03/08 01:14:49, sievers wrote:
> nit: The client doesn't know anything about EGL images being used, but we
should
> rather say that we are forcing RGBA8 mailbox textures or whatever. (Note that
we
> also don't support mipmaps, and only support GL_TEXTURE_2D).

I renamed this to force_software_yuv_conversion. WDYT?

[dcheng, 2016-03-08 18:33:15]

ipc changes seem reasonable, but i will defer l-g-t-m until the main owners sign
off. just a few random comments in the meantime.

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1253: }
Nit: no braces here to match local convention.

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:272: const
scoped_refptr<media::VideoFrame>& video_frame) {
Nit: this function doesn't appear to take ownership, so consider passing
|video_frame| as a raw pointer.

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:286: size_t bytes_per_row =
ResourceUtil::UncheckedWidthInBytes<size_t>(
Not really related to this CL, but it's hard for me to see why the Unchecked
versions are safe here (I assume they're safe though, since they're already used
elsewhere in this file)

[Tobias Sargeant, 2016-03-09 11:50:58]

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1253: }
On 2016/03/08 18:33:15, dcheng wrote:
> Nit: no braces here to match local convention.

Done.

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:272: const
scoped_refptr<media::VideoFrame>& video_frame) {
On 2016/03/08 18:33:15, dcheng wrote:
> Nit: this function doesn't appear to take ownership, so consider passing
> |video_frame| as a raw pointer.

Done. Also changed the other private functions so that they are consistent.

https://codereview.chromium.org/1761893003/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:286: size_t bytes_per_row =
ResourceUtil::UncheckedWidthInBytes<size_t>(
On 2016/03/08 18:33:15, dcheng wrote:
> Not really related to this CL, but it's hard for me to see why the Unchecked
> versions are safe here (I assume they're safe though, since they're already
used
> elsewhere in this file)

I agree that it is not obvious. This was copying the style from elsewhere in
this file. I assume that it's ok because video_frame must be in a consistent
state, with a correctly sized chunk of memory allocated for the frame data. If
we were going to overflow here, then that should have been impossible.

I couldn't see any advice in the discussion of the CL that introduced the other
instance. cc'ing prashant.n for comment.

[Tobias Sargeant, 2016-03-17 10:23:31]

Patchset #4 (id:60001) has been deleted

[Tobias Sargeant, 2016-03-17 10:23:41]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 10:23:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761893003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761893003/80001

[commit-bot: I haz the power, 2016-03-17 11:37:07]

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

[commit-bot: I haz the power, 2016-03-17 11:37:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tobias Sargeant, 2016-03-23 12:31:13]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-23 12:31:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761893003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761893003/100001

[commit-bot: I haz the power, 2016-03-23 13:47:46]

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

[commit-bot: I haz the power, 2016-03-23 13:47:47]

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

[prashant.n, 2016-03-26 18:57:11]

> I couldn't see any advice in the discussion of the CL that introduced the
other
> instance. cc'ing prashant.n for comment.

Basically unchecked versions are to be used only when

1. we are sure about there would be no overflow occurring OR
2. somewhere in start of the call stack we have used checked versions, so later
we can safely use unchecked versions.

2nd option is recommended.

[Tobias Sargeant, 2016-08-11 15:17:53]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 15:18:14]

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, 2016-08-11 15:18:16]

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

[commit-bot: I haz the power, 2016-08-11 15:18:18]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[Tobias Sargeant, 2016-08-11 15:18:46]

Description was changed from

==========
Avoid planar YUV resources when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Avoid planar YUV resources when one component EGL images are not supported

Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.

BUG=579060,632461
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Tobias Sargeant, 2016-08-11 15:27:31]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 15:28:05]

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, 2016-08-11 15:28:06]

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

[commit-bot: I haz the power, 2016-08-11 15:28:06]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[dcheng, 2016-08-15 18:24:24]

On 2016/03/26 18:57:11, prashant.n wrote:
> > I couldn't see any advice in the discussion of the CL that introduced the
> other
> > instance. cc'ing prashant.n for comment.
> 
> Basically unchecked versions are to be used only when
> 
> 1. we are sure about there would be no overflow occurring OR
> 2. somewhere in start of the call stack we have used checked versions, so
later
> we can safely use unchecked versions.
> 
> 2nd option is recommended.

As a reviewer, how can I tell that the checked versions were used earlier in the
stack?