Set the type of hardware decoded texture to UNSIGNED_BYTE

Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
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/2711903002
Cr-Commit-Position: refs/heads/master@{#457338}
Committed: https://chromium.googlesource.com/chromium/src/+/5c0ad74ee97e52704cd1516ceecfb1873b0b5884

[qiankun, 2017-02-23 09:20:19]

Description was changed from

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
==========

to

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
CQ_INCLUDE_TRYBOTS=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
==========

[qiankun, 2017-02-23 09:20:43]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 09:20:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiankun, 2017-02-23 09:28:19]

qiankun.miao@intel.com changed reviewers:
+ dongseong.hwang@intel.com, kbr@chromium.org, piman@chromium.org,
reveman@chromium.org, wuchengli@chromium.org, zmo@chromium.org

[qiankun, 2017-02-23 09:28:19]

Hi, reviewers

When I did Copy{Sub}TextureCHROMIUM from video texture to another texture, I
found type info was missing for the video texture. I uploaded this CL to discuss
with you what the correct type should be set for video texture. Can you give
some hits?

https://codereview.chromium.org/2711903002/diff/1/media/gpu/ipc/service/gpu_v...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2711903002/diff/1/media/gpu/ipc/service/gpu_v...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:453: GL_RGBA,
GL_UNSIGNED_BYTE, gfx::Rect());
I am not sure can this type be GL_FLOAT?

[commit-bot: I haz the power, 2017-02-23 11:43:06]

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

[commit-bot: I haz the power, 2017-02-23 11:43:07]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-02-24 01:01:42]

Description was changed from

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
CQ_INCLUDE_TRYBOTS=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

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
CQ_INCLUDE_TRYBOTS=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
==========

[Ken Russell (switch to Gerrit), 2017-02-24 01:01:42]

kbr@chromium.org changed reviewers:
+ hubbe@chromium.org
- piman@chromium.org

[Ken Russell (switch to Gerrit), 2017-02-24 01:06:29]

kbr@chromium.org changed reviewers:
+ jbauman@chromium.org
- dongseong.hwang@intel.com

[Ken Russell (switch to Gerrit), 2017-02-24 01:06:29]

It's a good question. I think these are actually YUV textures on platforms that
support them.

Since this is just metadata for the command buffer I think UNSIGNED_BYTE should
work, but am not sure. Probably jbauman@ or hubbe@ would know better.

Tests are needed for this code path. Do any currently exist?

[wuchengli, 2017-03-01 08:18:42]

I'm not familiar with the code. Removing myself from the reviewer.

[wuchengli, 2017-03-01 08:18:54]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[Ken Russell (switch to Gerrit), 2017-03-01 19:31:59]

Qiankun, what tests pass with this CL applied, or conversely, fail if it is not
applied?

[qiankun, 2017-03-02 01:51:17]

On 2017/03/01 19:31:59, Ken Russell wrote:
> Qiankun, what tests pass with this CL applied, or conversely, fail if it is
not
> applied?

