Remove StreamTextureManager

Remove StreamTextureManager

Remove stream texture manager and ties as well as special logic
(IsStreamTexture, IsStreamTextureOwner) from the decoder.
Instead attach a GLImage to the texture and track it im ImageManager.

BUG=239760

[no sievers, 2013-08-12 22:58:26]

ptal

Seems to simplify things a lot and removes a bunch of indirections.

Also gets stream textures working halfway with in-process mode (need to
implement the client-side portion yet).

[piman, 2013-08-12 23:22:07]

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:927: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
I assume the lookup can fail and you'd get a NULL image, so you probably need to
check image before doing image->AsGLImageStream().

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:943: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
same here wrt lookup

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:957: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
same here wrt lookup

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9488:
gfx::GLImage::CreateGLImageForStreamTexture(texture->service_id());
General question: is it ok for the GLImage to outlive the service id?

In the previous code, the whole infrastructure would tear down the StreamTexture
when the last reference to the texture on this context group was released (in
TextureManager::StopTracking), ensuring this. It was a bit of a hack (and not
compatible with ÜC), so I'm glad it's gone, but now the GLImage may outlive the
texture (if the user doesn't explicitly call DestroyStreamTexture).

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/te...
File gpu/command_buffer/service/texture_manager_unittest.cc (left):

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/te...
gpu/command_buffer/service/texture_manager_unittest.cc:2022: }
We should keep a test that we can Produce/Consume a texture bound to a GLImage.

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl.gyp#newcode44
ui/gl/gl.gyp:44: 'android/gl_image_stream.h',
These files are missing from the patch

[no sievers, 2013-08-13 00:21:35]

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:927: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
On 2013/08/12 23:22:07, piman wrote:
> I assume the lookup can fail and you'd get a NULL image, so you probably need
to
> check image before doing image->AsGLImageStream().

oops, thanks.

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:943: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
On 2013/08/12 23:22:07, piman wrote:
> same here wrt lookup

Done.

https://codereview.chromium.org/22824009/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:957: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
On 2013/08/12 23:22:07, piman wrote:
> same here wrt lookup

Done.

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9488:
gfx::GLImage::CreateGLImageForStreamTexture(texture->service_id());
On 2013/08/12 23:22:07, piman wrote:
> General question: is it ok for the GLImage to outlive the service id?
> 
> In the previous code, the whole infrastructure would tear down the
StreamTexture
> when the last reference to the texture on this context group was released (in
> TextureManager::StopTracking), ensuring this. It was a bit of a hack (and not
> compatible with ÜC), so I'm glad it's gone, but now the GLImage may outlive
the
> texture (if the user doesn't explicitly call DestroyStreamTexture).

I think it should be harmless, at least with the current entry points.

From looking at the current SurfaceTexture implementation, it seems like it does
not delete the texture for you during destruction (you construct the
SurfaceTexture from a GL texture). And it's a GC-collected Java object so you
cannot guarantee order anyways.

The only thing I can imagine going wrong would be:
1) the texture gets deleted (unbound and deleted)
2) eventually the driver returns the same texture name from glGenTextures()
3) we call UpdateTexImage() which binds the (still valid) EGLimage to the new
texture with the same name - now our tracking is messed up

3) cannot happen with the current entry points, because you cannot rebind the
GLimage to a new texture. But we need to make sure that stays the case (for
example if I added glBindTexImageExternal). The
GLImageStream::BindTexImageExternal()/ReleaseTexImageExternal() functions I
added actually do the right thing already (they change the binding to a new GL
texture).

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/te...
File gpu/command_buffer/service/texture_manager_unittest.cc (left):

https://codereview.chromium.org/22824009/diff/1/gpu/command_buffer/service/te...
gpu/command_buffer/service/texture_manager_unittest.cc:2022: }
On 2013/08/12 23:22:07, piman wrote:
> We should keep a test that we can Produce/Consume a texture bound to a
GLImage.

Done.

[reveman, 2013-08-13 00:48:13]

