Plumb texture target to VideoDecodeAccelerator::Client

Plumb texture target to VideoDecodeAccelerator::Client

Currently all VideoDecodeAccelerator::Clients use GL_TEXTURE_2D as their texture target.

Unfortunately the Mac decoder only supports GL_TEXTURE_RECTANGLE_ARB texture targets.

This CL allows decoders to choose which texture target they want.

BUG=127414
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141258

[Wez, 2012-05-17 16:51:38]

IDL versioning LGTM, FWIW.

[Ami GONE FROM CHROMIUM, 2012-05-17 17:13:56]

The *VDA's today are agnostic of what kind of textures they're handed, and I
think that's a good thing.
I think a much better way to do achieve your goal would be to provide the right
texture per platform in whatever responds to ProvidePictureBuffers().  For HTML5
that's
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/ren...

For ppapi it's only slightly trickier because the plugin implements PPB.  But
this is still a per-platform decision, so it makes sense to me to provide a
helper/util function for ppapi plugins to select the right texture for decoder
per platform.

Plumbing a per-platform constant through all these layers just seems crazy to me
:)

[sail, 2012-05-23 23:17:03]

On 2012/05/17 17:13:56, Ami Fischman wrote:
> The *VDA's today are agnostic of what kind of textures they're handed, and I
> think that's a good thing.
> I think a much better way to do achieve your goal would be to provide the
right
> texture per platform in whatever responds to ProvidePictureBuffers().  For
HTML5
> that's
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/ren...
> 
> For ppapi it's only slightly trickier because the plugin implements PPB.  But
> this is still a per-platform decision, so it makes sense to me to provide a
> helper/util function for ppapi plugins to select the right texture for decoder
> per platform.
> 
> Plumbing a per-platform constant through all these layers just seems crazy to
me
> :)

Hey Ami, I talked to Trung about this and we both agreed it would be better to
parameterize the texture target.

If the PPP_VideoDecoder_Dev is made available to NACL plugins then we'd want
them to be cross platform.

The GL_TEXTURE_RECTANGLE_ARB target is a standard ARB extension for non-power of
two texture sizes. It's not platform specific.

Also, the decoder code isn't really agnostic as far as the texture target. For
example, on the Mac side I explicitly bind the texture ID to the
GL_TEXTURE_RECTANGLE_ARB texture target.

What do you think?

Thanks,
Sailehs

[Ami GONE FROM CHROMIUM, 2012-05-23 23:42:33]

https://chromiumcodereview.appspot.com/10392141/diff/1/content/common/gpu/med...
File content/common/gpu/media/rendering_helper.h (right):

https://chromiumcodereview.appspot.com/10392141/diff/1/content/common/gpu/med...
content/common/gpu/media/rendering_helper.h:43: int texture_target,
uint32 to match other GL types

