Bug 54657 - commit 0bfd2ac causes great rendering problem is some programs
commit 0bfd2ac causes great rendering problem is some programs
Product: cairo
Classification: Unclassified
Component: general
x86-64 (AMD64) Linux (All)
: medium major
Assigned To: Carl Worth
cairo-bugs mailing list
Depends on:
  Show dependency treegraph
Reported: 2012-09-08 01:58 UTC by yuyichao
Modified: 2012-09-17 15:40 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

xorg.log (31.00 KB, text/plain)
2012-09-08 11:07 UTC, yuyichao
patch for fix all the problem in this report. (456 bytes, patch)
2012-09-13 21:45 UTC, Weng Xuetian
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description yuyichao 2012-09-08 01:58:04 UTC
After upgrading to cairo-git on archlinux, all gtk2 popup window/menu/tooltips have this kind of effect[1] (Notice that the random stuff around the menu should actually be drop shadow). This happens when I am using QtCurve Theme and also causes another program that uses pango not able to display it's window.

After compiling different commit of cairo-git I track down the single commit that causes the problem is this one.

0bfd2ac... xlib: Implement SHM fallbacks and fast upload paths 

I haven't tried to revert this single commit on git master but any commit before this one doesn't have the problem.

Comment 1 yuyichao 2012-09-08 02:01:48 UTC
what it should looks like
Comment 2 Chris Wilson 2012-09-08 07:34:16 UTC
You graphic driver looks broken, please attach your Xorg.log.
Comment 3 yuyichao 2012-09-08 11:07:09 UTC
Created attachment 66837 [details]
Comment 4 Chris Wilson 2012-09-08 15:30:03 UTC
It's a missing cairo_surface_flush in gtk2-engines-qtcurve, and even worse they have a memory leak:

--- shadowhelper.c.orig 2012-09-08 16:29:10.000000000 +0100
+++ shadowhelper.c      2012-09-08 16:28:36.000000000 +0100
@@ -58,6 +58,7 @@
         cairo_rectangle(cr, 0, 0, shadowSize, shadowSize);
+       cairo_surface_destroy(dest);
         return pixmap;

Also note that cairo_rectangle(); cairo_fill(); is just cairo_paint();
Comment 5 Weng Xuetian 2012-09-12 22:17:56 UTC
I also experience problem with this commit.

All the thing I do is just use cairo-xlib-surface to draw on xlib created window.

After I add cairo_surface_flush to every cairo_destroy (Though I didn't use xlib to draa), case seems to be better. (I don't use it and it never have problem beforece).

But cairo_xlib_surface_set_surface seems never have effect that it only draw content with the very origin size. Window I draw on is set to override_directted.

The workaround for me is to destroy xlib-surface and recreate a new one.
Comment 6 Chris Wilson 2012-09-13 11:48:27 UTC
Please try commit e2c4bb9465e6261eb79f24af52d339df0b563b55
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Sep 13 12:40:49 2012 +0100

    xlib: Fix regression in cairo_xlib_surface_set_drawable()
    In commit 0bfd2acd35547fc2bd0de99cc67d153f0170697d
    Author: Chris Wilson <chris@chris-wilson.co.uk>
    Date:   Mon Aug 13 01:34:12 2012 +0100
        xlib: Implement SHM fallbacks and fast upload paths
    I made the mistake of inverting the logic for
    cairo_xlib_surface_set_drawable() causing it then to never update.
    Thanks to Uli Schlachter for spotting my error.
    References: https://bugs.freedesktop.org/show_bug.cgi?id=54657
    Reported-by: Weng Xuetian <wengxt@gmail.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

The issue the qtcurves engine ran afoul of was that it was mixing cairo and external access to its shadow Pixmaps, without flushing the cairo rendering. That behaviour has been documented since cairo-1.0, and with time we rely upon it for more optimisations. So long as you observe the requirement to flush/mark-dirty around non-cairo access to cairo surfaces' Drawable, it should work on any backend.
Comment 7 Weng Xuetian 2012-09-13 14:00:45 UTC
No, that didn't fix my problem.

