Bug 67505

I opened some random images and moved the mouse pointer around on them. I also tried different tools. Cannot reproduce with gimp from debian testing:

$ LC_ALL=C gimp --version --verbose
GNU Image Manipulation Program version 2.8.4
git-describe: GIMP_2_8_2-186-g8a9af4c

using GEGL version 0.2.0 (compiled against version 0.2.0)
using GLib version 2.36.3 (compiled against version 2.32.4)
using GdkPixbuf version 2.28.2 (compiled against version 2.26.1)
using GTK+ version 2.24.20 (compiled against version 2.24.10)
using Pango version 1.32.5 (compiled against version 1.30.0)
using Fontconfig version 2.10.2 (compiled against version 2.9.0)
using Cairo version 1.12.14 (compiled against version 1.12.2)

cairo is not from debian, but my local clone. I tried with both master and the 1.12.14 tag.

Does this only break with gimp 2.9 / git/master?

This only happens on git master, Chris Wilson did a patch that
enables exploiting latest fad (tm) cairo features, or something :)

Easily reproducable with gimp 96a090d9b12d20deec20248a841f7b36e103c17c.

No idea what to do about this. I say the bug is in gimp, Chris blames cairo-xcb. The problem goes something like this:

- gimp draws to a small part of an image surface
- gimp wants this part to be visible in the GUI and uses it as source surface in cairo_paint()
- cairo-xcb uploads the whole image surface to the X11 server (which is way more than is actually needed)
- Since this was an expensive operation, cairo-xcb attaches a so called snapshot to the image surface. This snapshot keeps alive the temporary pixmap that the image surface was uploaded to.
<time passes>
- gimp wants to redraw another part of the visible image and draws to another part of the above image surface
- cairo-xcb sees the snapshot attached to the image surface and optimizes away the new upload, because it still has the pixmap that the surface was uploaded to before. Thus, old data is used.

I blame gimp because IMHO the API docs imply that it should call cairo_surface_flush() before directly modifying an image surface and mark_dirty() afterwards. The flush() would get rid of the snapshot. Gimp currently only does these calls the first time that it directly modifies the surface.

Chris argues that gimp never touches the same area of the image surface twice and that it calls flush();mark_dirty() when all of the temporary image was used. So the bug is that cairo-xcb uploads more of the image surface than it really has to. Also, it needlessly uploads to a temporary pixmap when it could paint directly to the target surface. (And such an optimization would fix the visible results of this bug and thus hide it)


If I add a call to cairo_surface_mark_dirty(xfer); to gimp_display_shell_render() (after xfer was modified, gimp dies: gimp-2.9: cairo-surface.c:1585: cairo_surface_mark_dirty_rectangle: Assertion `! _cairo_surface_has_snapshots (surface)' failed
(This basically says: You should have called cairo_surface_flush(), so that the snapshot is detached)

If I remove the call to _cairo_surface_attach_snapshot() from _cairo_xcb_surface_picture(), the problem (obviously) goes away (and cairo-xcb re-uploads the whole image surface on each cairo_paint()).

I officially have no opinion here, the code in gimpdisplayxfer.c
obviously depends on what exactly cairo does with the uploaded
image, so its correctness is defined entirely in cairo land. If
you decide the bug is in GIMP, feel free to tell me what to change,
or better have Chris fix it because I don't really understand
the details here :)

First we'll fix xcb not to copy the pixels here, as that is exactly what we're trying to avoid in gimp. (Then I plan to go back to lalala as it should just work (or be made to work) using "zero copy" transfers in OS/X, Win32 and Linux.)

Created attachment 86655 [details] [review]
Use same fix in cairo-xcb as in cairo-xlib

First, why does the existing code for in-place uploads in cairo-xcb not work here? The xfer surface that gimp uses has format ARGB32 aka depth=32, but the displayshell (or whatever it is called) only has depth=24. This means that the upload cannot be done with core X11.

cairo-xlib uses SHM's CreatePixmap request to have the server allocate a pixmap for this shm segment and then uses RENDER's Composite to copy over the data.

The attached patch implements the same in this one place for cairo-xcb, but IMHO there is way too much copy&paste for the actual copy-the-data-from-pixmap. Also, there is an open-coded XSync() in there, because this needs a round-trip when cairo_surface_flush() is called on the xfer surface and I am too lazy to implement this right now (cairo-x11 does this round-trip).

@match: Since you know gimp's code better, do you really need an ARGB32 surface from gimp_display_xfer_get_surface()? Since nothing in gimp_display_shell_render() draws some kind of "background" behind the ARGB32 surface, I guess that its content should always be opaque and a RGB24 surface can be used instead.
(I didn't test if that change in gimp actually hides this problem nor have I decided if it would be better to paper over this problem in gimp or have something done in cairo-xcb.)

Yes the drawn image *always* has alpha, it's put on top of the
checkerboard pattern which indicates transparency.

commit f82ae573fe4748874e6dc7025016f7bf6bc8f7f3
Author: Uli Schlachter <psychon@znc.in>
Date:   Sat Jun 20 10:59:52 2015 +0200

    XCB: Don't attach uploaded surfaces as snapshots
    
    When you draw (part of) a surface to an XCB surface, the XCB backend will safe
    the uploaded part as a snapshot to the input surface. This allows to re-use this
    picture in case the same surface is later used again as a source.
    
    However, other backends do not do this and this has caused and/or highlighted
    numerous bugs. Just skipping the snapshot fixes or hides these bugs.
    
    Papers-over: https://bugs.freedesktop.org/show_bug.cgi?id=67505
    Signed-off-by: Uli Schlachter <psychon@znc.in>

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/319.