https://chromiumcodereview.appspot.com/10392141/diff/1/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10392141/diff/1/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:347: static int
ToGLTextureTarget(
Hopefully can drop.

https://chromiumcodereview.appspot.com/10392141/diff/1/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/10392141/diff/1/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:330: // TODO(sail) : Add support for ARB
texture targets.
I think it's important to do these TODOs before the CL lands as a sanity check. 
For example, see 5 lines down.

https://chromiumcodereview.appspot.com/10392141/diff/1/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:335: count, size, &texture_ids,
&decoder_texture_target_)) {
Here CreateTextures() expects to be able to proscribe which target the texture
was created against.
In fact, it doesn't need freedom, but the resulting textures need to be tagged
w/ their targets.

So I think you should replace the TODO at l330 above with passing the
texture_target arg to PPB on to Factories::CreateTextures() and use the
passed-in texture value there (and change CreateTextures() to accept the target
as an input instead of output param).

https://chromiumcodereview.appspot.com/10392141/diff/1/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/10392141/diff/1/media/video/video_deco...
media/video/video_decode_accelerator.h:44: enum TextureTarget {
I don't think this (and its PP_VD_TT_D equivalent) are worth the extra
text/casting vs. just using a uint32 (like texture id).

https://chromiumcodereview.appspot.com/10392141/diff/1/ppapi/examples/video_d...
File ppapi/examples/video_decode/video_decode.cc (right):

https://chromiumcodereview.appspot.com/10392141/diff/1/ppapi/examples/video_d...
ppapi/examples/video_decode/video_decode.cc:343: // TODO(sail): Add support for
ARB texture types.
Can you do this as part of this CL?

[sail, 2012-05-29 18:58:09]

http://codereview.chromium.org/10392141/diff/1/content/common/gpu/media/rende...
File content/common/gpu/media/rendering_helper.h (right):

http://codereview.chromium.org/10392141/diff/1/content/common/gpu/media/rende...
content/common/gpu/media/rendering_helper.h:43: int texture_target,
On 2012/05/23 23:42:33, Ami Fischman wrote:
> uint32 to match other GL types

Done.

http://codereview.chromium.org/10392141/diff/1/content/common/gpu/media/video...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/10392141/diff/1/content/common/gpu/media/video...
content/common/gpu/media/video_decode_accelerator_unittest.cc:347: static int
ToGLTextureTarget(
On 2012/05/23 23:42:33, Ami Fischman wrote:
> Hopefully can drop.

Done.

http://codereview.chromium.org/10392141/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10392141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:330: // TODO(sail) : Add support for ARB
texture targets.
On 2012/05/23 23:42:33, Ami Fischman wrote:
> I think it's important to do these TODOs before the CL lands as a sanity
check. 
> For example, see 5 lines down.

Done.

http://codereview.chromium.org/10392141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:335: count, size, &texture_ids,
&decoder_texture_target_)) {
On 2012/05/23 23:42:33, Ami Fischman wrote:
> Here CreateTextures() expects to be able to proscribe which target the texture
> was created against.
> In fact, it doesn't need freedom, but the resulting textures need to be tagged
> w/ their targets.
> 
> So I think you should replace the TODO at l330 above with passing the
> texture_target arg to PPB on to Factories::CreateTextures() and use the
> passed-in texture value there (and change CreateTextures() to accept the
target
> as an input instead of output param).

Done.

http://codereview.chromium.org/10392141/diff/1/media/video/video_decode_accel...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/10392141/diff/1/media/video/video_decode_accel...
media/video/video_decode_accelerator.h:44: enum TextureTarget {
On 2012/05/23 23:42:33, Ami Fischman wrote:
> I don't think this (and its PP_VD_TT_D equivalent) are worth the extra
> text/casting vs. just using a uint32 (like texture id).

Done.

http://codereview.chromium.org/10392141/diff/1/ppapi/examples/video_decode/vi...
File ppapi/examples/video_decode/video_decode.cc (right):

http://codereview.chromium.org/10392141/diff/1/ppapi/examples/video_decode/vi...
ppapi/examples/video_decode/video_decode.cc:343: // TODO(sail): Add support for
ARB texture types.
On 2012/05/23 23:42:33, Ami Fischman wrote:
> Can you do this as part of this CL?

This part is implemented in this CL:
http://codereview.chromium.org/10411042/

I'd like to keep the change in a separate CL. If feel strongly that this
shouldn't be submitted like this then maybe I can wait for both to get an LGTM
and submit a single patch?

[Ami GONE FROM CHROMIUM, 2012-05-29 23:32:35]

lgtm

http://codereview.chromium.org/10392141/diff/10002/media/video/video_decode_a...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/10392141/diff/10002/media/video/video_decode_a...
media/video/video_decode_accelerator.h:104: const std::vector<PictureBuffer>&
buffers) = 0;
I wonder whether someday soon we'll regret not adding texture_target as a field
to PictureBuffer.

[sail, 2012-06-02 21:11:27]

kbr: src/content/common/gpu/ OWNERS review
brettw: src/ppapi/ and src/webkit/plugins/ppapi/ OWNERS review

[Ken Russell (switch to Gerrit), 2012-06-04 20:04:57]

content/common/gpu LGTM

I'm sure you know that shaders consuming these textures will have to be
adjusted; for example, the Mac port will need to use sampler2DRECT rather than
sampler2D.

[sail, 2012-06-04 20:31:03]

On 2012/06/04 20:04:57, kbr wrote:
> content/common/gpu LGTM
> 
> I'm sure you know that shaders consuming these textures will have to be
> adjusted; for example, the Mac port will need to use sampler2DRECT rather than
> sampler2D.

Yep! I'll send out the Pepper Flash side of this soon. I updated the
ppapi/samples/ code here:
http://codereview.chromium.org/10411042/diff/18002/ppapi/examples/video_decod...

[brettw, 2012-06-04 23:42:33]

https://chromiumcodereview.appspot.com/10392141/diff/14005/ppapi/proxy/ppp_vi...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

https://chromiumcodereview.appspot.com/10392141/diff/14005/ppapi/proxy/ppp_vi...
ppapi/proxy/ppp_video_decoder_proxy.cc:85:
dispatcher->local_get_interface()(PPP_VIDEODECODER_DEV_INTERFACE));
This code doesn't provide backwards compat. We may not care about this, though.
I assume our older versions of Flash that are distributed don't use HW decode at
all? If that's the case, this is OK, otherwise we should add backwards compat.

[sail, 2012-06-04 23:49:46]

https://chromiumcodereview.appspot.com/10392141/diff/14005/ppapi/proxy/ppp_vi...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

https://chromiumcodereview.appspot.com/10392141/diff/14005/ppapi/proxy/ppp_vi...
ppapi/proxy/ppp_video_decoder_proxy.cc:85:
dispatcher->local_get_interface()(PPP_VIDEODECODER_DEV_INTERFACE));
On 2012/06/04 23:42:33, brettw wrote:
> This code doesn't provide backwards compat. We may not care about this,
though.
> I assume our older versions of Flash that are distributed don't use HW decode
at
> all? If that's the case, this is OK, otherwise we should add backwards compat.

Yea, hardware decoding is disabled by default.

[brettw, 2012-06-05 19:35:05]

LGTM

[commit-bot: I haz the power, 2012-06-08 13:57:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10392141/14005

[commit-bot: I haz the power, 2012-06-08 14:22:56]

Try job failure for 10392141-14005 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-08 14:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10392141/27002

[commit-bot: I haz the power, 2012-06-08 16:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10392141/28005

[commit-bot: I haz the power, 2012-06-08 16:45:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10392141/28007

[commit-bot: I haz the power, 2012-06-08 18:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10392141/36063

[commit-bot: I haz the power, 2012-06-08 19:39:47]

Change committed as 141258