1151821 - globalCompositeOperation ignored in a clipped context 2d (regression since Firefox 37)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Szabo]

Attachment: globalCompositeOperationVSclip.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150406145430

Steps to reproduce:

Open the attached html file in Firefox.
There will be several canvas elements showing results of image rendering using different globalCompositeOperation settings. The top part of each image displays results without clipping, the bottom part displays results using clipping.
The clipping rectangle is set to the whole image area, so the top and bottom part of each image should be the same.


Actual results:

Starting from Firefox 37 most globalCompositeOperation settings will be ignored in a clipped context:
- "source-out", "destination-over", "destination-atop", "lighter" will render as "source-over"
- "source-atop", "source-in", "destination-in", "destination-out" won't render anything (destination stays intact)
As a result, the top and bottom part of those rendered images will differ.
(strangely, "difference" works as expected)


Expected results:

Top and bottom part of each image should be the same.

Comment 1

[Loic]

D2D 1.1 issue with canvas.

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=688f821edcd4&tochange=ab137ddd3746

Michael Wu — Bug 1082827: Allow D2D 1.1 to be selected for the canvas backend. r=bas

Comment 2

[Loic]

gfx.direct2d.use1_1=false fixes the issue.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Bas, can you create a test case for this?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8589667 [details] [diff] [review]
Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +949,5 @@
>    mDC->SetTransform(D2D1::IdentityMatrix());
>    mTransformDirty = true;
>  
>    if (patternSupported) {
>      if (D2DSupportsCompositeMode(aOp)) {

Can you add a high level comment here about what's going on. Even " Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask" would be good.

@@ +983,5 @@
> +        factory()->CreateRectangleGeometry(D2D1::RectF(0, 0, mSize.width, mSize.height), byRef(rectGeom));
> +        factory()->CreatePathGeometry(byRef(inverseGeom));
> +        RefPtr<ID2D1GeometrySink> sink;
> +        inverseGeom->Open(byRef(sink));
> +        rectGeom->CombineWithGeometry(geom, D2D1_COMBINE_MODE_EXCLUDE, D2D1::IdentityMatrix(), sink);

This code could be moved into a function like GetInverseClippedGeometry(size);

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Add a reftest

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8687854&repo=mozilla-inbound

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Bas, will you have time to work on this during the 38 cycle? Thanks

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Bas, will you have time to work on this during the 38 cycle? Thanks

Absolutely, I'm working on this right now, I was on a plane on friday sadly and unable to work on this then.

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v2

This patch attempts to properly manage the clips. D2D doesn't like having layers pushed when switching destination surfaces.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8594795 [details] [diff] [review]
Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v2

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

Is this actually the updated patch? I didn't notice any difference other than factoring out the method.

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v3

Proper new patch this time.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8596004 [details] [diff] [review]
Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v3

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +932,5 @@
>    mDC->SetTarget(mTempBitmap);
>    mDC->Clear(D2D1::ColorF(0, 0));
> +
> +  PushClipsToDC(mDC);
> +  mClipsArePushed = true;

How about moving mClipsArePushed into PushClipsToDC(mDC) and adding an assert(!mClipsArePushed) to that function as well

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dea4cb3a1bea
https://hg.mozilla.org/integration/mozilla-inbound/rev/191862a23efe

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dea4cb3a1bea
https://hg.mozilla.org/mozilla-central/rev/191862a23efe

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Bas, Jeff, is it possible to have an uplift request to 38 & 39? Thanks!

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 8596004 [details] [diff] [review]
Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v3

Approval Request Comment
[Feature/regressing bug #]: 1082827
[User impact if declined]: Incorrect rendering when using some globalCompositeOperators with a clip in Canvas
[Describe test coverage new/current, TreeHerder]: Several days of nightly testing
[Risks and why]: Low, should only affect complex operators
[String/UUID change made/needed]: None

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8596004 [details] [diff] [review]
Make sure all clips are popped and restore clipped out area if the operation is not bound by the mask v3

[Triage Comment]

[Triage Comment]
Sure, should be in 38 beta 8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3ba08d618d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6b8350569d9

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/987c18b686eb
https://hg.mozilla.org/releases/mozilla-release/rev/1bbb50c6a494

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/987c18b686eb
https://hg.mozilla.org/releases/mozilla-beta/rev/1bbb50c6a494