Closed Bug 1412375 Opened 7 years ago Closed 6 years ago

Masks incorrectly apply to masked contents individually instead of to the "transparency group"

Categories

(Core :: Graphics: WebRender, defect, P1)

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox61 --- fixed

People

(Reporter: mstange, Assigned: Gankra)

References

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(4 files, 1 obsolete file)

testcase
557 bytes, text/html
Details
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.
59 bytes, text/x-review-board-request
mstange
: review+
kats
: review+
Details
Bug 1412375 - Add reftest for atomicity of masks.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1412375 - adjust test expectations for better masking
59 bytes, text/x-review-board-request
kats
: review+
Details
Attached file testcase — 
The CSS mask property creates a stacking context and a conceptual intermediate surface. The masking operation needs to be performed with that intermediate surface as its input, analogous to what you'd do with an opacity group.

However, with webrender, it seems there is no intermediate surface, and instead the mask gets applied to the contents individually. This is incorrect.
Whiteboard: [wr-mvp] [triage]
Blocks: stage-wr-trains
status-firefox57: --- → unaffected
status-firefox58: affected → unaffected
Priority: -- → P3
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][wr-reserve-candidate]
Whiteboard: [wr-mvp] [triage][wr-reserve-candidate] → [wr-reserve] [gfx-noted]
Assignee: nobody → a.beingessner
Blocks: 1432101
This is being actively discussed in the upstream tracking issue https://github.com/servo/webrender/issues/1957
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241524

Looks good! It'll be interesting to see how our tests react to this.

::: layout/painting/nsDisplayList.cpp:9665
(Diff revision 1)
> -    // otherwise any nested ScrollingLayersHelper instances that are created
> -    // will get confused about which clips are pushed.
> -    aBuilder.PushClip(clipId, GetClipChain());
> +    // Create a new stacking context to attach the mask to, ensuring effects
> +    // like opacity are applied to the aggregate, and not the individual
> +    // elements.

I'd replace "effects like opacity are" with "the mask is".
Attachment #8967081 - Flags: review?(mstange) → review+
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241526

::: layout/painting/nsDisplayList.cpp:9664
(Diff revision 1)
>                                                                              bounds);
>    if (mask) {
> +    auto layoutBounds = wr::ToRoundedLayoutRect(bounds);
>      wr::WrClipId clipId = aBuilder.DefineClip(Nothing(), Nothing(),
> -        wr::ToRoundedLayoutRect(bounds), nullptr, mask.ptr());
> -    // Don't record this clip push in aBuilder's internal clip stack, because
> +        layoutBounds, nullptr, mask.ptr());
> +    

nit: some trailing whitespace here
nits addressed.
tons of regressions, but also a bunch of new passes!

Applying the layoutRect blindly to the stacking context was wrong. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=166851105edfaa1df2e52e05beb47dc7c116aa9b
Note that the "new passes" are just reductions in fuzziness for tests that are already fuzzy-if.
Ok the new try looks a lot better. All the failures are just fuzziness increases (seems like we're producing lower quality AA on a bunch of circles?)

No new passes, which means I'll need to writeup a reftest based on markus' test.
Some further analysis: 

* there's actually unexpected passes in wpt (hooray!)
* markus seems to think the fuzziness changes might actually be a concern, possibly a sign of double-clipping. kats, what do you think?
Flags: needinfo?(bugmail)
Actually if we were double-clipping wouldn't we get double transparency? This would mean markus' test would still fail, as the you would see the green through the blue (which I don't locally).
Yeah if it were double-clipping I would expect mask-opacity-1a.html to fail, but it looks like that's doing better now (the wpt version is passing, and the non-wpt version is the same as before). Overall this patch looks like an improvement to me - the slight increase in fuzziness I think is basically negligible and the passes in wpt are good.

So I'd be ok with landing it with the appropriate test annotation changes.
Flags: needinfo?(bugmail)
Attachment #8967138 - Attachment is obsolete: true
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241606

Do you have a try push with this? In particular I want to ensure that we make the fuzzy ranges as tight as possible, and that this actually passes on all platforms. A try push with test-verify jobs should run just this test.
try looks good, expanded the fuzzy range to 1-2 (locally I got 1, try unanimously wants 2).
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241736
Attachment #8967081 - Flags: review+
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241740

::: layout/reftests/bugs/reftest.list:2069
(Diff revision 2)
>  fuzzy-if(Android,66,574) fuzzy-if(d2d,89,777) fuzzy-if(!Android&&!d2d,1,31219) == 1425243-2.html 1425243-2-ref.html
>  == 1432541.html 1432541-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,true) == 1446470.html 1035091-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,false) == 1446470-2.html 1035091-ref.html
>  test-pref(layout.css.prefixes.gradients,false) == 1451874.html 1451874-ref.html
> +fuzzy(1-2,17500) == 1412375.html 1412375-ref.html

Let's make it 1-2,17500-17500
Attachment #8967151 - Flags: review?(bugmail) → review+
Comment on attachment 8967153 [details]
Bug 1412375 - adjust test expectations for better masking

https://reviewboard.mozilla.org/r/235804/#review241742

This looks good, thanks. I added linux-qr reftest jobs to your try push just to make sure those are all green too (I expect that to be the case since the windows ones were, but I like to be safe with stuff like this).
Attachment #8967153 - Flags: review?(bugmail) → review+
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241768

::: layout/reftests/bugs/reftest.list:2069
(Diff revision 2)
>  fuzzy-if(Android,66,574) fuzzy-if(d2d,89,777) fuzzy-if(!Android&&!d2d,1,31219) == 1425243-2.html 1425243-2-ref.html
>  == 1432541.html 1432541-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,true) == 1446470.html 1035091-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,false) == 1446470-2.html 1035091-ref.html
>  test-pref(layout.css.prefixes.gradients,false) == 1451874.html 1451874-ref.html
> +fuzzy(1-2,17500) == 1412375.html 1412375-ref.html

Actually it looks like it's passing perfectly on linux-qr. So we should one of the following:

fuzzy(0-2,0-17500)
fuzzy-if(!(webrender&&gtkWidget),1-2,17500-17500)
Keywords: checkin-needed
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edadd89f89fd
Create a clipped stacking context for nsDisplayMasks. r=kats,mstange
https://hg.mozilla.org/integration/autoland/rev/68b719bed991
Add reftest for atomicity of masks. r=kats
https://hg.mozilla.org/integration/autoland/rev/f4ccd608d691
adjust test expectations for better masking r=kats
Keywords: checkin-needed
Depends on: 1454097
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: