Bug 61876 - Regression since 1.11.4: Corrupt drawing on mingw64 build used with virt-viewer 0.5.5 binaries
Summary: Regression since 1.11.4: Corrupt drawing on mingw64 build used with virt-view...
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 1.12.14
Hardware: All All
: medium normal
Assignee: Chris Wilson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-05 22:16 UTC by Grant C.
Modified: 2013-04-02 07:39 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Screenshot demonstrating corrupt images (1.09 MB, image/png)
2013-03-05 22:16 UTC, Grant C.
Details
git bisect log (861 bytes, text/plain)
2013-03-20 03:13 UTC, Grant C.
Details
fix (2.00 KB, patch)
2013-04-01 22:47 UTC, Marc-Andre Lureau
Details | Splinter Review

Description Grant C. 2013-03-05 22:16:55 UTC
Created attachment 75988 [details]
Screenshot demonstrating corrupt images

When using libcairo-2.dll built using mingw64 coupled with virt-viewer 0.5.5 to connect to a Spice display server, image corruption occurs when trying to move any launched windows or when highlighting any desktop icons (see attached screenshot).

I've tested numerous releases and the earliest I could find this occurring is between the 1.11.2 (bug not present) and 1.11.4 (bug present) releases. Unfortunately, I was unable to do a git bisect between them due to hitting several compilation issues.

I've also tested a build of 1.12.4 as well, however the issue is still present in that release.

Problem initially spotted by a user on the spice-devel mailing list: http://lists.freedesktop.org/archives/spice-devel/2013-March/012496.html
Comment 1 Marc-Andre Lureau 2013-03-08 01:15:42 UTC
The windows client is usually compiled with Gtk+2 from fedora mingw (at least I can reproduce with mingw32-gtk2-2.24.15-1.fc18).

The SpiceGtk widget is a GtkDrawingArea that is gtk_widget_set_double_buffered(), whose expose event uses a gdk_cairo_create() context and drawn with spicex_draw_event(): http://cgit.freedesktop.org/spice/spice-gtk/tree/gtk/spice-widget-cairo.c

(it seems the rendering is ok when the the display is scaled)

Is there anything we can do to help?
Comment 2 Uli Schlachter 2013-03-09 22:38:42 UTC
(In reply to comment #0)
[...]
> Unfortunately, I was unable to do a git bisect between them due to hitting
> several compilation issues.
[...]

Do you happen to have the final range of possible-faulty commits around? Did you use "git bisect skip" to "work around" those compile issues?

(In reply to comment #1)
> The SpiceGtk widget is a GtkDrawingArea that is
> gtk_widget_set_double_buffered(), whose expose event uses a
> gdk_cairo_create() context and drawn with spicex_draw_event():
> http://cgit.freedesktop.org/spice/spice-gtk/tree/gtk/spice-widget-cairo.c

You have a reference leak in spicex_image_destroy(). You have to call cairo_surface_destroy() even when you call cairo_surface_finish(). The latter function doesn't drop any references.

Since this mentions DLLs and mingw64: The problem should be in cairo-windows, right? The image surface used as d->ximage contains the correct data?

Now that I have written this question: Do you call cairo_surface_mark_dirty() / cairo_surface_mark_dirty_rectangle() / cairo_surface_flush() where needed?
You could try inserting a call to cairo_surface_mark_dirty(d->ximage) at the beginning of spicex_draw_event() and see if it helps (but of course this is not a fix, the code which messes with the image data should mark it dirty instead)
Comment 3 Grant C. 2013-03-20 03:13:58 UTC
Created attachment 76798 [details]
git bisect log

I've attached a partial bisect log. After getting to this point, the individual revisions that built successfully caused a crash with remote-viewer, preventing me from being able to continue as at that point I'd be bisecting a different issue.

I can switch focus to finding the next revision that works without causing a crash, but that'd still leave us with a range of revisions that we can't easily build to track down which one is causing the regression..
Comment 4 Marc-Andre Lureau 2013-03-26 22:03:49 UTC
(In reply to comment #2)
> (In reply to comment #0)
> [...]
> > Unfortunately, I was unable to do a git bisect between them due to hitting
> > several compilation issues.
> [...]
> 
> Do you happen to have the final range of possible-faulty commits around? Did
> you use "git bisect skip" to "work around" those compile issues?

unfortunately, no. As Grant C. is saying, many commits do not build on win32 (>90%). I am afraid git bisect is not going to be of any help, there has been too much win32 broken change between 1.11.2 and 1.11.4

> (In reply to comment #1)
> You have a reference leak in spicex_image_destroy(). You have to call
> cairo_surface_destroy() even when you call cairo_surface_finish(). The
> latter function doesn't drop any references.

Ah, thanks for pointing out!

> Since this mentions DLLs and mingw64: The problem should be in
> cairo-windows, right? The image surface used as d->ximage contains the
> correct data?

Yes: hiding & showing back the widget or widget region is enough to fix the display (also the scaled version is correct, and using older version of cairo works)

> Now that I have written this question: Do you call
> cairo_surface_mark_dirty() / cairo_surface_mark_dirty_rectangle() /
> cairo_surface_flush() where needed?

No, we never did that, but should probably!

> You could try inserting a call to cairo_surface_mark_dirty(d->ximage) at the
> beginning of spicex_draw_event() and see if it helps (but of course this is
> not a fix, the code which messes with the image data should mark it dirty
> instead)

Unfortunately, it didn't help.

Thanks for your suggestions!
Comment 5 Marc-Andre Lureau 2013-04-01 22:47:00 UTC
Created attachment 77295 [details] [review]
fix

    win32: fix corrupted drawing
    
    Fix src bitmap coordinates, which origin is bottom-left. This is
    apparently a bug in StretchDIBits(), according to some comments on
    MSDN API documentation.
    
    The backend used to have this coordinate change in the past:
    
                if (!StretchDIBits (dst->dc,
                                    /* dst x,y,w,h */
                                    dst_r.x, dst_r.y + dst_r.height - 1,
                                    dst_r.width, - (int) dst_r.height,
                                    /* src x,y,w,h */
                                    src_r.x, src_extents.height - src_r.y + 1,
                                    src_r.width, - (int) src_r.height,
                                    src_image->data,
                                    &bi,
                                    DIB_RGB_COLORS,
                                    SRCCOPY))
Comment 6 Chris Wilson 2013-04-02 07:39:45 UTC
Oops, sorry about that. Thank you very much for the patch,

commit e66e9ac12e3e11af76f14e8de3cfee72d4299864
Author: Marc-André Lureau <marcandre.lureau@gmail.com>
Date:   Tue Apr 2 00:32:56 2013 +0200

    win32: fix corrupted drawing


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.