488250 - rotated or scaled widgets using moz-transform are not drawn

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Timothy Nikkel (:tnikkel)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8
Build Identifier: 

Widgets (button, scrollbars) that are transformed either using moz-transform or that are in a svg foreign object that has been transformed are not drawn. Translations work fine, but rotations or scales expose the problem. This only happens in mozilla-central, not 1.9.1. The problem first appears in the 2009-03-21 nightly, it is not in the 2009-03-20 nightly. I've only been able to reproduce it when connecting to a ubuntu linux box through vnc; locally on the box's monitor the problem does not show up.

Reproducible: Always

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: testcase

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: how the testcase appears

Comment 3

[Timothy Nikkel (:tnikkel)]

At /gfx/cairo/cairo/src/cairo-xlib-surface.c:713 the call to _cairo_image_surface_create_with_pixman_format has pixman_format equal to PIXMAN_x8b8g8r8, and this eventually gets to _cairo_image_surface_create_for_pixman_image at /gfx/cairo/cairo/src/cairo-image-surface.c:143 where we try to convert from a pixman format to a cairo format using the function _cairo_format_from_pixman_format (cairo-image-surface.c:41) which returns CAIRO_FORMAT_INVALID for pixman_format = PIXMAN_x8b8g8r8. This error eventually trickles back and nothing gets drawn. I don't know if this is the real issue, it might be that this is a fallback path and the main path shouldn't be failing.

Based on this and the regression range, it seems likely this was caused by bug 484076.

Comment 4

[Timothy Nikkel (:tnikkel)]

At http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#708 we try to get a pixman format from masks representing the R, G, B, and A channels, if this succeeds (ie there is a pixman format for those specific masks) then we call _cairo_image_surface_create_with_pixman_format to create a cairo image surface with that pixman format. Except a cairo image surface needs a cairo format and not a pixman format (cairo supports just a few common formats, pixman supports a bunch of less common ones), so _cairo_image_surface_create_with_pixman_format will fail if there is not a cairo format corresponding to the given pixman format. There is a fallback path for just such a case but we don't hit it because we only check if we can construct the pixman format from the masks, and not that we can represent that pixman format as a cairo format.

I put a check here for the pixman format that I was seeing (PIXMAN_x8b8g8r8) so it would use the fallback path and that fixed it for me. But a more general fix would be to call something like _cairo_format_from_pixman_format. There are a couple of other calls to _cairo_image_surface_create_with_pixman_format and _pixman_format_from_masks that probably need a similar treatment.

This logic didn't change in the aforementioned cairo/pixman update, so something else must have changed that gives us the non-cairo-representable PIXMAN_x8b8g8r8 format.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This patch fixes the problem for me.

There is still a similar (potential) error in cairo-xcb-surface.c. Namely at http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xcb-surface.c#437 This else is a waste of time, as it says, it will always fail. It should do what cairo-xlib-surface.c does in the same situation at http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#726 And since this is the only place where _cairo_image_surface_create_with_masks is called, that function can be removed once this is fixed.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I'm not really that familiar with this code so it would be good to figure out why the cairo upgrade broke it.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Chris Wilson was the last person to touch that code...

Comment 8

[Chris Wilson]

Nothing has obviously changed that could have started us attempting to use xBGR image fallbacks. We rely on pixman_format_supported_destination() to effectively filter pixel formats that we are unable to draw to, which should be sufficient for our uses (i.e. for temporary fallbacks that are never publically exposed).

In fact, I'm tempted to simplify the [xlib] fallback to use any supported destination pixman_format and fixup internal assumptions about the pixel format. Skimming the code, there seem like there are only a few spots that require fixing - though none look responsible for causing this drawing error.

Time for a bit of experimentation with vnc to see if I can trigger this error. tn, if you have the opportunity, can you attach gdb and put a breakpoint on _cairo_error()? (I think the patch is just a band-aid for an underlying bug.)

Comment 9

[Chris Wilson]