Let me rephrase my problem.

My call sequence is:
XCreateWindow (with ARGB visual and override redirect)
create xlib surface.
draw sth, flush surface.

xlib_surface_set_size (Enlarge it).
draw sth, flush surface.

Now the second draw will only happens in the first size.

screen shot is something like this.

Comment 8 Chris Wilson 2012-09-13 14:34:27 UTC
Getting closer. In this sequence, the issue is that although we flush the modifications, we didn't discard the backing storage for the fallback and so it retained the original size in later use.

commit 6ee216000ae487492fceda0fb3fecb20bb9a41f6
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Sep 13 15:25:49 2012 +0100

    xlib: Explicitly discard the fallback shm pixmap upon user modification
    If the user changes the size of the underlying drawable, we must make
    sure that we discard the current ShmPixmap in order to create a new
    fallback pixmap of the correct size next time.
Comment 9 Weng Xuetian 2012-09-13 15:39:23 UTC
Get this with new commit.

cairo-xlib-surface-shm.c:837: _cairo_xlib_surface_get_shm: Assertion `surface->base.damage == ((void *)0)' failed.
Comment 10 Weng Xuetian 2012-09-13 15:46:01 UTC
Sometimes I also might get:

cairo-xlib-surface.c:1494: get_compositor: Assertion `s->shm != ((void *)0)' failed.
Comment 11 Chris Wilson 2012-09-13 15:51:21 UTC
I think this is the last line of code in that function that has not been shown to be buggy...

commit 69d97d97bea86e7f4223d857803fb7f0ec0d369f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Sep 13 16:45:46 2012 +0100

    xlib: Force the fallback flush before updating the external Drawable
    _cairo_surface_begin_modification() performs an internal flush, for
    which the xlib backend skips flushing the fallback surface as it will
    continue to use it for the subsequent operation. In the case where we
    are flushing prior to updating the Drawable, we need to perform an
    external flush which will trigger the posting of the damage from the
    fallback surface.
    Reported-by: Weng Xuetian <wengxt@gmail.com>
    References: https://bugs.freedesktop.org/show_bug.cgi?id=54657
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Comment 12 Weng Xuetian 2012-09-13 21:42:42 UTC
This still persists.

cairo-xlib-surface-shm.c:837: _cairo_xlib_surface_get_shm: Assertion
`surface->base.damage == ((void *)0)' failed.
Comment 13 Weng Xuetian 2012-09-13 21:45:01 UTC
Created attachment 67122 [details] [review]
patch for fix all the problem in this report.

Sorry but I don't have much knowledge about cairo. But IMHO when shm is released base.damage need also to be release, and in that case fallback should be clear.
Comment 14 Chris Wilson 2012-09-13 21:59:31 UTC
Yup, the patch is right. My expectation was that the flush would destroy the damage, but I had forgotten that it would not do so until the fallback was idle as well. (Migration avoidance).

I pushed the same patch before noticing you had written one, sorry, or else I would have credited you with the complete fix.

commit 5c59d989f9037f94c80ccd7929dc05f4a95be4df
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Sep 13 22:50:11 2012 +0100

    xlib: Destroy the fallback damage along with the fallback surface
    Whenever we discard the fallback surface, we need to destroy the
    associated damage tracking, so move this into the common discard
    This should fix the issue when trying to flush the fallback before
    the user modifies any foreign Drawables. The current code issued the
    flush and then explicitly discard the fallback, but unless it was idle
    at the time of the flush the associated damage would not have also been
    destroyed. Asserts followed.
    References: https://bugs.freedesktop.org/show_bug.cgi?id=54657
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

commit 59248fb2628e86ff62abfbf122b88c2a299ec393
Author: Weng Xuetian <wengxt@gmail.com>
Date:   Thu Sep 13 22:56:57 2012 +0100

    xlib: Reset fallback counter when discarding the fallback
    References: https://bugs.freedesktop.org/show_bug.cgi?id=54657