Add texture target field to video frame (for use by native textures).

Add texture target field to video frame (for use by native textures).

To be used by https://bugs.webkit.org/show_bug.cgi?id=78398.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123255

[no sievers, 2012-02-22 03:43:02]

Is it ok to include GL/Gl.h in media/base/video_frame.cc?

[Ami GONE FROM CHROMIUM, 2012-02-22 04:51:00]

I like where this is heading!

(also, I wonder whether switching the existing uses of GL_TEXTURE_2D to
GL_TEXTURE_EXTERNAL_OES would work / have any extra costs; do you know what
EXTERNAL_OES actually means/does?)

https://chromiumcodereview.appspot.com/9416087/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/9416087/diff/1/media/base/video_frame....
media/base/video_frame.cc:7: #include <GL/gl.h>
Can drop this if you take my suggestion below.

https://chromiumcodereview.appspot.com/9416087/diff/1/media/base/video_frame....
media/base/video_frame.cc:62: frame->texture_target_ = GL_TEXTURE_2D;
This smells wrong to me (b/c what's 2Dlicious about this class/method?).  I
think at the very least mention of GL_TEXTURE_2D belongs one level higher (in
the caller, GpuVideoDecoder), but more likely it belongs even higher than that. 


The magic of TEXTURE_2D is introduced in
RendererGpuVideoDecoderFactories::AsyncCreateTextures, so ISTM *that's* where
the texture_target should start being specified from.  Following that logic
through would have media::GpuVideoDecoder::Factories::CreateTextures also return
the target (assuming we can use a single target for all returned textures).  A
purist approach would preserve the target alongside the texture_id in
media::PictureBuffer but that seems like overkill to me (both because it would
require plumbing through pepper as well, and because it is constant per video
element).  I think a reasonable compromise is to have CreateTextures return the
textures' target in an additional out param, and have GpuVideoDecoder remember
the target for later calls to WrapNativeTexture().  WDYT?

(a side benefit of this scheme is that everyone but
RendererGpuVideoDecoderFactories gets to treat the target as an opaque uint32 so
you don't have to think too hard about what may include <GL/gl.h> :))

[no sievers, 2012-02-22 20:05:05]

On 2012/02/22 04:51:00, Ami Fischman wrote:
> I like where this is heading!
> 
> (also, I wonder whether switching the existing uses of GL_TEXTURE_2D to
> GL_TEXTURE_EXTERNAL_OES would work / have any extra costs; do you know what
> EXTERNAL_OES actually means/does?)
> 

It can be used to bind EGLimages as textures. On Android it is used to bind
frames from streaming SurfaceTextures.
The separate texture target allows to limit manipulation of texture targets and
supporting driver-internal formats from the hardware decoder (that can then be
converted to RGB internally by the driver).
So it's possible that this can be useful/supported on other platforms, but
depends on the platform and drivers etc.

Definitely a good idea to push settings this field up further where it's known
what the target is, so I went with your suggestions in the new patch.

[no sievers, 2012-02-22 20:06:10]

Also see
http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt
for more detail.


On 2012/02/22 20:05:05, Daniel Sievers wrote:
> On 2012/02/22 04:51:00, Ami Fischman wrote:
> > I like where this is heading!
> > 
> > (also, I wonder whether switching the existing uses of GL_TEXTURE_2D to
> > GL_TEXTURE_EXTERNAL_OES would work / have any extra costs; do you know what
> > EXTERNAL_OES actually means/does?)
> > 
> 
> It can be used to bind EGLimages as textures. On Android it is used to bind
> frames from streaming SurfaceTextures.
> The separate texture target allows to limit manipulation of texture targets
and
> supporting driver-internal formats from the hardware decoder (that can then be
> converted to RGB internally by the driver).
> So it's possible that this can be useful/supported on other platforms, but
> depends on the platform and drivers etc.
> 
> Definitely a good idea to push settings this field up further where it's known
> what the target is, so I went with your suggestions in the new patch.

[Ami GONE FROM CHROMIUM, 2012-02-22 23:44:48]

lgtm

http://codereview.chromium.org/9416087/diff/3001/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

http://codereview.chromium.org/9416087/diff/3001/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:78:
gles2->BindTexture(GL_TEXTURE_2D, texture_id);
Could as well replace all the GL_TEXTURE_2D's from here on down with
*texture_target.

[commit-bot: I haz the power, 2012-02-22 23:50:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/9416087/10001

[commit-bot: I haz the power, 2012-02-23 05:20:33]

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

[commit-bot: I haz the power, 2012-02-23 06:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/9416087/10001

[commit-bot: I haz the power, 2012-02-23 08:29:23]

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