Re-land: ui: Move GLImage::BindTexImage fallback from GLImage implementations to GLES2CmdDecoder.

Re-land: ui: Move GLImage::BindTexImage fallback from GLImage implementations to GLES2CmdDecoder.

This allows the GPU service to properly track the memory usage
image backed textures.

It also reduces the complexity of GLImage implementations
significantly and makes it easier to support format and
buffer types that require a copy or conversion of data to
be used for sampling.

This change also includes a few minor GLImage cleanups such
as removing gfx:: namespace prefix in places where it's not
needed and making the CopyTexImage GLImage test not part of
the core GLImage tests as it's optional to support that
function.

BUG=526298
TEST=gl_tests --gtest_filter=GpuMemoryBuffer*, gpu_unittests, gl_unittests --gtest_filter=GLImage*

Committed: https://crrev.com/aa9e9d966688a7ef90fb12259b8e70ac7b6648d6
Cr-Commit-Position: refs/heads/master@{#355552}

[reveman, 2015-10-13 22:05:08]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2015-10-13 22:50:57]

On 2015/10/13 at 22:05:08, reveman wrote:
> 

Thank you, this will make it much easier to implement the 422 video format.

I'd still expect WillUseTexImage and DidUseTexImage to be called even in the
case we copy the texture. Why not?

[reveman, 2015-10-14 01:52:35]

reveman@chromium.org changed reviewers:
+ sievers@chromium.org

[Daniele Castagna, 2015-10-14 18:40:36]

GLImage and related code LGTM.

I'd probably prefer to pass the service texture id as a parameter of
CopyTexImage. Given that in another places we modify the currently bound texture
it probably makes more sense to leave it as it is in this CL though.

https://codereview.chromium.org/1401423003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1401423003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7643: return Texture::COPIED;
nit: should this be UNBOUND, not that it matters.