I started a BGR server using "vncserver -pixelformat bgr888" and confirmed that we do indeed create an image surface with PIXMAN_x8b8g8r8 and CAIRO_FORMAT_INVALID (cairo-xlib-surface.c, line ~712), but I have not seen any ill effects. [Though this was using my local cairo tree...]

tn can you clarify the symptoms of your bug? And if you can catch a backtrace for _cairo_error(), that would be great!

Comment 10

[Timothy Nikkel (:tnikkel)]

Thanks for looking into this.

Can you not reproduce using the testcase attached to this bug? Only rotated or scaled widgets expose the problem, which you are very unlikely to encounter unless you specifically look for them.

I setup vnc using this guide http://www.movingtofreedom.org/2007/02/16/howto-remote-desktop-with-vnc-in-ubuntu-edgy-gnu-linux/ except I changed the depth to 24, so I don't think I'm doing anything out of the ordinary for my vnc server. When connected to the machine the VNC viewer (on Windows) reports under connection info Pixel Format: depth 24 (32bpp) little-endian rgb888, Server Default: depth 24 (32bpp) little-endian bgr888.

Here is a backtrace.

#0  _cairo_error (status=CAIRO_STATUS_INVALID_FORMAT)
    at /src/gfx/cairo/cairo/src/cairo.c:86
#1  0xb4e6ed5c in *INT__moz_cairo_image_surface_create_for_data (
    data=0xaf2f7000 "ÿÿÿ", format=4294967295, width=47, height=23, stride=188)
    at /src/gfx/cairo/cairo/src/cairo-image-surface.c:493
#2  0xb4e869b1 in _wrap_image (src=0xaf880ca0, image=0xaf056570, 
    image_extra=0x0, out=0xbf91d7e8)
    at /src/gfx/cairo/cairo/src/cairo-surface.c:1297
#3  0xb4e86dbe in _cairo_surface_clone_similar (surface=0xaf054d00, 
    src=0xaf880ca0, src_x=0, src_y=0, width=47, height=23, 
    clone_offset_x=0xbf91d950, clone_offset_y=0xbf91d954, clone_out=0xbf91dc5c)
    at /src/gfx/cairo/cairo/src/cairo-surface.c:1399
#4  0xb4e79919 in _cairo_pattern_acquire_surface_for_surface (
    pattern=<value optimized out>, dst=0xaf054d00, x=0, y=0, width=47, 
    height=23, out=0xbf91dc5c, attr=0xbf91dc14)
    at /src/gfx/cairo/cairo/src/cairo-pattern.c:1968
#5  0xb4e7b114 in _cairo_pattern_acquire_surface (pattern=0xaf093c40, 
    dst=0xaf054d00, x=0, y=0, width=47, height=23, surface_out=0xbf91dc5c, 
    attributes=0xbf91dc14)
    at /src/gfx/cairo/cairo/src/cairo-pattern.c:2133
#6  0xb4e7b8d9 in _cairo_pattern_acquire_surfaces (src=0xaf093c40, mask=0x0, 
    dst=0xaf054d00, src_x=0, src_y=0, mask_x=0, mask_y=0, width=47, height=23, 
    src_out=0xbf91dc5c, mask_out=0xbf91dc58, src_attributes=0xbf91dc14, 
    mask_attributes=0xbf91dbd0)
    at /src/gfx/cairo/cairo/src/cairo-pattern.c:2208
#7  0xb4e6e575 in _cairo_image_surface_composite (op=CAIRO_OPERATOR_SOURCE, 
    src_pattern=0xaf093c40, mask_pattern=0x0, abstract_dst=0xaf054d00, 
    src_x=0, src_y=0, mask_x=0, mask_y=0, dst_x=0, dst_y=0, width=47, 
    height=23)
    at /src/gfx/cairo/cairo/src/cairo-image-surface.c:963
