Bug 61876

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?

(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)

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..

(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!

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))

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