Closed Bug 573626 Opened 9 years ago Closed 9 years ago

move GTK double buffering to the layer manager

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: perf)

Attachments

(3 files, 5 obsolete files)

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.
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.
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/xorg-devel@lists.x.org/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.)
Attachment #452907 - Flags: superreview?(roc)
Attachment #452907 - Flags: review?
Depends on: 567065
Attachment #452907 - Flags: review? → review?(jmuizelaar)
Attachment #452909 - Flags: review?(jmuizelaar)
Attachment #452919 - Flags: review?(roc)
Vlad, you could review the patches here.
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.
Attachment #452907 - Flags: superreview?(roc)
Attachment #452907 - Flags: review?(jmuizelaar)
Comment on attachment 452919 [details] [diff] [review]
move GTK double buffering into the layer manager

That simplifies this code a lot!
Attachment #452919 - Flags: review?(roc) → review+
Depends on: 574158
Touched up screen check and ref-counting.
Moved the TakePixmap changes to bug 574158.
Attachment #452907 - Attachment is obsolete: true
Attachment #453593 - Flags: superreview?(roc)
Attachment #453593 - Flags: review?(vladimir)
Attachment #453593 - Flags: superreview?(roc) → superreview+
Attachment #453593 - Flags: review?(vladimir)
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.
Attachment #453593 - Attachment is obsolete: true
Attachment #454473 - Flags: review?(vladimir)
updated along m-c, and updated a constructor in gfxQtNativeRenderer.cpp
Attachment #454473 - Attachment is obsolete: true
Do we need 567065 to land before we land this?
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.
Attachment #452909 - Attachment is obsolete: true
updated along trunk and reordered to apply on top of bug 574220.
Attachment #452919 - Attachment is obsolete: true
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
Attachment #455392 - Attachment description: part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v3.1 → part 1: provide an API to create Pixmap gfxXlibSurfaces with a Drawable hint for XCreatePixmap v3.1 [pushed]
You need to log in before you can comment on or make changes to this bug.