#8  0xb4e858f6 in _cairo_surface_composite (op=CAIRO_OPERATOR_SOURCE, 
    src=0xaf093c40, mask=0x0, dst=0xaf054d00, src_x=0, src_y=0, mask_x=0, 
    mask_y=0, dst_x=0, dst_y=0, width=47, height=23)
    at /src/gfx/cairo/cairo/src/cairo-surface.c:1557
#9  0xb4e88625 in _composite_trap_region (clip=0x0, src=0xaf093c40, 
    op=CAIRO_OPERATOR_SOURCE, dst=0xaf054d00, trap_region=0xbf91de70, 
    extents=0xbf91dea4)
    at /src/gfx/cairo/cairo/src/cairo-surface-fallback.c:449
#10 0xb4e88945 in _clip_and_composite_trapezoids (src=0xaf093c40, 
    op=CAIRO_OPERATOR_SOURCE, dst=0xaf054d00, traps=0xbf91defc, clip=0x0, 
    antialias=CAIRO_ANTIALIAS_NONE)
    at /src/gfx/cairo/cairo/src/cairo-surface-fallback.c:643
#11 0xb4e89158 in _cairo_surface_fallback_paint (surface=0xaf054d00, 
    op=CAIRO_OPERATOR_SOURCE, source=0xaf093c40)
    at /src/gfx/cairo/cairo/src/cairo-surface-fallback.c:752
#12 0xb4e84f0e in _cairo_surface_paint (surface=0xaf054d00, 
    op=CAIRO_OPERATOR_SOURCE, source=0xaf093c40, extents=0x0)
    at /src/gfx/cairo/cairo/src/cairo-surface.c:1756
#13 0xb4e6bd60 in _cairo_gstate_paint (gstate=0xaf20a020)
    at /src/gfx/cairo/cairo/src/cairo-gstate.c:898
#14 0xb4e64f93 in *INT__moz_cairo_paint (cr=0xaf20a000)
    at /src/gfx/cairo/cairo/src/cairo.c:1983
#15 0xb4e3681a in _copy_xlib_surface_to_image (temp_xlib_surface=0xaf880ca0, 
    format=CAIRO_FORMAT_ARGB32, width=47, height=23, data_out=0xbf91e298)
    at /src/gfx/thebes/src/cairo-xlib-utils.c:412
#16 0xb4e368fc in cairo_draw_with_gdk (cr=0xaf03d800, 
    callback=0xb4e5f3a1 <NativeRendering>, closure=0xbf91e33c, width=47, 
    height=23, is_opaque=CAIRO_GDK_DRAWING_TRANSPARENT, 
    capabilities=CAIRO_GDK_DRAWING_SUPPORTS_OFFSET, result=0x0)
    at /src/gfx/thebes/src/cairo-xlib-utils.c:561
#17 0xb4e5f2b6 in gfxGdkNativeRenderer::Draw (this=0xbf91e3cc, ctx=0xaf670b10, 
    width=47, height=23, flags=1, output=0x0)
    at /src/gfx/thebes/src/gfxGdkNativeRenderer.cpp:110
#18 0xb4f67568 in nsNativeThemeGTK::DrawWidgetBackground (this=0xb2b56000, 
    aContext=0xaf682ec0, aFrame=0xb6cdb1f0, aWidgetType=1 '\001', 
    aRect=@0xbf91e72c, aDirtyRect=@0xbf91e64c)
    at /src/widget/src/gtk2/nsNativeThemeGTK.cpp:779
#19 0xb2f41d9c in nsCSSRendering::PaintBackgroundWithSC (
    aPresContext=0xaf66dc00, aRenderingContext=@0xaf682ec0, 
    aForFrame=0xb6cdb1f0, aDirtyRect=@0xbf91e874, aBorderArea=@0xbf91e72c, 
    aColor=@0xb6ceb554, aBorder=@0xb6cdb23c, aUsePrintSettings=0, 
    aBGClipRect=0x0)
    at /src/layout/base/nsCSSRendering.cpp:1511
