904981 - DrawTargetCairo incorrectly uses device offsets

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: device-offset

When we convert a thebes gfxASurface into an azure SourceSurface (via GetSourceSurfaceForSurface), we normally drop and ignore the device offset set on the surface.

For SourceSurfaceCairo however, we just wrap the cairo_surface_t, so we retain this offset.

This is currently causing failures when using MaskSurface (from nsDisplayBoxShadowOuter).

Our implementation of gfxContext::Mask takes the device offset into account for the draw parameters, and the cairo backend does this again by applying the actual device offset.

There are a bunch of other gfxContext function (like SetSource) that don't take the device offset into account though. Should they?

The simple fix here is to temporarily clear the device offset inside the two gfxContext::Mask functions (patch attached).

Are there situations for the other uses of gfxASurface->SourceSurface where we might have a device offset to take into account?

We'd need to fix these users in gfxContext too, as well as probably stripping the device offset within DrawTargetCairo.

Comment 1

[Bas Schouten (:bas.schouten)]

Comment on attachment 789972 [details] [diff] [review]
device-offset

Review of attachment 789972 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.

Hrm, I believe it might be preferred to do this inside the Cairo Moz2D backend, although I should think about this a little more on how to do it cleanly.

Note we do take deviceOffset into account for gfxPatterns. I believe I checked that for our usecases of SetSource the deviceOffset is never relevant, and so I decided to minimize the wrapper's overhead and not deal with it. It could be dealt with fairly easily if we wanted to through GeneralPattern::Pattern&().

Technically because we do use it in gfxPattern we need to check if there are other drawing calls where we need to deal with it though.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Clear device offsets within DrawTargetCairo

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 792586 [details] [diff] [review]
Clear device offsets within DrawTargetCairo

Review of attachment 792586 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +69,5 @@
>  
>    cairo_t* mCtx;
>  };
>  
> +

nit: Don't need more whitespace :)

@@ +193,5 @@
>   				                      ReleaseData);
>    return surf;
>  }
>  
> +class AutoClearDeviceOffset

Perhaps this should grab a references, otherwise it needs to be well documented, this could purely in theory be used on a surface that's created in the same scope, and that surface would be dereferenced and destroyed before this would go out of scope. I'm tempted to say document, because we shouldn't need to use this on surfaces we create temporarily internally.

Comment 4

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13bb9200d300

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13bb9200d300