Use a singleton 1x1 offscreen surface for all TextureImageTransportSurface instances.

Use a singleton 1x1 offscreen surface for all TextureImageTransportSurface instances.

This is to reduce the number of dummy offscreen surfaces created for the purpose of
making TextureImageTransportSurface current.

BUG=NONE
TEST=manually on daisy; verified that the dummy surface only gets allocated when first needed and
destroyed when the last instance of TextureImageTrasnportSurface is destroyed.


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

[hshi1, 2012-09-24 21:10:55]

Please take a look. I'm not sure if it is the most elegant way to do this (maybe
there is a Singleton template that can do this more cleanly), but I've verified
that it is correct. Thanks!

[piman, 2012-09-24 21:18:35]

The way I would approach the problem is by adding a 1x1 offscreen surface into a
global class, such as the GpuChannelManager. It could be created on demand. I
would also fix up other places where it makes sense, e.g.
GpuCommandBufferStub::OnInitialize to catch all offscreen contexts.

http://codereview.chromium.org/10984004/diff/1/content/common/gpu/texture_ima...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10984004/diff/1/content/common/gpu/texture_ima...
content/common/gpu/texture_image_transport_surface.cc:25:
scoped_refptr<gfx::GLSurface> TextureImageTransportSurface::surface_;
We don't allow non-POD globals.

[apatrick_chromium, 2012-09-24 21:26:42]

When I last tried something like this, I ran into an issue where, once a
drawable had been made current with a particular GL context, some GLX + nVidia
drivers fail when the drawable is made current with a different context. This
was a year or so ago. I think backer might have hit the issue when working on
touch UI as well.

[hshi1, 2012-09-24 21:38:40]

http://codereview.chromium.org/10984004/diff/1/content/common/gpu/texture_ima...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10984004/diff/1/content/common/gpu/texture_ima...
content/common/gpu/texture_image_transport_surface.cc:25:
scoped_refptr<gfx::GLSurface> TextureImageTransportSurface::surface_;
On 2012/09/24 21:18:36, piman wrote:
> We don't allow non-POD globals.

In Patch Set #2 I've moved the global singleton to GpuChannelManager, does this
work for you?

Note that I've yet to address your comment regarding
GpuCommandBufferStub::OnInitialize(). Thanks.

[hshi1, 2012-09-24 21:44:16]

On 2012/09/24 21:26:42, apatrick_chromium wrote:
> When I last tried something like this, I ran into an issue where, once a
> drawable had been made current with a particular GL context, some GLX + nVidia
> drivers fail when the drawable is made current with a different context. This
> was a year or so ago. I think backer might have hit the issue when working on
> touch UI as well.

Didn't see this comment earlier. If this is the case then it would seem that
this CL simply wouldn't work.

[apatrick_chromium, 2012-09-24 21:51:22]

You could test to see if it is still the case. This was over a year ago and it
might have been fixed. It would have been whatever driver Goobuntu was using at
the time.

[hshi1, 2012-09-24 22:14:08]

How frequently (or rarely) did you run into the original problem? I'm not seeing
any issues on Goobuntu and on my daisy device so far, but I don't feel confident
with just half an hour of casual testing.

On 2012/09/24 21:51:22, apatrick_chromium wrote:
> You could test to see if it is still the case. This was over a year ago and it
> might have been fixed. It would have been whatever driver Goobuntu was using
at
> the time.

[apatrick_chromium, 2012-09-24 22:29:10]

It happened 100% for the affected driver. I don't remember the version. I only
saw it with nVidia. The linux_gpu trybot is nVidia if you want to test it on
something else.

[hshi1, 2012-09-24 23:45:19]

On 2012/09/24 22:29:10, apatrick_chromium wrote:
> It happened 100% for the affected driver. I don't remember the version. I only
> saw it with nVidia. The linux_gpu trybot is nVidia if you want to test it on
> something else.

I'm afraid this is still failing the linux_gpu tests.

[apatrick_chromium, 2012-09-25 00:04:06]

Those 4 tests were failing before. Without any evidence that there is still an
issue, I think it's okay to land this and watch for suspicious bug reports. I
think we'll get GLX errors in the about:gpu log if there's an issue. LGTM for my
part.

[hshi1, 2012-09-25 00:06:27]

Thanks. Piman@ - I'm still not sure how to handle the
GpuCommandBufferStub::OnInitialize() as you mentioned in #2. Would it be ok if
this gets landed first, then we handle that as a separate CL? Thanks.

[hshi1, 2012-09-25 00:06:27]

Thanks. Piman@ - I'm still not sure how to handle the
GpuCommandBufferStub::OnInitialize() as you mentioned in #2. Would it be ok if
this gets landed first, then we handle that as a separate CL? Thanks.

[piman, 2012-09-25 00:12:18]

Also, because you haven't changed GpuCommandBufferStub to use this too, the gpu
bots don't give you any data, since they don't exercise this path.

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/gpu_cha...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/gpu_cha...
content/common/gpu/gpu_channel_manager.cc:191: if
(make_current_surface_->HasOneRef()) {
This is bogus, because you don't have a way to know if the last person to call
this has released its last reference (think posted tasks).

I don't think there's any good reason not to hold on to the surface once it's
been created, until the GpuChannelManager gets destroyed.

So I would do instead:
gfx::GLSurface* GpuChannelManager::GetDefaultOffscreenSurface() {
  if (!default_offscreen_surface_.get()) {
    default_offscreen_surface_ = gfx::GLSurface::CreateOffscreenGLSurface(false,
gfx::Size(1, 1));
  }
  return default_offscreen_surface_.get();
}

And Destroy() in the destructor, after releasing the channels.

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.h (right):

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:115: GLSurface* surface_;
Keep this a refcount - there's no reason not to.

[hshi1, 2012-09-25 00:26:33]

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/gpu_cha...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/gpu_cha...
content/common/gpu/gpu_channel_manager.cc:191: if
(make_current_surface_->HasOneRef()) {
On 2012/09/25 00:12:18, piman wrote:
> This is bogus, because you don't have a way to know if the last person to call
> this has released its last reference (think posted tasks).
> 
> I don't think there's any good reason not to hold on to the surface once it's
> been created, until the GpuChannelManager gets destroyed.
> 
> So I would do instead:
> gfx::GLSurface* GpuChannelManager::GetDefaultOffscreenSurface() {
>   if (!default_offscreen_surface_.get()) {
>     default_offscreen_surface_ =
gfx::GLSurface::CreateOffscreenGLSurface(false,
> gfx::Size(1, 1));
>   }
>   return default_offscreen_surface_.get();
> }
> 
> And Destroy() in the destructor, after releasing the channels.

Done.

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.h (right):

http://codereview.chromium.org/10984004/diff/11001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:115: GLSurface* surface_;
On 2012/09/25 00:12:18, piman wrote:
> Keep this a refcount - there's no reason not to.

Done.

[piman, 2012-09-25 00:33:07]

LGTM, assuming it passes the gpu bots. Thanks!

[commit-bot: I haz the power, 2012-09-25 01:13:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10984004/10005

[commit-bot: I haz the power, 2012-09-25 03:15:04]

Change committed as 158512