https://codereview.chromium.org/1401423003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/1401423003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.h:146: gfx::GLImage*
GetLevelImage(GLint target,
nit: Maybe overload this without the state parameter since it's called in so
many places with it set to nullptr.

https://codereview.chromium.org/1401423003/diff/120001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1401423003/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:24: using WidgetToLayerMap =
std::map<AcceleratedWidget, CALayer*>;
Cool, didn't know we can use type alias.

[reveman, 2015-10-14 19:28:01]

On 2015/10/14 at 18:40:36, dcastagna wrote:
> GLImage and related code LGTM.
> 
> I'd probably prefer to pass the service texture id as a parameter of
CopyTexImage. Given that in another places we modify the currently bound texture
it probably makes more sense to leave it as it is in this CL though.

I've considering the switch to explicit texture ids in the past. If we do that,
then all functions should change name from TexImage to Texture
(s/BindTexImage/BindTexture/, etc.). The downside is that we'd instead need a
bunch of ScopedTextureBinders and support for binding an image to non-zero
mipmap level would be lost but that's not something we use today anyhow. I've
decided to keep it as is but I'm not against changing this in the future.

https://codereview.chromium.org/1401423003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1401423003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7643: return Texture::COPIED;
On 2015/10/14 at 18:40:35, Daniele Castagna wrote:
> nit: should this be UNBOUND, not that it matters.

State should change to COPIED to prevent us from copying each time we draw. We
only want to copy once after each BindTexImage2D command from the client.

https://codereview.chromium.org/1401423003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/1401423003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.h:146: gfx::GLImage*
GetLevelImage(GLint target,
On 2015/10/14 at 18:40:36, Daniele Castagna wrote:
> nit: Maybe overload this without the state parameter since it's called in so
many places with it set to nullptr.

Done.

[reveman, 2015-10-14 19:28:09]

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

[commit-bot: I haz the power, 2015-10-14 19:29:17]

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

[commit-bot: I haz the power, 2015-10-14 21:50:04]

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

[commit-bot: I haz the power, 2015-10-14 21:50:04]

Dry run: This issue passed the CQ dry run.

[no sievers, 2015-10-15 21:52:44]

https://codereview.chromium.org/1401423003/diff/160001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:139: TextureRef* texture =
texture_manager->GetTexture(client_texture_id_);
What if the original client texture was deleted, but it's still shared with
another texture through a mailbox?

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_definition.cc:95: return false;
NOTREACHED() also?

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1306: // If a level does not have
a GLImage bound to it, create a simple dump.
nit: how about 'Dump the texture allocation also if it's not shared with the
GLImage'?

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.h:42: enum ImageState { UNBOUND,
BOUND, COPIED };
nit: mind putting some comments with each enum about what it exactly means?

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.h:42: enum ImageState { UNBOUND,
BOUND, COPIED };
is the COPIED state actually used for anything?

[reveman, 2015-10-15 22:56:16]

ptal

https://codereview.chromium.org/1401423003/diff/160001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:139: TextureRef* texture =
texture_manager->GetTexture(client_texture_id_);
On 2015/10/15 at 21:52:44, sievers wrote:
> What if the original client texture was deleted, but it's still shared with
another texture through a mailbox?

hm, does that make it invalid to refer to the texture using the client id? I
changed it to use the service id in latest patch.

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_definition.cc:95: return false;
On 2015/10/15 at 21:52:44, sievers wrote:
> NOTREACHED() also?

Done. Here and below.

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1306: // If a level does not have
a GLImage bound to it, create a simple dump.
On 2015/10/15 at 21:52:44, sievers wrote:
> nit: how about 'Dump the texture allocation also if it's not shared with the
GLImage'?

Updated the comment to something along that line. Hopefully more clear now.

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/1401423003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.h:42: enum ImageState { UNBOUND,
BOUND, COPIED };
On 2015/10/15 at 21:52:44, sievers wrote:
> nit: mind putting some comments with each enum about what it exactly means?

Done. COPIED is used. See the comment in latest patch.

[reveman, 2015-10-15 22:56:21]

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

[commit-bot: I haz the power, 2015-10-15 22:56:39]

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

[no sievers, 2015-10-15 23:21:00]

lgtm

[reveman, 2015-10-15 23:31:32]

reveman@chromium.org changed reviewers:
+ fsamuel@chromium.org

[reveman, 2015-10-15 23:31:33]

+fsamuel for components/mus/gles2/

ericrk, fyi

[Fady Samuel, 2015-10-15 23:34:36]

mus lgtm

[commit-bot: I haz the power, 2015-10-16 01:21:27]

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

[commit-bot: I haz the power, 2015-10-16 01:21:28]

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

[reveman, 2015-10-16 06:57:06]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-16 06:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1401423003/#ps200001
(title: "stream_texture_android.h too")

[commit-bot: I haz the power, 2015-10-16 06:58:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1401423003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1401423003/200001

[commit-bot: I haz the power, 2015-10-16 09:33:28]

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

[commit-bot: I haz the power, 2015-10-16 09:33:29]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[reveman, 2015-10-16 20:55:50]

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

[commit-bot: I haz the power, 2015-10-16 20:56:27]

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

[commit-bot: I haz the power, 2015-10-16 21:44:45]

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

[commit-bot: I haz the power, 2015-10-16 21:44:46]

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

[reveman, 2015-10-19 13:28:29]

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

[commit-bot: I haz the power, 2015-10-19 13:29:00]

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

[reveman, 2015-10-19 13:36:09]

reveman@chromium.org changed reviewers:
+ liberato@chromium.org

[reveman, 2015-10-19 13:36:10]

+liberato, fyi I've update the AVDACodecImage code in content/common/gpu/media/
as part of this change. It should work in the same way as before but let me know
if you notice any issues.

[commit-bot: I haz the power, 2015-10-19 14:13:43]

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

[commit-bot: I haz the power, 2015-10-19 14:13:44]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2015-10-19 16:11:41]

https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.cc (left):

https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:87:
glBindTexture(GL_TEXTURE_EXTERNAL_OES,
this still needs to be done, but i don't see it in the new version.

https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:97:
glBindTexture(GL_TEXTURE_EXTERNAL_OES,
i think this still needs to happen as well.

[reveman, 2015-10-19 16:28:17]

https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.cc (left):

https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:87:
glBindTexture(GL_TEXTURE_EXTERNAL_OES,
On 2015/10/19 at 16:11:41, liberato wrote:
> this still needs to be done, but i don't see it in the new version.

We get this service id from the glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES)
call in AttachSurfaceTextureToContext(). That will return the service id that is
currently bound to the target and will be bound in future calls to
CopyTexImage(). I'm failing to see how the service id will ever not be bound
when this is called. What am I missing?

[liberato (no reviews please), 2015-10-19 16:46:08]

On 2015/10/19 16:28:17, reveman wrote:
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> File content/common/gpu/media/avda_codec_image.cc (left):
> 
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> content/common/gpu/media/avda_codec_image.cc:87:
> glBindTexture(GL_TEXTURE_EXTERNAL_OES,
> On 2015/10/19 at 16:11:41, liberato wrote:
> > this still needs to be done, but i don't see it in the new version.
> 
> We get this service id from the glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES)
> call in AttachSurfaceTextureToContext(). That will return the service id that
is
> currently bound to the target and will be bound in future calls to
> CopyTexImage(). I'm failing to see how the service id will ever not be bound
> when this is called. What am I missing?

because of the way the VideoDecodeAccelerator interface works, we must have
multiple (~4) Textures to support multiple video frames in-flight during
playback.  on android, we want all of these Textures to share the
SurfaceTexture.  we let one Texture's AVDACodecImage call
AttachSurfaceTextureToContext().  the others can't, though, since we re-use the
(T|t)extures and SurfaceTexture::Detach...() closes the platform texture.  so,
the other images just glBindTexture() the platform texture that the
SurfaceTexture knows about.

Another option is to modify the service_id of all the other Textures, so that
they all point to the magic one.

the goal of all of this is to avoid copying the decoded frame from
SurfaceTexture => teximage2d for every frame. The particular implementation is a
temporary measure that's required by how the video decode accelerator interface
currently works.  We're in the process of rewriting the VDA interface to make
this unneeded, but that's a much bigger undertaking.

[ericrk, 2015-10-19 17:15:44]

ericrk@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2015-10-19 17:15:45]

https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (left):

https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1259: // TODO(ericrk): Images may
have complex sizing not accounted for by
With current logic, estimated_size_ will always act as though there is a single
texture bound at each level, independent of whether there is an image bound as
well. Is that the pattern we want to go for now - might be fine - we'd be
treating the GLImages as though they aren't part of the texture I guess?

[reveman, 2015-10-19 19:12:29]

On 2015/10/19 at 16:46:08, liberato wrote:
> On 2015/10/19 16:28:17, reveman wrote:
> >
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > File content/common/gpu/media/avda_codec_image.cc (left):
> > 
> >
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > content/common/gpu/media/avda_codec_image.cc:87:
> > glBindTexture(GL_TEXTURE_EXTERNAL_OES,
> > On 2015/10/19 at 16:11:41, liberato wrote:
> > > this still needs to be done, but i don't see it in the new version.
> > 
> > We get this service id from the
glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES)
> > call in AttachSurfaceTextureToContext(). That will return the service id
that is
> > currently bound to the target and will be bound in future calls to
> > CopyTexImage(). I'm failing to see how the service id will ever not be bound
> > when this is called. What am I missing?
> 
> because of the way the VideoDecodeAccelerator interface works, we must have
multiple (~4) Textures to support multiple video frames in-flight during
playback.  on android, we want all of these Textures to share the
SurfaceTexture.  we let one Texture's AVDACodecImage call
AttachSurfaceTextureToContext().  the others can't, though, since we re-use the
(T|t)extures and SurfaceTexture::Detach...() closes the platform texture.  so,
the other images just glBindTexture() the platform texture that the
SurfaceTexture knows about.

Ok, got it. That's abusing the GLImage API a bit. There's not really a guarantee
that Will/DidUseTexImage gets called each time we draw. If bound to an FBO then
it won't.

> 
> Another option is to modify the service_id of all the other Textures, so that
they all point to the magic one.

I'm not a fan of that option. Can we somehow use the same TextureManager Texture
instance for all these client textures?

> 
> the goal of all of this is to avoid copying the decoded frame from
SurfaceTexture => teximage2d for every frame. The particular implementation is a
temporary measure that's required by how the video decode accelerator interface
currently works.  We're in the process of rewriting the VDA interface to make
this unneeded, but that's a much bigger undertaking.

Have you tried using the GPU for this copy? So SurfaceTexture ->
service-texture-id by binding the texture to a FBO and drawing the ST onto it.

I've updated the latest patch to still have the sneaky bind in it. It doesn't do
the restore though. If that's needed then we can add an ugly restore call with a
big TODO in the decoder for images and texture external target until we have a
better solution in place. Wdyt?

https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (left):

https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1259: // TODO(ericrk): Images may
have complex sizing not accounted for by
On 2015/10/19 at 17:15:45, ericrk wrote:
> With current logic, estimated_size_ will always act as though there is a
single texture bound at each level, independent of whether there is an image
bound as well. Is that the pattern we want to go for now - might be fine - we'd
be treating the GLImages as though they aren't part of the texture I guess?

Yes, that's the idea. estimated_size_ is the size for each level independent of
the images. If an image is bound, then that replaces the storage of the texture
and estimated_size_ should not be reported in the dumps. In all other cases
(UNBOUND/COPIED), estimated_size_ should be reported.

[liberato (no reviews please), 2015-10-19 19:33:44]

On 2015/10/19 19:12:29, reveman wrote:
> On 2015/10/19 at 16:46:08, liberato wrote:
> > On 2015/10/19 16:28:17, reveman wrote:
> > >
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > > File content/common/gpu/media/avda_codec_image.cc (left):
> > > 
> > >
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > > content/common/gpu/media/avda_codec_image.cc:87:
> > > glBindTexture(GL_TEXTURE_EXTERNAL_OES,
> > > On 2015/10/19 at 16:11:41, liberato wrote:
> > > > this still needs to be done, but i don't see it in the new version.
> > > 
> > > We get this service id from the
> glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES)
> > > call in AttachSurfaceTextureToContext(). That will return the service id
> that is
> > > currently bound to the target and will be bound in future calls to
> > > CopyTexImage(). I'm failing to see how the service id will ever not be
bound
> > > when this is called. What am I missing?
> > 
> > because of the way the VideoDecodeAccelerator interface works, we must have
> multiple (~4) Textures to support multiple video frames in-flight during
> playback.  on android, we want all of these Textures to share the
> SurfaceTexture.  we let one Texture's AVDACodecImage call
> AttachSurfaceTextureToContext().  the others can't, though, since we re-use
the
> (T|t)extures and SurfaceTexture::Detach...() closes the platform texture.  so,
> the other images just glBindTexture() the platform texture that the
> SurfaceTexture knows about.
> 
> Ok, got it. That's abusing the GLImage API a bit. There's not really a
guarantee
> that Will/DidUseTexImage gets called each time we draw. If bound to an FBO
then
> it won't.
> 
> > 
> > Another option is to modify the service_id of all the other Textures, so
that
> they all point to the magic one.
> 
> I'm not a fan of that option. Can we somehow use the same TextureManager
Texture
> instance for all these client textures?
> 
> > 
> > the goal of all of this is to avoid copying the decoded frame from
> SurfaceTexture => teximage2d for every frame. The particular implementation is
a
> temporary measure that's required by how the video decode accelerator
interface
> currently works.  We're in the process of rewriting the VDA interface to make
> this unneeded, but that's a much bigger undertaking.
> 
> Have you tried using the GPU for this copy? So SurfaceTexture ->
> service-texture-id by binding the texture to a FBO and drawing the ST onto it.
> 
> I've updated the latest patch to still have the sneaky bind in it. It doesn't
do
> the restore though. If that's needed then we can add an ugly restore call with
a
> big TODO in the decoder for images and texture external target until we have a
> better solution in place. Wdyt?
> 
>
https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/texture_manager.cc (left):
> 
>
https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
> gpu/command_buffer/service/texture_manager.cc:1259: // TODO(ericrk): Images
may
> have complex sizing not accounted for by
> On 2015/10/19 at 17:15:45, ericrk wrote:
> > With current logic, estimated_size_ will always act as though there is a
> single texture bound at each level, independent of whether there is an image
> bound as well. Is that the pattern we want to go for now - might be fine -
we'd
> be treating the GLImages as though they aren't part of the texture I guess?
> 
> Yes, that's the idea. estimated_size_ is the size for each level independent
of
> the images. If an image is bound, then that replaces the storage of the
texture
> and estimated_size_ should not be reported in the dumps. In all other cases
> (UNBOUND/COPIED), estimated_size_ should be reported.

some background that might help: avda_codec_image that you're looking at is not
enabled normally in production.  it's part of a major group of changes to how we
handle video in android.  the code that you're looking at is helping us do a/b
testing to see if the copy has any smoothness or power impacts.

use same Texture: unfortunately, we can't.  that's part of the api limitation
we're working around.

no unbind: i suspect that it'll work fine without the unbind.

thanks for taking the time with this!

-fl

[reveman, 2015-10-19 19:45:07]

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

[commit-bot: I haz the power, 2015-10-19 19:46:30]

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

[liberato (no reviews please), 2015-10-19 19:59:52]

On 2015/10/19 19:33:44, liberato wrote:
> On 2015/10/19 19:12:29, reveman wrote:
> > On 2015/10/19 at 16:46:08, liberato wrote:
> > > On 2015/10/19 16:28:17, reveman wrote:
> > > >
> >
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > > > File content/common/gpu/media/avda_codec_image.cc (left):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/1401423003/diff/260001/content/common/gpu/med...
> > > > content/common/gpu/media/avda_codec_image.cc:87:
> > > > glBindTexture(GL_TEXTURE_EXTERNAL_OES,
> > > > On 2015/10/19 at 16:11:41, liberato wrote:
> > > > > this still needs to be done, but i don't see it in the new version.
> > > > 
> > > > We get this service id from the
> > glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES)
> > > > call in AttachSurfaceTextureToContext(). That will return the service id
> > that is
> > > > currently bound to the target and will be bound in future calls to
> > > > CopyTexImage(). I'm failing to see how the service id will ever not be
> bound
> > > > when this is called. What am I missing?
> > > 
> > > because of the way the VideoDecodeAccelerator interface works, we must
have
> > multiple (~4) Textures to support multiple video frames in-flight during
> > playback.  on android, we want all of these Textures to share the
> > SurfaceTexture.  we let one Texture's AVDACodecImage call
> > AttachSurfaceTextureToContext().  the others can't, though, since we re-use
> the
> > (T|t)extures and SurfaceTexture::Detach...() closes the platform texture. 
so,
> > the other images just glBindTexture() the platform texture that the
> > SurfaceTexture knows about.
> > 
> > Ok, got it. That's abusing the GLImage API a bit. There's not really a
> guarantee
> > that Will/DidUseTexImage gets called each time we draw. If bound to an FBO
> then
> > it won't.
> > 
> > > 
> > > Another option is to modify the service_id of all the other Textures, so
> that
> > they all point to the magic one.
> > 
> > I'm not a fan of that option. Can we somehow use the same TextureManager
> Texture
> > instance for all these client textures?
> > 
> > > 
> > > the goal of all of this is to avoid copying the decoded frame from
> > SurfaceTexture => teximage2d for every frame. The particular implementation
is
> a
> > temporary measure that's required by how the video decode accelerator
> interface
> > currently works.  We're in the process of rewriting the VDA interface to
make
> > this unneeded, but that's a much bigger undertaking.
> > 
> > Have you tried using the GPU for this copy? So SurfaceTexture ->
> > service-texture-id by binding the texture to a FBO and drawing the ST onto
it.
> > 
> > I've updated the latest patch to still have the sneaky bind in it. It
doesn't
> do
> > the restore though. If that's needed then we can add an ugly restore call
with
> a
> > big TODO in the decoder for images and texture external target until we have
a
> > better solution in place. Wdyt?
> > 
> >
>
https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/texture_manager.cc (left):
> > 
> >
>
https://codereview.chromium.org/1401423003/diff/260001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/texture_manager.cc:1259: // TODO(ericrk): Images
> may
> > have complex sizing not accounted for by
> > On 2015/10/19 at 17:15:45, ericrk wrote:
> > > With current logic, estimated_size_ will always act as though there is a
> > single texture bound at each level, independent of whether there is an image
> > bound as well. Is that the pattern we want to go for now - might be fine -
> we'd
> > be treating the GLImages as though they aren't part of the texture I guess?
> > 
> > Yes, that's the idea. estimated_size_ is the size for each level independent
> of
> > the images. If an image is bound, then that replaces the storage of the
> texture
> > and estimated_size_ should not be reported in the dumps. In all other cases
> > (UNBOUND/COPIED), estimated_size_ should be reported.
> 
> some background that might help: avda_codec_image that you're looking at is
not
> enabled normally in production.  it's part of a major group of changes to how
we
> handle video in android.  the code that you're looking at is helping us do a/b
> testing to see if the copy has any smoothness or power impacts.
> 
> use same Texture: unfortunately, we can't.  that's part of the api limitation
> we're working around.
> 
> no unbind: i suspect that it'll work fine without the unbind.
> 
> thanks for taking the time with this!
> 
> -fl

lgtm

[commit-bot: I haz the power, 2015-10-19 20:54:43]

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

[commit-bot: I haz the power, 2015-10-19 20:54:45]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-10-19 21:22:55]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-19 21:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, dcastagna@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1401423003/#ps280001
(title: "keep sneakily bind the ST texture handle")

[commit-bot: I haz the power, 2015-10-19 21:24:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1401423003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1401423003/280001

[commit-bot: I haz the power, 2015-10-19 21:37:00]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-10-19 21:37:43]

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

[reveman, 2015-10-19 23:36:34]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1418483003/ by reveman@chromium.org.

The reason for reverting is: Causing android gpu bots to fail.

[reveman, 2015-10-20 02:53:42]

Description was changed from

==========
ui: Move GLImage::BindTexImage fallback from GLImage implementations to
GLES2CmdDecoder.

This allows the GPU service to properly track the memory usage
image backed textures.

It also reduces the complexity of GLImage implementations
significantly and makes it easier to support format and
buffer types that require a copy or conversion of data to
be used for sampling.

This change also includes a few minor GLImage cleanups such
as removing gfx:: namespace prefix in places where it's not
needed and making the CopyTexImage GLImage test not part of
the core GLImage tests as it's optional to support that
function.

BUG=526298
TEST=gl_tests --gtest_filter=GpuMemoryBuffer*, gpu_unittests, gl_unittests
--gtest_filter=GLImage*

Committed: https://crrev.com/ac2696e324bda3824952148f831e76a8b80594b3
Cr-Commit-Position: refs/heads/master@{#354870}
==========

to

==========
Re-land: ui: Move GLImage::BindTexImage fallback from GLImage implementations to
GLES2CmdDecoder.

This allows the GPU service to properly track the memory usage
image backed textures.

It also reduces the complexity of GLImage implementations
significantly and makes it easier to support format and
buffer types that require a copy or conversion of data to
be used for sampling.

This change also includes a few minor GLImage cleanups such
as removing gfx:: namespace prefix in places where it's not
needed and making the CopyTexImage GLImage test not part of
the core GLImage tests as it's optional to support that
function.

BUG=526298
TEST=gl_tests --gtest_filter=GpuMemoryBuffer*, gpu_unittests, gl_unittests
--gtest_filter=GLImage*
==========

[reveman, 2015-10-20 02:59:49]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-20 02:59:50]

Re-landing this after removing the NOTREACHED I added to
GLImageSync::CopyTex(Sub)Image from latest patch. That function is actually
called so we need keep returning false from there without crashing.

[reveman, 2015-10-20 02:59:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, dcastagna@chromium.org, sievers@chromium.org,
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1401423003/#ps300001
(title: "Remove NOTREACHED() from GLImageSync::CopyTexImage")

[commit-bot: I haz the power, 2015-10-20 03:00:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1401423003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1401423003/300001

[commit-bot: I haz the power, 2015-10-20 04:04:08]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-10-20 04:04:57]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-10-20 04:05:07]

Patchset 16 (id:??) landed as
https://crrev.com/779bd9550a06358ff068edc7458fc8b7b0a69147
Cr-Commit-Position: refs/heads/master@{#354972}

[vasilii, 2015-10-20 08:48:23]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/1418603002/ by vasilii@chromium.org.

The reason for reverting is: Unfortunately Android gpu bots still fail
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus....

[reveman, 2015-10-21 13:14:29]

Description was changed from

==========
Re-land: ui: Move GLImage::BindTexImage fallback from GLImage implementations to
GLES2CmdDecoder.

This allows the GPU service to properly track the memory usage
image backed textures.

It also reduces the complexity of GLImage implementations
significantly and makes it easier to support format and
buffer types that require a copy or conversion of data to
be used for sampling.

This change also includes a few minor GLImage cleanups such
as removing gfx:: namespace prefix in places where it's not
needed and making the CopyTexImage GLImage test not part of
the core GLImage tests as it's optional to support that
function.

BUG=526298
TEST=gl_tests --gtest_filter=GpuMemoryBuffer*, gpu_unittests, gl_unittests
--gtest_filter=GLImage*

Committed: https://crrev.com/779bd9550a06358ff068edc7458fc8b7b0a69147
Cr-Commit-Position: refs/heads/master@{#354972}
==========

to

==========
Re-land: ui: Move GLImage::BindTexImage fallback from GLImage implementations to
GLES2CmdDecoder.

This allows the GPU service to properly track the memory usage
image backed textures.

It also reduces the complexity of GLImage implementations
significantly and makes it easier to support format and
buffer types that require a copy or conversion of data to
be used for sampling.

This change also includes a few minor GLImage cleanups such
as removing gfx:: namespace prefix in places where it's not
needed and making the CopyTexImage GLImage test not part of
the core GLImage tests as it's optional to support that
function.

BUG=526298
TEST=gl_tests --gtest_filter=GpuMemoryBuffer*, gpu_unittests, gl_unittests
--gtest_filter=GLImage*
==========

[reveman, 2015-10-22 15:28:30]

I was able to reproduce this issue locally and have verified that the latest
patch fix the problem. Re-landing.

https://codereview.chromium.org/1401423003/diff/300001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1401423003/diff/300001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:144:
texture->SetLevelImage(GL_TEXTURE_EXTERNAL_OES, 0, this,
This needs to happen even if a pending frame was not available. ie. it needs to
be moved out of the "if (has_pending_frame_)" scope. This was the reason for the
failure on the waterfall and has been fixed in latest patch.

This function also needs to return false if target is not
GL_TEXTURE_EXTERNAL_OES or the currently bound texture doesn't match
|texture_id_|. Also fixed in latest patch.

[reveman, 2015-10-22 15:28:35]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-22 15:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, dcastagna@chromium.org, sievers@chromium.org,
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1401423003/#ps320001
(title: "fix stream texture issue")

[commit-bot: I haz the power, 2015-10-22 15:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1401423003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1401423003/320001

[commit-bot: I haz the power, 2015-10-22 16:43:15]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-10-22 16:44:14]

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