https://codereview.chromium.org/22824009/diff/4001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9450: error::Error
GLES2DecoderImpl::HandleCreateStreamTextureCHROMIUM(
Could this become CreateStreamImageCHROMIUM and require BindTexImageCHROMIUM to
be called for it to be bound to a texture?

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode44
ui/gl/gl_image.h:44: virtual void UpdateTexImage();
Lets make this part of the core API and define it as something that is useful to
all types of GLImages.

For 0-copy we currently call BindTex/ReleaseTex before/after each use of an
image. This makes it hard to avoid unnecessary cache flushes and implement an
efficient buffered version (ie. GLImageShm). I'd like to change this and only
have the client call BindTexImage/ReleaseTexImage when contents have changed. To
do that we need new API that allows GLImageEGL to do what it currently does in
ReleaseTexImage after each use of the texture. I was planning to add a pair of
functions that would be called before/after an image bound to a texture is used
on the service side. Something like:

virtual void WillUseTexImage();
virtual void DidUseTexImage();

You could do what you currently do in UpdateTexImage() for stream images in
WillUseTexImage() instead. Wdyt?

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode52
ui/gl/gl_image.h:52: unsigned int texture_id);
hm, texture_id. Could this instead take some new version of SurfaceTextureBridge
that instead creates the texture id binding as a result of calling BindTexImage?

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode55
ui/gl/gl_image.h:55: virtual GLImageStream* AsGLImageStream() { return NULL; }
what do we have to do to remove this?

