Destroy GLX windows when they are backgrounded

Destroy GLX windows when they are backgrounded

Because GLX windows cannot be destroyed separately from their
X windows on all platforms, create a separate child X window to use
with GLX. Destroy this child X window when the backbuffer for the
window is no longer needed.

Because the GL surface may need to be made current while its
backbuffer is destroyed (e.g, to destroy GL resources for a
backgrounded tab), create a dummy 1x1 GL surface which is
never destroyed and can always be made current.

Because the child X window created will cover its parent, create an
event listener to forward expose events from the child window to
the parent, so that the parent can know to repaint.

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

[ccameron, 2013-09-11 09:07:17]

This is the last step in minimizing the GPU memory usage on NVIDIA cards.

After this I will remove their blacklist entry.

[piman, 2013-09-11 23:05:09]

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:408: class XExposeEventForwarder : public
base::MessagePumpObserver {
nit: move this to an anonoymous namespace.

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:414:
base::MessagePumpX11::Current()->RemoveObserver(this);
If this is a lazy instance, the destructor will run during AtExit, at which
point I think base::MessagePumpX11::Current() will be NULL (or worse).
You can make it Leaky, but maybe we could do something along the lines of adding
to the pump when we add the first window pair, and remove it when we remove the
last pair, wdyt?

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:472: if (!XGetWindowAttributes(g_display,
parent_window_, &attributes)) {
Could we cache this? It requires a round trip to X.

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:500: 
Do we need to glXMakeCurrent the dummy_window_ here?

[ccameron, 2013-09-12 01:37:13]

Thanks!!

I've updated the patch a big in the mean time -- I'm using both the front+back
buffer allocation as a signal (like what's done in
IOSurfaceImageTransportSurface) because the meaning of front/back buffer isn't
particularly clear.

I've also updated the memory manager to take some more pressure off on Linux by
only keeping around one backbuffer.

With these changes we have
10 tabs: 192MB -> 107MB (-> 95MB)
20 tabs: 309MB -> 120MB (-> 103MB)
30 tabs: 426MB -> 129MB (-> 107MB)

Of note is that if we share the dummy_window_ we can get down to the (-> XMB)
numbers, but I haven't added that to this patch (more diffs, more moving parts,
etc).

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:408: class XExposeEventForwarder : public
base::MessagePumpObserver {
On 2013/09/11 23:05:09, piman wrote:
> nit: move this to an anonoymous namespace.

Done.

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:414:
base::MessagePumpX11::Current()->RemoveObserver(this);
On 2013/09/11 23:05:09, piman wrote:
> If this is a lazy instance, the destructor will run during AtExit, at which
> point I think base::MessagePumpX11::Current() will be NULL (or worse).
> You can make it Leaky, but maybe we could do something along the lines of
adding
> to the pump when we add the first window pair, and remove it when we remove
the
> last pair, wdyt?

Good catch, thanks. Moved to the Add/RemoveParentChildPair functions.

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:472: if (!XGetWindowAttributes(g_display,
parent_window_, &attributes)) {
On 2013/09/11 23:05:09, piman wrote:
> Could we cache this? It requires a round trip to X.

Actually, we can just use CopyFromParent and avoid caching and querying
altogether.

https://codereview.chromium.org/23452026/diff/9001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:500: 
On 2013/09/11 23:05:09, piman wrote:
> Do we need to glXMakeCurrent the dummy_window_ here?

Doesn't look like it. And I may make dummy_window_ shared in a future patch,
which would make this not even possible.

[ccameron, 2013-09-12 20:27:19]

It looks like the last upload was corrupted -- this one shows diffs now.

(oh, and the try jobs are failing because a dependent change got reverted --
it's going back in as https://codereview.chromium.org/23530050/).

[piman, 2013-09-12 20:35:52]

LGTM. Agreed on discarding the window on !FB_allocation and !BB_allcoation.

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc#n...
ui/gl/gl_surface_glx.cc:354: };
nit: DISALLOW_COPY_AND_ASSIGN.

Maybe DCHECK in the destructor that the map is empty?

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc#n...
ui/gl/gl_surface_glx.cc:494: return;
nit: return at the end of a method, not needed.

[ccameron, 2013-09-13 09:33:38]

Thanks!!

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc#n...
ui/gl/gl_surface_glx.cc:354: };
On 2013/09/12 20:35:52, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN.
> 
> Maybe DCHECK in the destructor that the map is empty?

Done.

https://codereview.chromium.org/23452026/diff/32001/ui/gl/gl_surface_glx.cc#n...
ui/gl/gl_surface_glx.cc:494: return;
On 2013/09/12 20:35:52, piman wrote:
> nit: return at the end of a method, not needed.

Done.

[commit-bot: I haz the power, 2013-09-13 19:03:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/23452026/51001

[commit-bot: I haz the power, 2013-09-13 22:39:55]

Change committed as 223148