Sorry for late response on this CL due to whole week in-house meeting. This CL
fixed failures under conformance2/textures/video/* on Mac OSX.

Ken, what type of test should I add for this CL, gpu_unittests or other test?

[Ken Russell (switch to Gerrit), 2017-03-02 02:38:47]

kbr@chromium.org changed reviewers:
+ dalecurtis@chromium.org, posciak@chromium.org
- hubbe@chromium.org, jbauman@chromium.org, reveman@chromium.org

[Ken Russell (switch to Gerrit), 2017-03-02 02:38:48]

CC'ing OWNERS. Will follow up with another comment.

https://codereview.chromium.org/2711903002/diff/1/media/gpu/ipc/service/gpu_v...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2711903002/diff/1/media/gpu/ipc/service/gpu_v...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:453: GL_RGBA,
GL_UNSIGNED_BYTE, gfx::Rect());
On 2017/02/23 09:28:19, qiankun wrote:
> I am not sure can this type be GL_FLOAT?

There's ongoing work by aleksandar.stojiljkovic in http://crbug.com/624436 and
related bugs to support 16-bit depth textures. We should be careful not to break
them.

[Ken Russell (switch to Gerrit), 2017-03-02 02:40:13]

On 2017/03/02 01:51:17, qiankun wrote:
> On 2017/03/01 19:31:59, Ken Russell wrote:
> > Qiankun, what tests pass with this CL applied, or conversely, fail if it is
> not
> > applied?
> 
> Sorry for late response on this CL due to whole week in-house meeting.

No problem.

> This CL
> fixed failures under conformance2/textures/video/* on Mac OSX.

Thanks for that information. Please just add it to the CL description, and in
what situations, for example:

Fixes failures of WebGL conformance2/textures/video* tests on macOS when
CopyTextureCHROMIUM is enabled for WebGL 2.0.

> Ken, what type of test should I add for this CL, gpu_unittests or other test?

It'd be difficult to add such a test. The WebGL 2.0 conformance tests are good
enough. (in my opinion)

How long will it be before those tests can be enabled in the mode where they
will act as regression tests?

If the comment I posted above about 16-bit depth textures is addressed, then
this looks good, but you'll need an OWNERS review.

[aleksandar.stojiljkovic, 2017-03-02 12:44:36]

On 2017/03/02 02:40:13, Ken Russell wrote:
> On 2017/03/02 01:51:17, qiankun wrote:
> > On 2017/03/01 19:31:59, Ken Russell wrote:
> > > Qiankun, what tests pass with this CL applied, or conversely, fail if it
is
> > not
> > > applied?
> > 
> > Sorry for late response on this CL due to whole week in-house meeting.
> 
> No problem.
> 
> > This CL
> > fixed failures under conformance2/textures/video/* on Mac OSX.
> 
> Thanks for that information. Please just add it to the CL description, and in
> what situations, for example:
> 
> Fixes failures of WebGL conformance2/textures/video* tests on macOS when
> CopyTextureCHROMIUM is enabled for WebGL 2.0.
> 
> > Ken, what type of test should I add for this CL, gpu_unittests or other
test?
> 
> It'd be difficult to add such a test. The WebGL 2.0 conformance tests are good
> enough. (in my opinion)
> 
> How long will it be before those tests can be enabled in the mode where they
> will act as regression tests?
> 
> If the comment I posted above about 16-bit depth textures is addressed, then
> this looks good, but you'll need an OWNERS review.

We have 16 bit depth video -> float texture support and I see that gpu & browser
tests are passing here.
@kbr, thanks for CC-ing. If needed, no problem to modify something when
submitting performance optimizations related to depth capture.

Don't know if/how this *might* affect potential HDR video decoding to RGB10_A2.
Let's figure it later after work on RGB10_A2 [1] advances.

One concern:
Original change [2] explicitly set 0 for type. Don't know why - all the
calculations under e.g. GLES2Util::ComputeImageDataSizesES3 seems ignored when
type is 0. 
Then, there is a method TextureManager::ExtractTypeFromStorageFormat [3] - could
if be useful here?

[1]
https://github.com/KhronosGroup/WebGL/issues/2274
[2]
https://codereview.chromium.org/24152009
[2]
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag...

[DaleCurtis, 2017-03-02 17:44:02]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org

[DaleCurtis, 2017-03-02 17:44:03]

=>sandersd

[hubbe, 2017-03-02 18:40:06]

On 2017/03/02 12:44:36, aleksandar.stojiljkovic wrote:
> On 2017/03/02 02:40:13, Ken Russell wrote:
> > On 2017/03/02 01:51:17, qiankun wrote:
> > > On 2017/03/01 19:31:59, Ken Russell wrote:
> > > > Qiankun, what tests pass with this CL applied, or conversely, fail if it
> is
> > > not
> > > > applied?
> > > 
> > > Sorry for late response on this CL due to whole week in-house meeting.
> > 
> > No problem.
> > 
> > > This CL
> > > fixed failures under conformance2/textures/video/* on Mac OSX.
> > 
> > Thanks for that information. Please just add it to the CL description, and
in
> > what situations, for example:
> > 
> > Fixes failures of WebGL conformance2/textures/video* tests on macOS when
> > CopyTextureCHROMIUM is enabled for WebGL 2.0.
> > 
> > > Ken, what type of test should I add for this CL, gpu_unittests or other
> test?
> > 
> > It'd be difficult to add such a test. The WebGL 2.0 conformance tests are
good
> > enough. (in my opinion)
> > 
> > How long will it be before those tests can be enabled in the mode where they
> > will act as regression tests?
> > 
> > If the comment I posted above about 16-bit depth textures is addressed, then
> > this looks good, but you'll need an OWNERS review.
> 
> We have 16 bit depth video -> float texture support and I see that gpu &
browser
> tests are passing here.
> @kbr, thanks for CC-ing. If needed, no problem to modify something when
> submitting performance optimizations related to depth capture.
> 
> Don't know if/how this *might* affect potential HDR video decoding to
RGB10_A2.
> Let's figure it later after work on RGB10_A2 [1] advances.
> 
> One concern:
> Original change [2] explicitly set 0 for type. Don't know why - all the
> calculations under e.g. GLES2Util::ComputeImageDataSizesES3 seems ignored when
> type is 0. 
> Then, there is a method TextureManager::ExtractTypeFromStorageFormat [3] -
could
> if be useful here?
> 
> [1]
> https://github.com/KhronosGroup/WebGL/issues/2274
> [2]
> https://codereview.chromium.org/24152009
> [2]
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag...

FYI; right now we decode HDR video to 16-bit float textures.
We might try to optimize that in the future though.

[Ken Russell (switch to Gerrit), 2017-03-06 05:15:05]

On 2017/03/02 18:40:06, hubbe wrote:
> On 2017/03/02 12:44:36, aleksandar.stojiljkovic wrote:
> > On 2017/03/02 02:40:13, Ken Russell wrote:
> > > On 2017/03/02 01:51:17, qiankun wrote:
> > > > On 2017/03/01 19:31:59, Ken Russell wrote:
> > > > > Qiankun, what tests pass with this CL applied, or conversely, fail if
it
> > is
> > > > not
> > > > > applied?
> > > > 
> > > > Sorry for late response on this CL due to whole week in-house meeting.
> > > 
> > > No problem.
> > > 
> > > > This CL
> > > > fixed failures under conformance2/textures/video/* on Mac OSX.
> > > 
> > > Thanks for that information. Please just add it to the CL description, and
> in
> > > what situations, for example:
> > > 
> > > Fixes failures of WebGL conformance2/textures/video* tests on macOS when
> > > CopyTextureCHROMIUM is enabled for WebGL 2.0.
> > > 
> > > > Ken, what type of test should I add for this CL, gpu_unittests or other
> > test?
> > > 
> > > It'd be difficult to add such a test. The WebGL 2.0 conformance tests are
> good
> > > enough. (in my opinion)
> > > 
> > > How long will it be before those tests can be enabled in the mode where
they
> > > will act as regression tests?
> > > 
> > > If the comment I posted above about 16-bit depth textures is addressed,
then
> > > this looks good, but you'll need an OWNERS review.
> > 
> > We have 16 bit depth video -> float texture support and I see that gpu &
> browser
> > tests are passing here.
> > @kbr, thanks for CC-ing. If needed, no problem to modify something when
> > submitting performance optimizations related to depth capture.
> > 
> > Don't know if/how this *might* affect potential HDR video decoding to
> RGB10_A2.
> > Let's figure it later after work on RGB10_A2 [1] advances.
> > 
> > One concern:
> > Original change [2] explicitly set 0 for type. Don't know why - all the
> > calculations under e.g. GLES2Util::ComputeImageDataSizesES3 seems ignored
when
> > type is 0. 
> > Then, there is a method TextureManager::ExtractTypeFromStorageFormat [3] -
> could
> > if be useful here?
> > 
> > [1]
> > https://github.com/KhronosGroup/WebGL/issues/2274
> > [2]
> > https://codereview.chromium.org/24152009
> > [2]
> >
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag...
> 
> FYI; right now we decode HDR video to 16-bit float textures.
> We might try to optimize that in the future though.

Is there a way to query at this point in the code which texture format's being
used by the decoder? Please give Qiankun advice. Thanks.

[Zhenyao Mo, 2017-03-10 00:21:50]

Ping.

[sandersd (OOO until July 31), 2017-03-13 19:53:38]

As for media style and correctness, lgtm.

[qiankun, 2017-03-14 02:31:26]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 02:32:19]

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-03-14 03:54:07]

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

[commit-bot: I haz the power, 2017-03-14 03:54:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[qiankun, 2017-03-14 09:20:37]

Description was changed from

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
CQ_INCLUDE_TRYBOTS=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

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
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
==========

[qiankun, 2017-03-14 09:22:12]

Patchset #2 (id:20001) has been deleted

[qiankun, 2017-03-14 09:24:56]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 09:25:08]

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-03-14 10:55:47]

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

[commit-bot: I haz the power, 2017-03-14 10:55:48]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-03-14 16:13:27]

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

https://codereview.chromium.org/2711903002/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:470: DCHECK(format ==
GL_BGRA_EXT);
Add a DCHECK to make sure GL_UNSIGNED_BYTE is correct here. We need to re-visit
the texture type when we support more textures such as 16-bit depth textures in
future.

[qiankun, 2017-03-14 16:15:11]

On 2017/03/13 19:53:38, sandersd wrote:
> As for media style and correctness, lgtm.

Thanks for reviewing. If we add 16-bit video texture in future, we can revisit
this code.

[qiankun, 2017-03-16 01:36:34]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2017-03-16 01:36:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2711903002/#ps40001
(title: "add DCHECK")

[commit-bot: I haz the power, 2017-03-16 01:37:09]

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-03-16 03:32:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489628194974220,
"parent_rev": "5a22ca4cf2ae7796f308999cfdaa0ae8cd5705b8", "commit_rev":
"5c0ad74ee97e52704cd1516ceecfb1873b0b5884"}

[commit-bot: I haz the power, 2017-03-16 03:33:48]

Description was changed from

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
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

==========
Set the type of hardware decoded texture to UNSIGNED_BYTE

We need type info when copying video texture to another texture. 0 is
not a valid texture type. 0 type causes DoCopySubTextureCHROMIUM to
fail.

Format/internalformat info was added in the following two CLs:
https://codereview.chromium.org/775863005
https://codereview.chromium.org/759573002

BUG=612542
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/2711903002
Cr-Commit-Position: refs/heads/master@{#457338}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c0ad74ee97e52704cd1516ceecf...
==========

[commit-bot: I haz the power, 2017-03-16 03:33:49]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5c0ad74ee97e52704cd1516ceecf...