Closed Bug 1539846 Opened 6 months ago Closed 4 months ago

Picture (with SVG filter) not displayed completely

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: jandem, Assigned: jrmuizel)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [wr-april])

Attachments

(3 files, 1 obsolete file)

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.

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

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

Flags: needinfo?(jdemooij)
Attached video Screen recording
Flags: needinfo?(jdemooij)

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

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.

(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.

Flags: needinfo?(jmuizelaar)

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

Flags: needinfo?(jmuizelaar)
Summary: Picture not displayed completely → Picture (with SVG filter) not displayed completely
Blocks: wr-67
Priority: -- → P2
Attached file Standalone test (obsolete) —

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

Attached file Standalone test case

This reduces the size and turns on the filter unconditionally

Attachment #9054422 - Attachment is obsolete: true

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

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

Has Regression Range: --- → yes
Regressed by: 1460491
Assignee: nobody → bugzmuiz

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

You should remove your name from your other account

Assignee: bugzmuiz → jmuizelaar
Whiteboard: [wr-april]

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.

Blocks: wr-68
No longer blocks: wr-67

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.
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a6f2d97508e
Ensure building rect changes cause invalidations. r=mstange
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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:
Attachment #9062580 - Flags: approval-mozilla-beta?
Blocks: wr-67
No longer blocks: wr-68

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

low risk, approved for our beta branch before RC

Attachment #9062580 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.