GNOME Bugzilla – Bug 650426
Missing call to cairo_surface_flush
Last modified: 2015-11-29 13:17:03 UTC
Created attachment 187985 [details] [review] proposed patch Today I compiled cairo-1.10.2 with $ ./configure --prefix=$HOME/tmp --enable-debug --enable-xlib-xcb --enable-xcb --enable-tee --enable-xml --enable-xcb-shm and gimp now crashes as soon as I open an image. It is a failed assertion in cairo gimp-2.7: cairo-surface.c:1173: cairo_surface_mark_dirty_rectangle: Assertion `! _cairo_surface_has_snapshots (surface)' Reading the comment in cairo-surface.c preceeding the assert and the documentation about cairo_surface_mark_dirty I think it is correct to call cairo_surface_flush in gimp_display_shell_render_info_init before accessing the raw data. On both 'shell->mask_surface' and 'shell->render_surface' cairo_surface_mark_dirty is possibly called twice without an intervening call to 'cairo_surface_flush' and evidently when a cairo backend saves a snapshot that assertion fails. If there is no objection I will push the attached patch.
Is the order in the comment wrong? (from drawing with cairo to drawing directly). Looks like it's going from directly to with cairo.
The following call to 'cairo_image_surface_get_data (dest)' shows that info->dest will be used later directly, not through cairo.
Oops, sorry :).
Comment on attachment 187985 [details] [review] proposed patch Looks absolutely right, please push.
Pushed, hopefully correctly. author Massimo Valentini <mvalentini@src.gnome.org> 2011-05-18 06:41:10 (GMT) committer Massimo Valentini <mvalentini@src.gnome.org> 2011-05-18 06:41:10 (GMT) commit 3efa2062c517e3365efc1f1f7a22a0016a85f35c (patch) (side-by-side diff) tree 3b2c82ed35e6f97a37ef6b96d92087609bd28dd1 parent 3e51484b188d5a9a42c730bd7162b270a81f064f (diff) Bug 650426 - Missing call to cairo_surface_flushHEADmaster app: before drawing directly on the surface. Diffstat (more/less context) (ignore whitespace changes) -rw-r--r-- app/display/gimpdisplayshell-render.c 5
Created attachment 188619 [details] [review] Proposed patch I've experienced another crash for the assertion in cairo_surface_mark_dirty, so I grepped the sources for cairo_surface_get_data. Here are the results: cairo_surface_get_data is used in three ways: 1. Drawing directly to a surface that is successively used in cairo and is possibly reused. cairo_surface_flush is mandatory before drawing directly app/display/gimpcanvastransformpreview.c|938| missing cairo_surface_flush app/display/gimpcanvastransformpreview.c|1176| missing app/display/gimpdisplayshell-render.c|374| present app/widgets/gimpviewrenderer.c|956| present app/widgets/gimpviewrenderergradient.c|193| present app/widgets/gimpviewrendererpalette.c|149| present modules/display-filter-color-blind.c|366| called in gimp_color_display_convert_surface modules/display-filter-gamma.c|189| called in gimp_color_display_convert_surface modules/display-filter-high-contrast.c|189| called in gimp_color_display_convert_surface modules/display-filter-lcms.c|263| called in gimp_color_display_convert_surface modules/display-filter-proof.c|245| called in gimp_color_display_convert_surface 2. Drawing directly to a surface just after its creation (and not reused). The documentation of cairo_surface_mark_dirty suggests that it is wise to call cairo_surface_flush even in this case, but now, when missing, there is no crash. libgimpwidgets/gimpcairo-utils.c|149| present app/widgets/gimpcolorbar.c|190| missing libgimp/gimpgradientselectbutton.c|520| missing modules/color-selector-water.c|229| missing plug-ins/common/file-pdf-save.c|1146| missing plug-ins/print/print-draw-page.c|203| missing plug-ins/print/print-preview.c|829| missing 3. Only reading the content of the surface. I'd say cairo_surface_flush is harmless. app/text/gimptextlayer.c|693| present plug-ins/common/file-pdf-load.c|737| present plug-ins/common/file-pdf-load.c|744| present libgimp/gimplayer.c|278| missing libgimpwidgets/gimppickbutton.c|383| missing The patch attached here is necessary. Comments for the use cases 2 and 3 are appreciated.
what about two wrappers around cairo_surface_get_data? const guchar* gimp_cairo_surface_get_data_read_only (cairo_surface_t *s) { return cairo_surface_get_data (s); } and guchar* gimp_cairo_surface_get_data_read_write (cairo_surface_t *s) { cairo_surface_flush (s); return cairo_surface_get_data (s); } I find cairo_surface_flush misleading.
We have a pretty bad history of "useful" utility functions. They appear really nice when we introduce them, but them something, somewhere changes and they end up hangling around in libgimp unused and then only clutter the API for as long as we make compatibility promises. I would prefer to simply flush the surfaces before getting their data pointers. Feel free to simply push the patches as you see fit.
commit 13716023837e5cdaa97ae979828d5938eb153ddb Author: Massimo Valentini <mvalentini@src.gnome.org> Date: Thu May 26 19:54:01 2011 +0200 app: call cairo_surface_flush before drawing directly