#20 0xb2f4236c in nsCSSRendering::PaintBackground (aPresContext=0xaf66dc00, 
    aRenderingContext=@0xaf682ec0, aForFrame=0xb6cdb1f0, 
    aDirtyRect=@0xbf91e874, aBorderArea=@0xbf91e72c, aUsePrintSettings=0, 
    aBGClipRect=0x0)
    at /src/layout/base/nsCSSRendering.cpp:1344
#21 0xb302d1f4 in nsButtonFrameRenderer::PaintBorderAndBackground (
    this=0xb6cdb22c, aPresContext=0xaf66dc00, aRenderingContext=@0xaf682ec0, 
    aDirtyRect=@0xbf91e874, aRect=@0xbf91e77c)
    at /src/layout/forms/nsButtonFrameRenderer.cpp:272
#22 0xb302d2a9 in nsDisplayButtonBorderBackground::Paint (this=0xaf209820, 
    aBuilder=0xbf91ea30, aCtx=0xaf682ec0, aDirtyRect=@0xbf91e874)
    at /src/layout/forms/nsButtonFrameRenderer.cpp:183
#23 0xb2f51877 in nsDisplayList::Paint (this=0xaf2098a0, aBuilder=0xbf91ea30, 
    aCtx=0xaf682ec0, aDirtyRect=@0xbf91e874)
    at /src/layout/base/nsDisplayList.cpp:312
#24 0xb2f519ad in nsDisplayTransform::Paint (this=0xaf209888, 
    aBuilder=0xbf91ea30, aCtx=0xaf682ec0, aDirtyRect=@0xbf91e90c)
    at /src/layout/base/nsDisplayList.cpp:1171
#25 0xb2f51877 in nsDisplayList::Paint (this=0xaf2098f0, aBuilder=0xbf91ea30, 
    aCtx=0xaf682ec0, aDirtyRect=@0xbf91e90c)
    at /src/layout/base/nsDisplayList.cpp:312
#26 0xb2f51a20 in nsDisplayClip::Paint (this=0xaf2098e4, aBuilder=0xbf91ea30, 
    aCtx=0xaf682ec0, aDirtyRect=@0xbf91ed1c)
    at /src/layout/base/nsDisplayList.cpp:954
#27 0xb2f51877 in nsDisplayList::Paint (this=0xbf91ea24, aBuilder=0xbf91ea30, 
    aCtx=0xaf682ec0, aDirtyRect=@0xbf91ed1c)
    at /src/layout/base/nsDisplayList.cpp:312
#28 0xb2f742cc in nsLayoutUtils::PaintFrame (aRenderingContext=0xaf682ec0, 
    aFrame=0xb6cc05b0, aDirtyRegion=@0xbf91ecfc, aBackground=4294967295)
    at /src/layout/base/nsLayoutUtils.cpp:1104
#29 0xb2f84479 in PresShell::Paint (this=0xaf66e800, aView=0xaf68ca00, 
    aRenderingContext=0xaf682ec0, aDirtyRegion=@0xbf91ecfc)
    at /src/layout/base/nsPresShell.cpp:5571
#30 0xb3345e63 in nsViewManager::RenderViews (this=0xaf68c2e0, 
    aView=0xb6cb0380, aRC=@0xaf682ec0, aRegion=@0xbf91edc0)
    at /src/view/src/nsViewManager.cpp:609
#31 0xb334689c in nsViewManager::Refresh (this=0xaf68c2e0, aView=0xb6cb0380, 
    aContext=0xaf682ec0, aRegion=0xaf682e40, aUpdateFlags=1)
    at /src/view/src/nsViewManager.cpp:511
#32 0xb3347311 in nsViewManager::DispatchEvent (this=0xaf68c2e0, 
    aEvent=0xbf91f04c, aStatus=0xbf91ef60)
    at /src/view/src/nsViewManager.cpp:1105
#33 0xb333fcb1 in HandleEvent (aEvent=0xbf91f04c)
    at /src/view/src/nsView.cpp:167
#34 0xb4f42ffa in nsWindow::DispatchEvent (this=0xaf8f6790, aEvent=0xbf91f04c, 
    aStatus=@0xbf91f160)
    at /src/widget/src/gtk2/nsWindow.cpp:580
