All users were logged out of Bugzilla on October 13th, 2018

move GTK double buffering to the layer manager

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({perf})

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Comment 2

8 years ago
Created attachment 452907 [details] [diff] [review]
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/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?
(Assignee)

Updated

8 years ago
Depends on: 567065
(Assignee)

Updated

8 years ago
Attachment #452907 - Flags: review? → review?(jmuizelaar)
(Assignee)

Comment 3

8 years ago
Created attachment 452909 [details] [diff] [review]
support force-24bpp in gfxXlibSurface::CreateSimilar
Attachment #452909 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

8 years ago
Created attachment 452919 [details] [diff] [review]
move GTK double buffering into the layer manager
Attachment #452919 - Flags: review?(roc)
Vlad, you could review the patches here.
(Assignee)

Comment 6

8 years ago
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+
(Assignee)

Updated

8 years ago
Depends on: 574158
(Assignee)

Comment 8

8 years ago
Created attachment 453593 [details] [diff] [review]
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.
Attachment #452907 - Attachment is obsolete: true
Attachment #453593 - Flags: superreview?(roc)
Attachment #453593 - Flags: review?(vladimir)
Attachment #453593 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

8 years ago
Attachment #453593 - Flags: review?(vladimir)
(Assignee)

Comment 9

8 years ago
Created attachment 454473 [details] [diff] [review]
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.
Attachment #453593 - Attachment is obsolete: true
Attachment #454473 - Flags: review?(vladimir)
(Assignee)

Comment 10

8 years ago
Created attachment 455392 [details] [diff] [review]
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
Attachment #454473 - Attachment is obsolete: true
Do we need 567065 to land before we land this?
(Assignee)

Comment 12

8 years ago
Created attachment 455418 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Attachment #452909 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Created attachment 455419 [details] [diff] [review]
part 3: move GTK double buffering into the layer manager

updated along trunk and reordered to apply on top of bug 574220.
Attachment #452919 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
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.