573626 - move GTK double buffering to the layer manager

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

With retained layers (Bug 564991) there is often no need to double-buffer
drawing to the window.  Bug 564991 includes support to move double buffering to the layer manager, so that it can decide whether double-buffering is necessary, and enables this with GDI.  This bug is for GTK.

Comment 1

[Karl Tomlinson (:karlt)]

Fennec uses 24-bit back buffers for rendering to windows, even on 16-bit systems.
Moving those back buffers out of gtk2/nsWindow.cpp requires moving that
support.
http://hg.mozilla.org/mobile-browser/file/1135faf54be6/app/mobile.js#l108

I could imagine 24-bit might be best for complex painting/compositing.
OTOH when scrolling using a single retained layer, 16-bit seems more
appealing.  But if the retained layers were 16-bit, they might still want
a 24-bit buffer for rendering, and, with multiple layers, they may end up
being composited which may be better done with a 24-bit format.

The most sensible/simple option for force-24bpp seem to be to use 24-bit
buffers from CreateSimilarSurface.  That means that retained layers and a
compositing back-buffer will both use 24-bit operations.  There will just be
a single conversion blitting to the window.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap

When creating Pixmaps it is best to provide a reasonable hint as to what other
Drawable the Pixmap will be used with.  This helps the X server decide whether
to put the Pixmap in video or system memory (and choose which video card's
memory).  The existing GTK widget back-buffering code does this, as does
cairo_surface_create_similar (but it doesn't let us choose the PictFormat).

http://cgit.freedesktop.org/cairo/commit/?id=ac743e25fa7f8bd720219a5c909fe8bbd20b11b2
http://www.mail-archive.com/[e-mail]/msg04428.html

Constructors are a bit inconvenient because they don't fail, so the caller has
to check the surface status.  Passing a Drawable to a constructor would also
make it look version similar to existing wrapper constructors.  A static
"Create" method avoids these issues.

This patch also touches up TakePixmap to handle surfaces that don't pass
Init(), and the error handling code in Init() should not destroy cairo
surfaces that it doesn't own.

gfxXlibSurface(Display *dpy, Visual *visual,
               const gfxIntSize& size, int depth)
is unused and was kindof broken using the default depth for what might not be
a default visual.  With the fix for bug 567065, there isn't much need for it
as it's easy to use XRenderFindVisualFormat to get a PictFormat from a Visual
anyway.

(That ImageFormatRGB16_565 stuff in gfxPlatformGtk::CreateOffscreenSurface
looks scary.)

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: support force-24bpp in gfxXlibSurface::CreateSimilar

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: move GTK double buffering into the layer manager

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Vlad, you could review the patches here.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 452907 [details] [diff] [review]
provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap

Need to touch up screen check and ref-counting here.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 452919 [details] [diff] [review]
move GTK double buffering into the layer manager

That simplifies this code a lot!

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v2

Touched up screen check and ref-counting.
Moved the TakePixmap changes to bug 574158.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v3

The previous version of this patch had a problem that
gfxXlibSurface(cairo_surface_t*) wouldn't set mDrawable appropriately when the
cairo_surface_t was an error surface and so TakePixmap wouldn't have worked.

This patch does things a bit differently to fix that and also adds a replacement for

gfxXlibSurface(Display *dpy, Visual *visual,
               const gfxIntSize& size, int depth)

On displays without the RENDER extension this will still be useful.  On such
displays, image surfaces would probably generally be better than Xlib surfaces, but some things (native themes, plugins) still need Xlib surfaces.
I'm planning to move cairo-xlib-utils.c to C++ so it can use this Create method.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v3.1 [pushed]

updated along m-c, and updated a constructor in gfxQtNativeRenderer.cpp

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we need 567065 to land before we land this?

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: part 2: support force-24bpp in gfxXlibSurface::CreateSimilar

updated along m-c, and reordered so as to apply on top of bug 574220.

(In reply to comment #11)
> Do we need 567065 to land before we land this?

This part 2 would cause native renderer fallback without bug 567065.

Part 1 can land without bug 567065.

My apologies for the interconnectedness of this.
I wasn't expecting bug 567065 to bounce.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: part 3: move GTK double buffering into the layer manager

updated along trunk and reordered to apply on top of bug 574220.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 455392 [details] [diff] [review]
part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v3.1 [pushed]

http://hg.mozilla.org/mozilla-central/rev/23dd52fb1c09

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/0bf406200238
http://hg.mozilla.org/mozilla-central/rev/4abcf73fe93a