1539846 - Picture (with SVG filter) not displayed completely

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

STR:

1. Load https://webkit.org/blog/8685/introducing-the-jetstream-2-benchmark-suite/
2. Wait for it to load
3. Scroll down. The bar graph shows up partly with WR enabled.

This is on OS X, MBP.

Comment 1

[Jan de Mooij [:jandem]]

I can take a screenshot or try to record a video if people are unable to repro..

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I don't see this on Mac. Can you record a video?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Screen recording

Comment 4

[Jan de Mooij [:jandem]]

If I switch to a different tab and back, everything looks fine.

Comment 5

[Jan de Mooij [:jandem]]

Hm I just noticed I don't get the black background in other browsers or a February 1st Nightly. That's also with WR turned off. Let me bisect that first.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)

Hm I just noticed I don't get the black background in other browsers or a February 1st Nightly. That's also with WR turned off. Let me bisect that first.

Ah that's bug 1494034. I can't reproduce with ui.systemUsesDarkTheme = 0.

@Jeff: maybe you can try with ui.systemUsesDarkTheme = 1 ? You have to add the pref manually.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Yes. The dark theme adds a filter to the image and we take a different (broken) path in WebRender.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Standalone test

Slightly reduced copy of the website, just in case they decide to change their CSS.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Standalone test case

This reduces the size and turns on the filter unconditionally

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I think this worked in early January and has regressed sometime between then and now.

Comment 11

[Alice0775 White]

Reproducible: on Windows10
STR: comment#0 with following userContent.css
AR: the image is truncated

img {
filter: url("#invertLightness") !important;
}

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=63988e7c891f8d664f428fbaf91d9b181e3557e1&tochange=0571c2da7c437eb965ebf0cb79a4d991e6d7b8c2

Regressed by:
0571c2da7c43 Matt Woodrow — Bug 1460491 - Part 2: Only recompute visibility for items if they are newly added to this layer, or intersect one that changed. r=jnicol
920d49a96e6e Matt Woodrow — Bug 1460491 - Part 1: Split nsDisplayItem::mVisibleRect into two members, one for each stated purpose. Gets rid of the save/restore since we no longer modify the building rect. r=jnicol

Comment 12

[Jeff Muizelaar [:jrmuizel]]

This is caused by us only drawing the visible rect of the filter. I have some idea how to fix it.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

You should remove your name from your other account

Comment 14

[Aria Beingessner [:Gankra]]

Jeff is on leave, and it's my understanding that the change he was working on for this is involved enough that it wouldn't make sense for anyone else to pick it up and try to fix this for 67. Bumping to 68.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1539846. Ensure building rect changes cause invalidations.

Typically this would be handled by the visible region of the layer
changing. However, since we build the container layer for the filter
item directly the visible region doesn't get set or checked. As a
shortcut to using more of FLB we just ensure the building rect hasn't
changed.

The situations under which this bugs shows up are somewhat rare:

• The filtered item needs to be in transform so that it's bounds
  are not changed by scrolling.
• The filtered item needs to contain items that change their drawing
  depending on the building rect. In this case an image with downscale
  on decode.
• The filter needs to be unsupported by WebRender.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a6f2d97508e
Ensure building rect changes cause invalidations. r=mstange

Comment 17

[Alexandru Michis [:malexandru]]

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

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9062580 [details]
Bug 1539846. Ensure building rect changes cause invalidations.

Beta/Release Uplift Approval Request

• User impact if declined: Sometimes we don't fully paint content that has an SVG filter applied. e.g. https://webkit.org/blog/8685/introducing-the-jetstream-2-benchmark-suite/
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This only has an impact when WebRender is enabled. It doesn't touch the non-WebRender code path. We're rolling out WebRender gradually and if there's any fallout we can easily stop that rollout.

In the worst case this should only cause additional invalidations and it seems like those situations are quite rare so I'm not very worried about any performance decrease.

• String changes made/needed:

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9062580 [details]
Bug 1539846. Ensure building rect changes cause invalidations.

low risk, approved for our beta branch before RC

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f293cecba8fd