gpu: Make GPU command decoder know right format of a GPU video texture in Win.

gpu: Make GPU command decoder know right format of a GPU video texture in Win.

DXVAVideoDecodeAccelerator slightly creates BGRA texture while GpuVideoAccelerator reports
RGBA format to GPU command decoder.
This CL lets GPU command decoder know BGRA format in Win.

BUG=438691
Committed: https://crrev.com/5bcfb71ba1d2d3b8f94b5bdb2a19f8cdba58a928
Cr-Commit-Position: refs/heads/master@{#307444}

[dshwang, 2014-12-03 19:33:32]

dongseong.hwang@intel.com changed reviewers:
+ kbr@chromium.org, shannonwoods@chromium.org

[dshwang, 2014-12-03 19:33:33]

@kbr, could you review?
I think we can remove BGRA copy hack in ANGLE after this CL.

[dshwang, 2014-12-03 20:10:39]

After this CL, https://codereview.chromium.org/744713002/ can be re-landed,
because glCopyTexthreCHROMIUM don't use glCopyTexSubImage if source format is
BGRA and target format is RGBA.
Currenly, glCopyTexthreCHROMIUM misunderstands the source format is RGBA.

[Ken Russell (switch to Gerrit), 2014-12-04 01:17:08]

kbr@chromium.org changed reviewers:
+ dalecurtis@chromium.org, miu@chromium.org, scherkus@chromium.org

[Ken Russell (switch to Gerrit), 2014-12-04 01:25:18]

+people from the media team for review

https://codereview.chromium.org/775863005/diff/1/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/775863005/diff/1/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:380: GLenum format =
os_win ? GL_BGRA_EXT : GL_RGBA;
Please define some sort of API that is overridden by
dxva_video_decode_accelerator.cc to fetch this format, so the use of
D3DFMT_X8R8G8B8 / D3DFMT_A8R8G8B8 and GL_BGRA_EXT are close together.

I'm also concerned that this may not be forward compatible with the OpenGL ES
3.0 upgrade. Is GL_BGRA_EXT a valid internal format? According to
https://www.khronos.org/registry/gles/extensions/APPLE/APPLE_texture_format_B...
-- which is the only BGRA extension that was upgraded for ES 3.0 -- it looks
like it needs to be GL_BGRA8_EXT.

Finally, how much testing has this change undergone? It has the potential to
break video playback in many scenarios -- directly on the page, drawn into 2D
and WebGL canvas contexts -- and I'm very concerned that we don't have enough
test coverage to catch regressions in automated fashion. See
http://crbug.com/436578 .

[dshwang, 2014-12-05 12:48:13]

On 2014/12/04 01:25:18, Ken Russell wrote:
> +people from the media team for review

Thank you for reviewing and appointing.
@scherkus, could you review?

> 
>
https://codereview.chromium.org/775863005/diff/1/content/common/gpu/media/gpu...
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/775863005/diff/1/content/common/gpu/media/gpu...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:380: GLenum format =
> os_win ? GL_BGRA_EXT : GL_RGBA;
> Please define some sort of API that is overridden by
> dxva_video_decode_accelerator.cc to fetch this format, so the use of
> D3DFMT_X8R8G8B8 / D3DFMT_A8R8G8B8 and GL_BGRA_EXT are close together.

Yes, done. I add new method to only DXVAVideoDecodeAccelerator, instead of
GpuVideoDecodeAccelerator.
It's because I don't want to touch innocent tons of subclasses of
GpuVideoDecodeAccelerator.

> I'm also concerned that this may not be forward compatible with the OpenGL ES
> 3.0 upgrade. Is GL_BGRA_EXT a valid internal format? According to
>
https://www.khronos.org/registry/gles/extensions/APPLE/APPLE_texture_format_B...
> -- which is the only BGRA extension that was upgraded for ES 3.0 -- it looks
> like it needs to be GL_BGRA8_EXT.

GL_BGRA8_EXT should be used by only glTexStorage*

APPLE_texture_format_BGRA8888 declairs that
    Note that despite TexStorage* being defined in terms of TexImage*, 
    BGRA8_EXT is not accepted as an <internalformat> parameter to TexImage* 
    when the GL is OpenGL ES 3.0.

In addition, Chromium never use GL_RGBA8 for TexImage* also. Chromium uses
GL_RGBA for TexImage* and GL_RGBA8 for TexStorage2D.

> Finally, how much testing has this change undergone? It has the potential to
> break video playback in many scenarios -- directly on the page, drawn into 2D
> and WebGL canvas contexts -- and I'm very concerned that we don't have enough
> test coverage to catch regressions in automated fashion. See
> http://crbug.com/436578 .

I build chrome with proprietary_codecs=1 ffmpeg_branding=Chrome on Win 8.1 and
tests layout tests and webgl_conformance tests
layout tests: fast/canvas and virtual/gpu/fast/canvsa
webgl_conformance: textures/tex-images-and-sub-image-2d-with-*

and test real sites
h.264 (gpu video) : http://browsertests.herokuapp.com/media/h264.html
gpu video to webgl :
http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html
gpu video to canvas 2d : http://html5demos.com/video-canvas

[scherkus (not reviewing), 2014-12-05 16:44:11]

Heads up I'm on vacation so earliest I'll be able to review this is Monday.
On Dec 5, 2014 4:48 AM, <dongseong.hwang@intel.com> wrote:

> On 2014/12/04 01:25:18, Ken Russell wrote:
>
>> +people from the media team for review
>>
>
> Thank you for reviewing and appointing.
> @scherkus, could you review?
>
>
>
> https://codereview.chromium.org/775863005/diff/1/content/
> common/gpu/media/gpu_video_decode_accelerator.cc
>
>> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
>>
>
>
> https://codereview.chromium.org/775863005/diff/1/content/
> common/gpu/media/gpu_video_decode_accelerator.cc#newcode380
>
>> content/common/gpu/media/gpu_video_decode_accelerator.cc:380: GLenum
>> format =
>> os_win ? GL_BGRA_EXT : GL_RGBA;
>> Please define some sort of API that is overridden by
>> dxva_video_decode_accelerator.cc to fetch this format, so the use of
>> D3DFMT_X8R8G8B8 / D3DFMT_A8R8G8B8 and GL_BGRA_EXT are close together.
>>
>
> Yes, done. I add new method to only DXVAVideoDecodeAccelerator, instead of
> GpuVideoDecodeAccelerator.
> It's because I don't want to touch innocent tons of subclasses of
> GpuVideoDecodeAccelerator.
>
>  I'm also concerned that this may not be forward compatible with the
>> OpenGL ES
>> 3.0 upgrade. Is GL_BGRA_EXT a valid internal format? According to
>>
>
> https://www.khronos.org/registry/gles/extensions/
> APPLE/APPLE_texture_format_BGRA8888.txt
>
>> -- which is the only BGRA extension that was upgraded for ES 3.0 -- it
>> looks
>> like it needs to be GL_BGRA8_EXT.
>>
>
> GL_BGRA8_EXT should be used by only glTexStorage*
>
> APPLE_texture_format_BGRA8888 declairs that
>     Note that despite TexStorage* being defined in terms of TexImage*,
>     BGRA8_EXT is not accepted as an <internalformat> parameter to TexImage*
>     when the GL is OpenGL ES 3.0.
>
> In addition, Chromium never use GL_RGBA8 for TexImage* also. Chromium uses
> GL_RGBA for TexImage* and GL_RGBA8 for TexStorage2D.
>
>  Finally, how much testing has this change undergone? It has the potential
>> to
>> break video playback in many scenarios -- directly on the page, drawn
>> into 2D
>> and WebGL canvas contexts -- and I'm very concerned that we don't have
>> enough
>> test coverage to catch regressions in automated fashion. See
>> http://crbug.com/436578 .
>>
>
> I build chrome with proprietary_codecs=1 ffmpeg_branding=Chrome on Win 8.1
> and
> tests layout tests and webgl_conformance tests
> layout tests: fast/canvas and virtual/gpu/fast/canvsa
> webgl_conformance: textures/tex-images-and-sub-image-2d-with-*
>
> and test real sites
> h.264 (gpu video) : http://browsertests.herokuapp.com/media/h264.html
> gpu video to webgl :
> http://browsertests.herokuapp.com/media/mdn_samples_webgl_
> sample8/index.html
> gpu video to canvas 2d : http://html5demos.com/video-canvas
>
>
> https://codereview.chromium.org/775863005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2014-12-05 20:53:43]

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.h:68: unsigned
GetSufaceInternalFormat() const;
typo: should be GetSurfaceInternalFormat

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:376: format =
static_cast<DXVAVideoDecodeAccelerator*>(
Could you please avoid this downcast by defining a virtual function on
GpuVideoDecodeAccelerator with a default implementation returning GL_RGBA?

[dshwang, 2014-12-08 13:47:28]

Thank you for reviewing. Addressed all.

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.h:68: unsigned
GetSufaceInternalFormat() const;
On 2014/12/05 20:53:43, Ken Russell wrote:
> typo: should be GetSurfaceInternalFormat

Oops, thx. Done.

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/775863005/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:376: format =
static_cast<DXVAVideoDecodeAccelerator*>(
On 2014/12/05 20:53:43, Ken Russell wrote:
> Could you please avoid this downcast by defining a virtual function on
> GpuVideoDecodeAccelerator with a default implementation returning GL_RGBA?

Done.

[Ken Russell (switch to Gerrit), 2014-12-08 19:52:34]

Thanks for the cleanup - LGTM - but you'll need an OWNERS review in media/.

https://codereview.chromium.org/775863005/diff/40001/media/video/video_decode...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/775863005/diff/40001/media/video/video_decode...
media/video/video_decode_accelerator.h:148: virtual unsigned
GetSurfaceInternalFormat() const;
Minor note: the Google C++ style guide says to avoid using types like "unsigned"
- see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types .
Either add a comment or return int here and static_cast it to GLenum in the
implementation.

[dshwang, 2014-12-08 20:30:38]

On 2014/12/08 19:52:34, Ken Russell wrote:
> Thanks for the cleanup - LGTM - but you'll need an OWNERS review in media/.

Thank you for review.

@scherkus, could you review media/ ? This CL is needed to re-land
https://codereview.chromium.org/744713002/

https://codereview.chromium.org/775863005/diff/40001/media/video/video_decode...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/775863005/diff/40001/media/video/video_decode...
media/video/video_decode_accelerator.h:148: virtual unsigned
GetSurfaceInternalFormat() const;
On 2014/12/08 19:52:34, Ken Russell wrote:
> Minor note: the Google C++ style guide says to avoid using types like
"unsigned"
> - see
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
.
> Either add a comment or return int here and static_cast it to GLenum in the
> implementation.

I see. Forward declare GLenum like gles2_cmd_format.h, shader.h, and
accelerated_surface_mac.h

[scherkus (not reviewing), 2014-12-08 22:51:36]

lgtm w/ nits

https://codereview.chromium.org/775863005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/775863005/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:399: // FIXME: after
moving to D3D11, remove this workaround. crbug.com/438691
nit: chromium typically uses the TODO(username):

would you mind updating this?

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

https://codereview.chromium.org/775863005/diff/60001/media/video/video_decode...
media/video/video_decode_accelerator.h:149: // FIXME: after moving to D3D11,
remove this workaround. crbug.com/438691
ditto

[dshwang, 2014-12-09 09:36:02]

On 2014/12/08 22:51:36, scherkus wrote:
> lgtm w/ nits

Thank you for reviewing.

https://codereview.chromium.org/775863005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/775863005/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:399: // FIXME: after
moving to D3D11, remove this workaround. crbug.com/438691
On 2014/12/08 22:51:36, scherkus wrote:
> nit: chromium typically uses the TODO(username):
> 
> would you mind updating this?

Done. Yes, of course :)

[dshwang, 2014-12-09 09:36:07]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-12-09 09:36:37]

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

[commit-bot: I haz the power, 2014-12-09 10:31:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-09 10:32:37]

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