779029 - Mask region ignored when rendering with Direct2D

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

The region specified for a mask (x/y/width/height) is ignored when rendering with Direct2D.

See the attached test case. The mask region should limit the output to the green rectangle but instead is showing up to the red rectangle.

With Direct2D disabled it works fine. Also looks fine in ESR10.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

What kind of user effect are we talking about here? What is this a regression from?

Comment 2

[Brian Birtles (:birtles)]

(In reply to Lukas Blakk [:lsblakk] from comment #1)
> What kind of user effect are we talking about here? What is this a
> regression from?

The bounds for the mask area are completely ignored. In the example test case, the area that should be hidden will be displayed.

A bad result would be that important information that should be visible would be covered by the un-masked region making a site unuseable. A less severe result would simply be that graphics look horrible due to graphical elements showing where they should not.

The regression is possibly from enabling the Azure-Thebes wrapper by default (bug 715768).

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

It would be great to get a regression window on this to see if its been affecting users for a while or not.

Comment 4

[Joe Drew (not getting mail)]

This is almost certainly a bug introduced by turning on Azure content, so I don't know that a regression window would be helpful.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Ah, thanks for clarifying that.  At this point in the Firefox 15 cycle I don't see this being something to hold a release up, so not tracking for 15.

Comment 6

[Bas Schouten (:bas.schouten)]

This testcase is currently broken on all renderers and platforms. Presumably this has something to do with DLBI? I do have a patch that I believe will fix this (it fixes some other similar testcases)

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Try to respect EXTEND_NONE when masking

I raised bug 797507 for the problem with this test-case on all platforms. I believe this should fix the bug mentioned here, JWatt made several testcases for me which seem to work fine with this patch.

Comment 8

[Jonathan Watt [:jwatt]]

It would be good to have some helper API for this for when we start converting more code to use Azure directly. Maybe a RAII class where you can just do something like:

  AutoMaybeClipToSurface autoClipper(...args...);
  DT->Mask(...args...);

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 667612 [details] [diff] [review]
Try to respect EXTEND_NONE when masking

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

::: gfx/thebes/gfxContext.cpp
@@ +1403,5 @@
>      cairo_mask(mCairo, pattern->CairoPattern());
>    } else {
> +    bool needsClip = false;
> +    if (pattern->GetType() == gfxPattern::PATTERN_SURFACE &&
> +        pattern->Extend() == gfxPattern::EXTEND_NONE) {

This could use a high level description of how it's trying to solve the problem.

@@ +1436,5 @@
>        gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(mDT, surface);
>  
>      gfxPoint pt = surface->GetDeviceOffset();
> +
> +    mDT->PushClipRect(Rect(offset.x, offset.y, sourceSurf->GetSize().width, sourceSurf->GetSize().height));

Shouldn't the clip only happen with EXTEND_NONE?

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 667612 [details] [diff] [review]
Try to respect EXTEND_NONE when masking

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

::: gfx/thebes/gfxContext.cpp
@@ +1436,5 @@
>        gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(mDT, surface);
>  
>      gfxPoint pt = surface->GetDeviceOffset();
> +
> +    mDT->PushClipRect(Rect(offset.x, offset.y, sourceSurf->GetSize().width, sourceSurf->GetSize().height));

Didn't you say cairo_mask_surfaces uses the default pattern? So that's always EXTEND_NONE, right?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> 
> Didn't you say cairo_mask_surfaces uses the default pattern? So that's
> always EXTEND_NONE, right?

I believe so.

Comment 12

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/224fe2a12a04

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest/crashtest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e247f9d207

https://tbpl.mozilla.org/php/getParsedLog.php?id=17087035&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17087183&tree=Mozilla-Inbound

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Try to respect EXTEND_NONE when masking v2

Fixed a couple of small issues, carrying r+.

Comment 15

[Bas Schouten (:bas.schouten)]

Comment on attachment 684510 [details] [diff] [review]
Try to respect EXTEND_NONE when masking v2

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

Requesting review because there were some small API changes.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 684510 [details] [diff] [review]
Try to respect EXTEND_NONE when masking v2

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

::: gfx/thebes/gfxContext.cpp
@@ +1407,5 @@
> +      // In this situation the mask will be fully transparent (i.e. nothing
> +      // will be drawn) outside of the bounds of the surface. We can support
> +      // that by clipping out drawing to that area.
> +      Rect surfaceSourceRect;
> +      if (!pattern->IsAzure() &&

When do we use non-azure patterns?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 684510 [details] [diff] [review]
Try to respect EXTEND_NONE when masking v2

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

Also, this should get a test.

::: gfx/thebes/gfxContext.cpp
@@ +1407,5 @@
> +      // In this situation the mask will be fully transparent (i.e. nothing
> +      // will be drawn) outside of the bounds of the surface. We can support
> +      // that by clipping out drawing to that area.
> +      Rect surfaceSourceRect;
> +      if (!pattern->IsAzure() &&

At minimum this should have a comment why.

Comment 18

[Jonathan Watt [:jwatt]]

I created a reftest for this as requested by Bas and pushed it (disabled for now):

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb200c943cc

Bas, you just need to uncomment the line in the reftest.list file in your push. Can you add the comment requested in comment 17 and push?

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5bb200c943cc

Comment 20

[Bas Schouten (:bas.schouten)]

Attachment: Try to respect EXTEND_NONE when masking v3

Added the requested comment and enabling the test jwatt made.

Comment 21

[Jonathan Watt [:jwatt]]

Any thoughts on this, Jeff?

Comment 22

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c8751945bb

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7c8751945bb