#35 0xb4f40b29 in nsWindow::OnExposeEvent (this=0xaf8f6790, 
    aWidget=0xb37e2510, aEvent=0xbf91f5a4)
    at /src/widget/src/gtk2/nsWindow.cpp:2302
#36 0xb4f410e8 in expose_event_cb (widget=0xb37e2510, event=0xbf91f5a4)
    at /src/widget/src/gtk2/nsWindow.cpp:5261
#37 0xb79cb036 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#38 0xb759cc4b in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#39 0xb75b3095 in ?? () from /usr/lib/libgobject-2.0.so.0
#40 0xb75b462b in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#41 0xb75b4c26 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#42 0xb7ae033e in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#43 0xb79c4f73 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#44 0xb77459b5 in ?? () from /usr/lib/libgdk-x11-2.0.so.0
#45 0xb7745fcf in gdk_window_process_all_updates ()
   from /usr/lib/libgdk-x11-2.0.so.0
#46 0xb7745ffb in ?? () from /usr/lib/libgdk-x11-2.0.so.0
#47 0xb772946b in ?? () from /usr/lib/libgdk-x11-2.0.so.0
#48 0xb750c7e1 in ?? () from /usr/lib/libglib-2.0.so.0
#49 0xb750e718 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#50 0xb7511dc3 in ?? () from /usr/lib/libglib-2.0.so.0
#51 0xb7511f81 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#52 0xb4f490fc in nsAppShell::ProcessNextNativeEvent (this=0xb6a86c40, 
    mayWait=0) at /src/widget/src/gtk2/nsAppShell.cpp:144
#53 0xb4f68624 in nsBaseAppShell::DoProcessNextNativeEvent (this=0xb6a86c40, 
    mayWait=0)
    at /src/widget/src/xpwidgets/nsBaseAppShell.cpp:151
#54 0xb4f68a64 in nsBaseAppShell::OnProcessNextEvent (this=0xb6a86c40, 
    thr=0xb6b9b100, mayWait=1, recursionDepth=0)
    at /src/widget/src/xpwidgets/nsBaseAppShell.cpp:278
#55 0xb7d051f4 in nsThread::ProcessNextEvent (this=0xb6b9b100, mayWait=1, 
    result=0xbf91f850) at /src/xpcom/threads/nsThread.cpp:497
#56 0xb7cb0fa2 in NS_ProcessNextEvent_P (thread=0x0, mayWait=1)
    at nsThreadUtils.cpp:230
#57 0xb4f68b3c in nsBaseAppShell::Run (this=0xb6a86c40)
    at /src/widget/src/xpwidgets/nsBaseAppShell.cpp:170
#58 0xb4d98140 in nsAppStartup::Run (this=0xb6ab4d90)
    at /src/toolkit/components/startup/src/nsAppStartup.cpp:192
#59 0xb7eab6f5 in XRE_main (argc=1, argv=0xbf91fe14, aAppData=0xb6b06540)
    at /src/toolkit/xre/nsAppRunner.cpp:3340
#60 0x080498cc in main (argc=1, argv=0xbf91fe14)
    at /src/browser/app/nsBrowserApp.cpp:156

Comment 11

[Chris Wilson]

Attachment: Clone the image surface using the pixman_format

Here's the buglet: our attempt at wrapping the image surface within a clone used the cairo format and not the underlying pixman format.

(Though for upstream, I anticipated using the approach of adding the reference count to the win32 backing image instead of the generic _wrap_image().)

Comment 12

[Timothy Nikkel (:tnikkel)]

Excellent! That patch fixes this bug for me. Thanks.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should we be attempting to get this patch into the tree?

Comment 14

[Chris Lord [:cwiiis]]

The test-case works fine for me (on Linux/x64), I think this has since been fixed, possibly by our updating to a later version of Cairo?

Marking as WORKSFORME, please re-open if you think this is incorrect.