Bug 100793

Summary:	Issue with multiple-monitor setup with the desktop DC.
Product:	cairo	Reporter:	Eric Hoffman <ehoffman>
Component:	win32 backend	Assignee:	cairo-bugs mailing list <cairo-bugs>
Status:	RESOLVED FIXED	QA Contact:	cairo-bugs mailing list <cairo-bugs>
Severity:	normal
Priority:	medium
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Multi-monitor issue with negative extent patch Multi-monitor issue with negative extent patch (v2) Multi-monitor issue with negative extent patch (v2)

Description Eric Hoffman 2017-04-26 04:22:25 UTC

Created attachment 131034 [details] [review]
Multi-monitor issue with negative extent patch

Hi

I was actually initially investigating an issue with Gimp, but the root of the issue really is with cairo.

Under Win32, when you have multiple-monitor setup, Windows create a 'virtual desktop', which is a rectangle surface that extend all monitors.

The primary monitor always start with the top-left corner at coordinate (0,0).  If you have any other monitors which extends to the left, or above the primary monitor, the virtual desktop top-left corner will actually have coordinates which are negative.

This create an issue in cairo when you use the desktop DC, i.e. when you use a DC from the function GetDC(NULL).  The same think occur with other third party libraries like GTK, when you access the cairo surface from the root window.

In GIMP, I did apply a patch for the color picker, by using Windows native code for the affected function, but past that, I have investigated cairo and did a patch for this issue.

Basically, when the _cairo_win32_save_initial_clip() function is called with a DC, it get and save the extent.  When (and only when) the provided DC is the desktop DC, it is possible that the extent starts with a negative x,y.  In this case, and since the desktop DC will not be clipped, we can check if the extents x,y are negative.  In this case, make the extents start at (0,0), and record the offset.

Then, when performing image map/unmap, the BitBlt function has to take this offset into account.  For example, the fallback DC, even though it's created with CreateCompatibleDC, will have it's extents x,y at (0,0).  For a one to one copy between the original DC, and the fallback DC, the translation has to be taken into account.

That is what this patch does.  So basically, under Win32, the desktop DC will always start at (0,0).  This is in like with other libraries, like GTK, which return screen coordinates starting at (0,0).

That is also probably the root cause of bug 96482 (Cairo 1.15.2: Crash in cairo_fill if target surface is created with cairo_win32_surface_create), since in that bug, when the author apply a positive viewport transform, the DC most probably return a negative offset.

That may be the the root cause of bug 69617 too, although I'm not sure... (GIMP's color picker of color dialog not work, returns black).

Regards,
Eric Hoffman

Comment 1 Eric Hoffman 2017-04-26 04:32:22 UTC

For more info on the issue, I have written a very detailed explanation of the crash on the GIMP Bugzilla
https://bugzilla.gnome.org/show_bug.cgi?id=740634#c56

Regards,
Eric Hoffman

Comment 2 Eric Hoffman 2017-04-28 04:15:01 UTC

Created attachment 131112 [details] [review]
Multi-monitor issue with negative extent patch (v2)

Fixed the last submitted patch, for the BitBlt done in _cairo_win32_display_surface_flush(), when failing to get the damaged zones.

Comment 3 Eric Hoffman 2017-04-28 04:18:00 UTC

Created attachment 131113 [details] [review]
Multi-monitor issue with negative extent patch (v2)

Sorry, this should be the right one.  Last one had another patch applied.

Comment 4 Uli Schlachter 2017-04-29 13:05:45 UTC

Count me as impressed. Does anyone with non-zero cairo-win32 knowledge want to take a look at this? If not, I'll just submit to the long "wall of text" and commit this patch (eventually...). (No, I have no clue about windows)

Comment 5 Bryce Harrington 2018-02-06 23:16:40 UTC

Ditto what Uli said, but since it's been almost a year I went ahead with landing the patch.  If it causes any issues, I imagine they'll be fairly obvious and easy to pinpoint to this change, and hopefully with it landed it'll finally get some testing.

I took a look at 96482 and 69617 but it's not clear to me that they are indeed fixed by this, as their conditions sound subtly different.  But I'll post a request to test to them.

Thanks, landed:

To ssh://git.freedesktop.org/git/cairo
   a8571a3..62b724a  master -> master

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.