[piman, 2013-08-13 00:52:04]

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
File ui/gl/android/gl_image_stream.cc (right):

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
ui/gl/android/gl_image_stream.cc:46: {
nit: { on previous line

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
ui/gl/android/gl_image_stream.cc:53:
surface_texture_bridge_->SetFrameAvailableCallback(frame_cb);
Doesn't this cause a reference cycle?
frame_cb references this, this references surface_texture_bridge_ and
surface_texture_bridge_ references frame_cb (its storage is refcounted).

[no sievers, 2013-08-14 04:01:23]

https://codereview.chromium.org/22824009/diff/4001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9450: error::Error
GLES2DecoderImpl::HandleCreateStreamTextureCHROMIUM(
On 2013/08/13 00:48:13, David Reveman wrote:
> Could this become CreateStreamImageCHROMIUM and require BindTexImageCHROMIUM
to
> be called for it to be bound to a texture?


Yes, I think that sounds good. But since it needs a lot of plumbing (or mostly
code auto-generation), I wanted to do it in a follow-up. (While I think this
patch might actually have to go into M30.)

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode44
ui/gl/gl_image.h:44: virtual void UpdateTexImage();
On 2013/08/13 00:48:13, David Reveman wrote:
> Lets make this part of the core API and define it as something that is useful
to
> all types of GLImages.
> 
> For 0-copy we currently call BindTex/ReleaseTex before/after each use of an
> image. This makes it hard to avoid unnecessary cache flushes and implement an
> efficient buffered version (ie. GLImageShm). I'd like to change this and only
> have the client call BindTexImage/ReleaseTexImage when contents have changed.
To
> do that we need new API that allows GLImageEGL to do what it currently does in
> ReleaseTexImage after each use of the texture. I was planning to add a pair of
> functions that would be called before/after an image bound to a texture is
used
> on the service side. Something like:
> 
> virtual void WillUseTexImage();
> virtual void DidUseTexImage();
> 
> You could do what you currently do in UpdateTexImage() for stream images in
> WillUseTexImage() instead. Wdyt?

Done. I *might* have a usage case for the client needing to explicitly update it
for stream textures though. But let's not worry about it now.

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode52
ui/gl/gl_image.h:52: unsigned int texture_id);
On 2013/08/13 00:48:13, David Reveman wrote:
> hm, texture_id. Could this instead take some new version of
SurfaceTextureBridge
> that instead creates the texture id binding as a result of calling
BindTexImage?

As discussed per chat leaving this for later. 
(One option to support this without SurfaceTexture.AttachToGLContext, which is
only available in JellyBean and up, is to allow a GLimage to override the
service_id of a texture.)

https://codereview.chromium.org/22824009/diff/4001/ui/gl/gl_image.h#newcode55
ui/gl/gl_image.h:55: virtual GLImageStream* AsGLImageStream() { return NULL; }
On 2013/08/13 00:48:13, David Reveman wrote:
> what do we have to do to remove this?

I have ideas of how to GL-ify the stuff that happens in gpu_channel.cc (which is
the only place that needs it to map to the current behavior).

SetSize and the matrix callback will ideally go away from GLImageStream. That
leaves a frame available callback that can maybe be exposed in the base class
here?

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
File ui/gl/android/gl_image_stream.cc (right):

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
ui/gl/android/gl_image_stream.cc:46: {
On 2013/08/13 00:52:04, piman wrote:
> nit: { on previous line

Done.

https://codereview.chromium.org/22824009/diff/5044/ui/gl/android/gl_image_str...
ui/gl/android/gl_image_stream.cc:53:
surface_texture_bridge_->SetFrameAvailableCallback(frame_cb);
On 2013/08/13 00:52:04, piman wrote:
> Doesn't this cause a reference cycle?
> frame_cb references this, this references surface_texture_bridge_ and
> surface_texture_bridge_ references frame_cb (its storage is refcounted).

Thanks, no point for the callback to add a ref back to |this| anyway. I added a
weak ptr factory.

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5663:
texture->GetLevelImage(texture->target(), 0);
need to worry about multiple leves/images here?

[reveman, 2013-08-14 15:22:43]

https://codereview.chromium.org/22824009/diff/39001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/22824009/diff/39001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:924: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
LookupStreamImage? to remove the need for AsGLImageStream..

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1401: void
PrepareTexImageForDraw(Texture* texture);
PrepareTextureForRender? and maybe not be specific to textures with GLImage
attached but include all the logic from PrepareTexturesForRender.

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9492:
image_manager()->AddImage(image, image_id);
I thought we decided to track these separably from standard GLImages and use a
different Id namespace until the usage matches the basic concept of GLImage. I'm
ok with using the image manager in the meantime if you just add something like
ImageManager::Add/RemoveStreamImage() and LookupStreamImage().

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h#newcode44
ui/gl/gl_image.h:44: virtual void WillUseTexImage();
please move this above all static methods.

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h#newcode52
ui/gl/gl_image.h:52: unsigned int texture_id);
I prefer if you didn't add this until the usage matches existing GLImage usage.

[no sievers, 2013-08-14 18:16:48]

https://codereview.chromium.org/22824009/diff/39001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/22824009/diff/39001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:924: scoped_refptr<gfx::GLImage>
image(image_manager_->LookupImage(stream_id));
On 2013/08/14 15:22:44, David Reveman wrote:
> LookupStreamImage? to remove the need for AsGLImageStream..

AsGLImageStream allows me to limit the visibility of derived type to here though
(and hopefully I'll be able to remove the need for it in general).

The common base class in the rest of the code works better with the unit tests.

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1401: void
PrepareTexImageForDraw(Texture* texture);
On 2013/08/14 15:22:44, David Reveman wrote:
> PrepareTextureForRender? and maybe not be specific to textures with GLImage
> attached but include all the logic from PrepareTexturesForRender.

Done.

https://codereview.chromium.org/22824009/diff/39001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9492:
image_manager()->AddImage(image, image_id);
On 2013/08/14 15:22:44, David Reveman wrote:
> I thought we decided to track these separably from standard GLImages and use a
> different Id namespace until the usage matches the basic concept of GLImage.
I'm
> ok with using the image manager in the meantime if you just add something like
> ImageManager::Add/RemoveStreamImage() and LookupStreamImage().

See change above. Now that the pattern matches better, can we track them
together?

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h#newcode44
ui/gl/gl_image.h:44: virtual void WillUseTexImage();
On 2013/08/14 15:22:44, David Reveman wrote:
> please move this above all static methods.

Done.

https://codereview.chromium.org/22824009/diff/39001/ui/gl/gl_image.h#newcode52
ui/gl/gl_image.h:52: unsigned int texture_id);
On 2013/08/14 15:22:44, David Reveman wrote:
> I prefer if you didn't add this until the usage matches existing GLImage
usage.

Done.