574220 - gfxGdkNativeRenderer does not use requested visual for temp surfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

nsNativeThemeGTK expects gfxGdkNativeRenderer to use surfaces with the default
visual, but this doesn't necessarily happen:

http://hg.mozilla.org/mozilla-central/annotate/e910fd948e5b/gfx/thebes/src/cairo-xlib-utils.c#l336

Actually nsNativeThemeGTK uses gtk_widget_get_colormap() but we set that to
gdk_rgb_get_colormap().

http://hg.mozilla.org/mozilla-central/annotate/e910fd948e5b/toolkit/xre/nsAppRunner.cpp#l3211

We should pass this visual to the native renderer.

Comment 1

[Karl Tomlinson (:karlt)]

Regression from bug 422221.

Comment 2

[Karl Tomlinson (:karlt)]

I think this is what is caused the following issues on try-linux-slave23.build.mozilla.org DISPLAY=:2 with the patch for bug 567065:

(Gecko:4142): Gtk-CRITICAL **: gtk_paint_box: assertion `style->depth == gdk_drawable_get_depth (window)' failed
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/tryserver-linux-debug-unittest-reftest/build/reftest/tests/layout/reftests/css-default/submit-button/default-submit-button-1.html |

###!!! ABORT: X_PolyFillRectangle: BadMatch (invalid parameter attributes); sync; id=0x0

TEST-UNEXPECTED-FAIL | file:///builds/slave/tryserver-linux-debug-unittest-reftest/build/reftest/tests/content/test/reftest/bug559996-ref.html | Exited with code 6 during test run

_XError (/usr/src/debug/libX11-1.0.3/src/XlibInt.c:2890)
_XReply (/usr/src/debug/libX11-1.0.3/src/XlibInt.c:1818)
XSync (/usr/src/debug/libX11-1.0.3/src/Sync.c:50)
_XSyncFunction (/usr/src/debug/libX11-1.0.3/src/Synchro.c:39)
XFillRectangle (/usr/src/debug/libX11-1.0.3/src/FillRct.c:97)
gdk_x11_draw_rectangle (/usr/src/debug/gtk+-2.10.4/gdk/x11/gdkdrawable-x11.c:445)
IA__gdk_draw_rectangle (/usr/src/debug/gtk+-2.10.4/gdk/gdkdraw.c:408)
gdk_pixmap_draw_rectangle (/usr/src/debug/gtk+-2.10.4/gdk/gdkpixmap.c:254)
IA__gdk_draw_rectangle (/usr/src/debug/gtk+-2.10.4/gdk/gdkdraw.c:408)
IA__gtk_style_apply_default_background (/usr/src/debug/gtk+-2.10.4/gtk/gtkstyle.c:2039)
17  libxul.so!moz_gtk_scrollbar_trough_paint [gtk2drawing.c:27ae439c66c1 : 1284 + 0x4f]
    eip = 0x0206dca1   esp = 0xbfcb8ff0   ebp = 0xbfcb9048
    Found by: previous frame's frame pointer
18  libxul.so!moz_gtk_widget_paint [gtk2drawing.c:27ae439c66c1 : 3191 + 0x2d]
    eip = 0x02072592   esp = 0xbfcb9050   ebp = 0xbfcb9088   ebx = 0x02b477f0
    esi = 0xbfcb93d8   edi = 0x00000005
    Found by: call frame info
19  libxul.so!ThemeRenderer::NativeDraw [nsNativeThemeGTK.cpp:27ae439c66c1 : 653 + 0x40]
    eip = 0x020aa025   esp = 0xbfcb9090   ebp = 0xbfcb90e8   ebx = 0x02b477f0
    esi = 0xbfcb93d8   edi = 0x00000005
    Found by: call frame info
20  libxul.so!NativeRendering [gfxGdkNativeRenderer.cpp:27ae439c66c1 : 67 + 0x37]
    eip = 0x02472c64   esp = 0xbfcb90f0   ebp = 0xbfcb9148   ebx = 0x02b477f0
    esi = 0xbfcb92e8   edi = 0x020a9f1c
    Found by: call frame info
21  libxul.so!_draw_onto_temp_xlib_surface [cairo-xlib-utils.c:27ae439c66c1 : 389 + 0x31]
    eip = 0x02434352   esp = 0xbfcb9150   ebp = 0xbfcb9188   ebx = 0x02b477f0
    esi = 0x000000c8   edi = 0x0000000f
    Found by: call frame info
22  libxul.so!cairo_draw_with_gdk [cairo-xlib-utils.c:27ae439c66c1 : 541 + 0x1e]
    eip = 0x0243474e   esp = 0xbfcb9190   ebp = 0xbfcb91e8   ebx = 0x02b477f0
    esi = 0x000000c8   edi = 0x0000000f
    Found by: call frame info
23  libxul.so!gfxGdkNativeRenderer::Draw [gfxGdkNativeRenderer.cpp:27ae439c66c1 : 110 + 0x7d]
    eip = 0x02472a68   esp = 0xbfcb91f0   ebp = 0xbfcb9298   ebx = 0x02b477f0
    esi = 0x000000c8   edi = 0x0000000f
    Found by: call frame info
24  libxul.so!nsNativeThemeGTK::DrawWidgetBackground [nsNativeThemeGTK.cpp:27ae439c66c1 : 807 + 0x37]
    eip = 0x020abb12   esp = 0xbfcb92a0   ebp = 0xbfcb9438   ebx = 0x02b477f0
    esi = 0x000000c8   edi = 0x0000000f
    Found by: call frame info
25  libxul.so!nsCSSRendering::PaintBackgroundWithSC [nsCSSRendering.cpp:27ae439c66c1 : 2153 + 0x3e]
    eip = 0x01137250   esp = 0xbfcb9440   ebp = 0xbfcb95a8   ebx = 0x02b477f0
    esi = 0x09120b04   edi = 0x0b115ab8
    Found by: call frame info
26  libxul.so!nsCSSRendering::PaintBackground [nsCSSRendering.cpp:27ae439c66c1 : 1474 + 0x4c]
    eip = 0x01137904   esp = 0xbfcb95b0   ebp = 0xbfcb95f8   ebx = 0x02b477f0
    esi = 0x09120b04   edi = 0x0b115ab8
    Found by: call frame info
27  libxul.so!nsDisplayBackground::Paint [nsDisplayList.cpp:27ae439c66c1 : 769 + 0x79]
    eip = 0x0114a415   esp = 0xbfcb9600   ebp = 0xbfcb9658   ebx = 0x02b477f0
    esi = 0x09120b04   edi = 0x0b115ab8
    Found by: call frame info
28  libxul.so!mozilla::FrameLayerBuilder::DrawThebesLayer [FrameLayerBuilder.cpp:27ae439c66c1 : 532 + 0x27]
    eip = 0x011039a3   esp = 0xbfcb9660   ebp = 0xbfcb96d8   ebx = 0x02b477f0
    esi = 0x0114a30e   edi = 0x020dd1f2
    Found by: call frame info
29  libxul.so!mozilla::layers::BasicThebesLayer::Paint [BasicLayers.cpp:27ae439c66c1 : 264 + 0x22]
    eip = 0x0247aeaf   esp = 0xbfcb96e0   ebp = 0xbfcb9708   ebx = 0x02b477f0
    esi = 0x0928c4ec   edi = 0x0247add3
    Found by: call frame info
30  libxul.so!mozilla::layers::BasicLayerManager::PaintLayer [BasicLayers.cpp:27ae439c66c1 : 679 + 0x37]
    eip = 0x0247b9bc   esp = 0xbfcb9710   ebp = 0xbfcb9848   ebx = 0x02b477f0
    esi = 0x0928c4ec   edi = 0x0247add3
    Found by: call frame info
31  libxul.so!mozilla::layers::BasicLayerManager::PaintLayer [BasicLayers.cpp:27ae439c66c1 : 682 + 0x1f]
    eip = 0x0247b9f3   esp = 0xbfcb9850   ebp = 0xbfcb9988   ebx = 0x02b477f0
    esi = 0x092b5da0   edi = 0x0247cffe
    Found by: call frame info
32  libxul.so!mozilla::layers::BasicLayerManager::EndTransaction [BasicLayers.cpp:27ae439c66c1 : 573 + 0x2c]
    eip = 0x0247bd34   esp = 0xbfcb9990   ebp = 0xbfcb99a8   ebx = 0x02b477f0
    esi = 0x0905b708   edi = 0x0247b15c
    Found by: call frame info
33  libxul.so!nsDisplayList::PaintForFrame [nsDisplayList.cpp:27ae439c66c1 : 466 + 0x29]
    eip = 0x0114b15a   esp = 0xbfcb99b0   ebp = 0xbfcb9a48   ebx = 0x02b477f0
    esi = 0x0905b708   edi = 0x0247b15c
    Found by: call frame info
34  libxul.so!nsDisplayList::PaintRoot [nsDisplayList.cpp:27ae439c66c1 : 414 + 0x30]
    eip = 0x0114b323   esp = 0xbfcb9a50   ebp = 0xbfcb9a68   ebx = 0x02b477f0
    esi = 0x020d7c96   edi = 0x00000000
    Found by: call frame info
35  libxul.so!PresShell::RenderDocument [nsPresShell.cpp:27ae439c66c1 : 5332 + 0x32]
    eip = 0x011943a7   esp = 0xbfcb9a70   ebp = 0xbfcb9e18   ebx = 0x02b477f0
    esi = 0x020d7c96   edi = 0x00000000
    Found by: call frame info
36  libxul.so!nsCanvasRenderingContext2D::DrawWindow [nsCanvasRenderingContext2D.cpp:27ae439c66c1 : 3448 + 0x40]
    eip = 0x0152b6bd   esp = 0xbfcb9e20   ebp = 0xbfcb9f28   ebx = 0x02b477f0
    esi = 0x01193e1c   edi = 0x00000000
    Found by: call frame info

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: add gfxXlibSurface::GetVisualAndColormap()

I'm refactoring gfxXlibNativeRenderer, and need a way to get a Colormap for
windowless plugins.  I'd like to do this in a way that can be used by both Qt
and GTK ports.  Requesting gfxXlibSurface for an appropriate colormap is an
appealing API.

However we don't want a colormap on every surface as these use ~ 20kB each and
there is a little overhead in creating them.  A more significant issue is that
the plugin process will sometimes need to leak resources for each colormap id
that it is given (bug 569775).

Therefore a table is kept of colormaps allocated on the display and these are
shared between surfaces of similar format.

XESetCloseDisplay is used to clean up the table at the appropriate time.
This is the same function that cairo-xlib-display.c uses.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: part 2: remove gfxXlibNativeRenderer.cpp

The old gfxXlibNativeRenderer.cpp was mostly a wrapper around
cairo-xlib-utils.c (and unused as of bug 422221).

cairo-xlib-utils.c was written with the intention of including it in cairo,
but this seems unlikely to happen.

I'd like to create gfxXlibSurfaces from the native renderer so I want it to be
in a C++ file.  (In the future we should also move over the image surfaces and
use gfxAlphaRecovery.)

This patch just removes the existing gfxXlibNativeRenderer.cpp wrapper.
The next patch will move cairo-xlib-utils.c to gfxXlibNativeRenderer.cpp.
(mq, at least, gets confused when these are in the same patch.)

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: part 3: refactor gfxXlibNativeRenderer and make sure the surface matches the requested visual

This is the main patch.

gfxXlibNativeRenderer.cpp mostly resembles the old cairo-xlib-utils.c, with
some C++ and API touch ups.  Plugin rendering uses gfxXlibNativeRenderer
directly for the GTK port and gfxQtNativeRenderer now has the same API as
gfxXlibNativeRenderer.

(The Qt port could actually use gfxXlibNativeRenderer, but I haven't tested so
don't want to switch over in this patch.  For RENDER_BUFFERED with
QPaintEngine::X11 it would be a win.  For RENDER_QPAINTER, adding some
detection of qpainter surface engines could enable the fast path.)

gfxGdkNativeRenderer is now a special purpose renderer for GTK themes. 
On X11, it derives from gfxXlibNativeRenderer.
On DirectFB, it still uses the old half-implementation.

The functional changes here are:

* The temp surface is created to match the requested visual, if alternate
  visuals are not supported.

* The requested visual is provided to the native renderer, to
  handle when GTK does not use the default visual.
  (.e.g. https://bugs.launchpad.net/ubuntu/+bug/491521)

* If alternate visuals are not supported, the direct path is now taken
  whenever the requested visual can be used on the surface, even if the visual
  does not have the same id as that returned from
  cairo_xlib_surface_get_visual().  This will enable us to use visuals with
  different GLX properties on our windows.

Other minor changes:

* I removed DRAW_SUPPORTS_OFFSET because we always set that flag (and I don't
  see us needing to unset it in the near future).

* gdk_pixmap_foreign_new_for_screen is now done in the GdkNativeRenderer where
  it knows the colormap, instead of in gfxPlatformGtk::GetGdkDrawable which
  had to hunt for a colormap.

* nsPluginInstanceOwner::Paint is now not compiled (or used) for DirectFB.
  It's implementation was never completed.

* gfxPlatformGtk::SetGdkDrawable() is now a static method.

Comment 6

[Karl Tomlinson (:karlt)]

part 3 here is written on top of part 1 of bug 573626.

Comment 7

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

Comment on attachment 455362 [details] [diff] [review]
part 3: refactor gfxXlibNativeRenderer and make sure the surface matches the requested visual

+# include "gfxXlibNativeRenderer.h"

Lose the space after #

+    cairo_bool_t DrawDirect(cairo_t *cr, nsIntSize bounds,
+                            PRUint32 flags, Screen *screen, Visual *visual);
+
+    cairo_bool_t DrawOntoTempSurface(gfxXlibSurface *tempXlibSurface,
+                                     double background_gray_value);

PRBool here I guess. cairo_*_t is tolerable in gfxXlibNativeRenderer.cpp for now but it's not good to leak to a .h file, even if these are private methods. We should probably clean it up completely in another patch though.

+    bool supports_alternate_visual = flags & DRAW_SUPPORTS_ALTERNATE_VISUAL;
+    bool supports_alternate_screen = supports_alternate_visual
+        && (flags & DRAW_SUPPORTS_ALTERNATE_SCREEN);

PRBool, then I think you need (flags & DRAW_SUPPORTS_ALTERNATE_VISUAL) != 0

+        bool supports_alternate_visual =
+            flags & gfxXlibNativeRenderer::DRAW_SUPPORTS_ALTERNATE_VISUAL;
+        bool supports_alternate_screen = supports_alternate_visual
+            && (flags & gfxXlibNativeRenderer::DRAW_SUPPORTS_ALTERNATE_SCREEN);

ditto

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: part 3: refactor gfxXlibNativeRenderer and make sure the surface matches the requested visual

Updated to m-c.
Applies on top of part 1 of bug 573626, and on top of bug 574583.

Comment 9

[Karl Tomlinson (:karlt)]

(Addressed review comments, too.)

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 1: add gfxXlibSurface::GetVisualAndColormap()

Updated to trunk, and reordered in my queue so as to apply before part 2 of bug 573626.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: part 1: add PRBool gfxXlibSurface::GetVisualAndColormap()

bool -> PRBool, at roc's request.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 455414 [details] [diff] [review]
part 3: refactor gfxXlibNativeRenderer and make sure the surface matches the requested visual

>+    GdkRectangle clipRect;
>+    if (numClipRects) {
>+        NS_ASSERTION(numClipRects == 1, "Too many clip rects");
>+        clipRect.x = clipRects[0].x;
>+        clipRect.x = clipRects[0].y;

We're not actually using the path (yet) but that last line should be clipRect.y.

Comment 13

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/643e585512f1
http://hg.mozilla.org/mozilla-central/rev/e76b299bc8cd
http://hg.mozilla.org/mozilla-central/rev/e815780a1e67