445250 - cairo_draw_with_xlib should provide a non-NULL visual to callback (crash with windowless plugins and opacity < 1)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

With opacity < 1 the current target surface has generally been created with _cairo_xlib_surface_create_similar_with_format.  For such surfaces, cairo_xlib_surface_get_visual returns NULL because the surface is created with a xrender_format rather than a visual.  cairo_draw_with_xlib should not pass this NULL visual to the callback.

(Trying to draw with a NULL visual causes problems for windowless plugins.)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: moonlight testcase

(modified from http://www.communitymx.com/content/source/E5141/wmodetrans.htm)

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> (modified from http://www.communitymx.com/content/source/E5141/wmodetrans.htm)

I mean http://www.designwithsilverlight.com/tutorials/LCDClock/Default.html

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: flash testcase

(modified from http://www.communitymx.com/content/source/E5141/wmodetrans.htm)

Don't open unless you want to reproduce a crash.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: use a Visual matching the PictFormat or fall back

This is what I think is the best fix.  If a visual matching the PictFormat is
found then that is passed to the callback; otherwise it falls back the slow
path.

Builds including this fix are available here:

https://build.mozilla.org/tryserver-builds/2008-07-13_18:[e-mail]-null-visual-dirty-clip/

For the case where the PictFormat is PictStandardARGB32 (as in the test cases
here):

There is no matching visual if the Composite extension is not enabled on the X
server or if the environment contains XLIB_SKIP_ARGB_VISUALS=1.  Therefore the
slow path is taken.  In this situation

* The Moonlight and Adobe plugins render as expected (but slower).

* Swfdec renders with a dark (instead of transparent) background (which is
  applied with alpha < 1).  This is an issue that I've seen before with Swfdec
  and the slow path.

When Composite is enabled and the ARGB visual is not disabled through
XLIB_SKIP_ARGB_VISUALS:

* The Moonlight plugin renders as expected.

* The Swfdec plugin (current git) produces a "BadMatch (invalid parameter
  attributes)".  This is due to using the wrong Visual (from the toplevel
  window), which does not match the Drawable.  The results are as expected with
  the small patch at https://bugs.freedesktop.org/show_bug.cgi?id=16717

* The Adobe beta plugin (d525) also produces a BadMatch (invalid parameter
  attributes)", which is due to calling gdk_draw_image with a GdkImage and
  GdkDrawable of different depths.

#2  0xb6adebfa in _XError (dpy=0xb6327800, rep=0xbfb11b50)
    at ../../src/XlibInt.c:2907
#3  0xb6ae06c4 in _XReply (dpy=0xb6327800, rep=0xbfb11b50, extra=0, discard=1)
    at ../../src/XlibInt.c:1833
#4  0xb6ada5da in XSync (dpy=0xb6327800, discard=0) at ../../src/Sync.c:48
#5  0xb6ada755 in _XSyncFunction (dpy=0xb6327800) at ../../src/Synchro.c:37
#6  0xb6ad3382 in XPutImage (dpy=0xb6327800, d=52429730, gc=0x9e21eb70,
    image=0x9e2a5620, req_xoffset=0, req_yoffset=0, x=1, y=1, req_width=200,
    req_height=200) at ../../src/PutImage.c:1032
#7  0xb6da85c7 in gdk_x11_draw_image (drawable=0xb5b8d630, gc=0xa0357830,
    image=0xad6b2cf8, xsrc=0, ysrc=0, xdest=1, ydest=1, width=200, height=200)
    at /build/buildd/gtk+2.0-2.12.0/gdk/x11/gdkdrawable-x11.c:800
#8  0xb6d802a2 in IA__gdk_draw_image (drawable=0xb5b8d630, gc=0xa0357830,
    image=0xad6b2cf8, xsrc=0, ysrc=0, xdest=1, ydest=1, width=200, height=200)
    at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
#9  0xb6d8b8bc in gdk_pixmap_draw_image (drawable=0x9e20ec30, gc=0xa0357830,
    image=0xad6b2cf8, xsrc=0, ysrc=0, xdest=1, ydest=1, width=200, height=200)
    at /build/buildd/gtk+2.0-2.12.0/gdk/gdkpixmap.c:407
#10 0xb6d802a2 in IA__gdk_draw_image (drawable=0x9e20ec30, gc=0xa0357830,
    image=0xad6b2cf8, xsrc=0, ysrc=0, xdest=1, ydest=1, width=200, height=200)
    at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
#11 0x9f7a7bb0 in ?? () from /home/karl/flash/d525/libflashplayer.so
#12 0x9f79bd8a in ?? () from /home/karl/flash/d525/libflashplayer.so
#13 0x9f792d80 in ?? () from /home/karl/flash/d525/libflashplayer.so
#14 0x9f7978b4 in Private_HandleEvent ()

(gdb) f 10
#10 0xb6d802a2 in IA__gdk_draw_image (drawable=0x9e20ec30, gc=0xa0357830,
    image=0xad6b2cf8, xsrc=0, ysrc=0, xdest=1, ydest=1, width=200, height=200)
    at /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c:703
703     in /build/buildd/gtk+2.0-2.12.0/gdk/gdkdraw.c
(gdb) p gdk_drawable_get_depth(drawable)
$10 = 32
(gdb) p *image
$11 = {parent_instance = {g_type_instance = {g_class = 0xad7c2f60},
    ref_count = 1, qdata = 0x0}, type = GDK_IMAGE_NORMAL, visual = 0xb633a810,
  byte_order = GDK_LSB_FIRST, width = 200, height = 200, depth = 24, bpp = 4,
  bpl = 800, bits_per_pixel = 32, mem = 0x9e581000, colormap = 0x0,
  windowing_data = 0xad6b2d30}

Comment 5

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

Would it make more sense to put _visual_from_xrender_format in cairo?

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: fall back when cairo_xlib_surface_get_visual returns NULL [pushed]

(In reply to comment #5)
> Would it make more sense to put _visual_from_xrender_format in cairo?

Yes.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: return a suitable visual from cairo_xlib_surface_get_visual if available

There are only two places where xlib surfaces are created without a visual (and there is a suitable visual), so it seems sensible to plug these.  _get_image_surface and _draw_image_surface also examine the visual so this information is useful before cairo_xlib_surface_get_visual.
 
I'll see if I can upstream this to cairo after I see if there is any feedback on
http://lists.cairographics.org/archives/cairo/2008-July/014489.html

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 329786 [details] [diff] [review]
return a suitable visual from cairo_xlib_surface_get_visual if available

submitted to cairo:

http://marc.info/?l=cairo&m=121624409609428&w=2

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 329784 [details] [diff] [review]
fall back when cairo_xlib_surface_get_visual returns NULL [pushed]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/f84c0fa10290

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 329784 [details] [diff] [review]
fall back when cairo_xlib_surface_get_visual returns NULL [pushed]

This is a safe NULL check that we should take on 1.9.0 to solve the problem reported here.

This patch does not contain the cairo changes and so will not show the ARGB issues in comment 4.

Automated testing depends on having a windowless plugin in our source tree
(bug 386144), so I've tested manually on the testcases here.

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 329784 [details] [diff] [review]
fall back when cairo_xlib_surface_get_visual returns NULL [pushed]

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 329784 [details] [diff] [review]
fall back when cairo_xlib_surface_get_visual returns NULL [pushed]

Checked in to CVS:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1217794624&maxdate=1217794742&who=karlt%2B%25karlt.net