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

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: szdy12, Assigned: bas.schouten)

Tracking

({regression, testcase})

37 Branch
mozilla40
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 wontfix, firefox38+ fixed, firefox39+ fixed, firefox38.0.5 fixed, firefox40+ fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

4 years ago
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

4 years ago
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
Blocks: 1082827
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Flags: needinfo?(bas)
Keywords: regression, testcase
Product: Firefox → Core
Version: 40 Branch → 37 Branch

Comment 2

4 years ago
gfx.direct2d.use1_1=false fixes the issue.
Assignee

Updated

4 years ago
Assignee: nobody → bas
Flags: needinfo?(bas)
Bas, can you create a test case for this?
Flags: needinfo?(bas)
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);
Attachment #8589667 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 7

4 years ago
Posted patch Add a reftestSplinter Review
Flags: needinfo?(bas)
Attachment #8590296 - Flags: review?(jmuizelaar)
Attachment #8590296 - Flags: review?(jmuizelaar) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8687854&repo=mozilla-inbound
Flags: needinfo?(bas)
Bas, will you have time to work on this during the 38 cycle? Thanks
Assignee

Updated

4 years ago
Duplicate of this bug: 1152824
Assignee

Comment 11

4 years ago
(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.
Flags: needinfo?(bas)
Assignee

Comment 12

4 years ago
This patch attempts to properly manage the clips. D2D doesn't like having layers pushed when switching destination surfaces.
Attachment #8594795 - Flags: review?(jmuizelaar)
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.
Attachment #8594795 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 14

4 years ago
Proper new patch this time.
Attachment #8594795 - Attachment is obsolete: true
Attachment #8596004 - Flags: review?(jmuizelaar)
Assignee

Updated

4 years ago
Attachment #8589667 - Attachment is obsolete: true
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
Attachment #8596004 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/dea4cb3a1bea
https://hg.mozilla.org/mozilla-central/rev/191862a23efe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Bas, Jeff, is it possible to have an uplift request to 38 & 39? Thanks!
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
Assignee

Comment 19

4 years ago
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
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
Attachment #8596004 - Flags: approval-mozilla-beta?
Attachment #8596004 - Flags: approval-mozilla-aurora?
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
Attachment #8596004 - Flags: approval-mozilla-release+
Attachment #8596004 - Flags: approval-mozilla-beta?
Attachment #8596004 - Flags: approval-mozilla-aurora?
Attachment #8596004 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.