Closed Bug 2013682 Opened 7 months ago Closed 6 months ago

Broken subpixel AA and picture caching on linux, due to border-radius on the <body> element to support rounded corners.

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Version:
Firefox 149
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
150 Branch
Tracking Flags:
Tracking Status
firefox-esr140 --- unaffected
firefox147 --- unaffected
firefox148 --- unaffected
firefox149 --- unaffected
firefox150 + fixed

People

(Reporter: tgnff242, Assigned: gw)

References

(Regression)

Details

(Keywords: nightly-community, perf-alert, regression)

Attachments

(4 files, 1 obsolete file)

0001-WIP-Global-clip-prototype-for-picture-cache-slices.patch
14.42 KB, patch
Details | Diff | Splinter Review
Test-case describing the approach
868 bytes, text/html
Details
combine.rs
3.50 KB, text/rust
Details
Bug 2013682 - Allow stacking contexts with groups clips to be promoted to compositor clips
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:149.0) Gecko/20100101 Firefox/149.0

Steps to reproduce:

You can observe this here on the text of any bugzilla ticket page, even when gfx.webrender.quality.force-subpixel-aa-where-possible:true.

Actual results:

No subpixel AA.

Expected results:

Introduced by Bug 2005854.

mozregression result:
https://hg-edge.mozilla.org/integration/autoland/pushloghtml?fromchange=c652bb4f7aa58ff9a36d67390a64f0495381c66d&tochange=8d54cc2c66e9d90b6f297c017c6294c3c6b30edd

Has STR: --- → yes
Keywords: nightly-community, regression
Regressed by: 2005854

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: WebRender' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Graphics: WebRender
Product: Firefox → Core

:emilio, since you are the author of the regressor, bug 2005854, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)

Can you confirm that this works on a maximized window?

Depends on: 1979083
Flags: needinfo?(emilio) → needinfo?(tgnff242)

I think this broke because my patch moved the clip to the root, so it's hitting the same issue as bug 1979083. Glenn, how fixable is this?

If we can't fix this soon-ish, I need to ponder other alternatives to make bug 1952602 work, but they'd be pretty annoying...

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mozilla)

I guess another thing we could do if possible would be to add a global clip to WebRender somehow? This clip we want to apply to the toplevel window...

Summary: Broken subpixel AA → Broken subpixel AA on linux, due to border-radius on the <body> element to support rounded corners.
Summary: Broken subpixel AA on linux, due to border-radius on the <body> element to support rounded corners. → Broken subpixel AA and picture caching on linux, due to border-radius on the <body> element to support rounded corners.

[Tracking Requested - why for this release]: Power / scrolling performance / visual regression on Linux.

tracking-firefox149: --- → ?

Indeed, it works as expected in maximised windows.

Flags: needinfo?(tgnff242)

Set release status flags based on info from the regressing bug 2005854

status-firefox147: --- → unaffected
status-firefox148: --- → unaffected
status-firefox149: --- → affected
status-firefox-esr140: --- → unaffected

I don't think it's going to be easy to support this in WR in the near future (just due to resourcing). The global clip idea is interesting though - that sounds like it might be a good workaround without too much engineering effort. Did you have an idea of how / where this would be specified?

Flags: needinfo?(mozilla)
Severity: -- → S2

We can access it pretty easily from any thread in the main process, so wherever would be convenient for WR to receive it. Should be easy to get via the renderer interface around here, much like we get the window visibility here or so?

Would that work? If you write the WR-side code with a, let's say, 10px hard-coded radius, I can write the code to plumb the right value wherever that ends up being, does that sound like a plan?

Feel free to use a single radius, that way we can also fix the rounded bottom corners while at it (but otherwise a four-corner radius would also work).

Flags: needinfo?(mozilla)

If possible, I think it'd be better to provide it as part of the display list (probably just the root display list from the main process?) since that means the information would be available during frame building which is where we'd want it (rather than later at compositing time on the render thread). Would that still be easy enough from Gecko perspective?

Flags: needinfo?(mozilla) → needinfo?(emilio)

Yeah, for sure, that should be trivial as well.

Flags: needinfo?(emilio)

An extremely hacky proof of concept done with claude, not in any way production ready.

Is this the kind of thing you're thinking though Emilio?

If I revert the change in this bug (so picture caching works), apply this patch, I can set an env var WR_GLOBAL_CLIP_RADIUS, which gets passed to both WR and also the theme look and feel.

Seems to sort of work? I can get radius clips on all corners of the main window in kde, while picture caching still seems to be working - I haven't tried with all sidebar configs etc.

If this seems useful we can look at making a proper patch for it.

Assignee: nobody → mozilla
Flags: needinfo?(emilio)

This builds on the previous patch and allows rounded corners to work
with picture caching etc on Linux.

Yeah, that seems to work alright. Comment 15 implements all the Gecko-side plumbing that's needed, fwiw. It ended up being rather straight-forward I think :)

Only thing that I've noticed is that, if you enable the rounded bottom corners, then the web content clip goes wrong. I think that's because the rounded_rect_contains_rounded_rect check fails (since the bottom right corner of the is clipped by the window clip, but the top left corner is clipped by the content clip).

My understanding is that we'd want to combine the two clips together in that case, rather than the current behavior which just forgets about the inner clip. It seems doable with some tweaks to the rounded_rect_count code in the slice builder, perhaps?

However we don't enable the bottom corners by default so that could be a follow-up, if needed.

Flags: needinfo?(emilio) → needinfo?(mozilla)

I don't think we can just combine the two clips together, since the rect portion is (I assume) different between the clips. I was assuming that if there was a content clip it would be completely inside and usable rather than the global clip. Maybe I can support > 1 rounded rect at the global level.

Flags: needinfo?(mozilla)

Hmm, but supporting multiple rounded rect clips at that level is much larger / more invasive change (would likely require changes to all 6 compositor impls, I think). Would the global clip idea definitely require > 1 clip, or could we somehow get away without that?

Flags: needinfo?(emilio)

Well, I meant intersecting the two clips, effectively. The two rects but I think given how they are defined it should be possible to intersect them (in this case at least)?

So for the global clip we'd have these two clips:

  • Window clip: Rounded 10px (for example) rect
  • Rounded content area clip: Rounded 8px clip in the top left corner.

Wouldn't the intersected clip be effectively a single rect with an 8px clip in the top left corner and a 10px in the bottom right corner? I guess that is true as long as the rounded edges of the two clip rects don't intersect, but that holds for all our purposes I think. Or am I missing something?

In any case even if we couldn't deal with the rounded bottom corner case, it'd be an improvement for now, we don't ship that by default right now and it also breaks picture caching before my patch.

Flags: needinfo?(emilio) → needinfo?(mozilla)

(Just for context)

It seems like we should be able to write, effectively, a:

Maybe<RoundedRect> RoundedRectIntersection(RoundedRect a, RoundedRect b);

which gives you a result if both rounded rect clips can be "combined" into one.

Seems that'd be doable and should cover most cases we care about, after some discussion with Glenn.

Might want to split that off to a separate bug (bug 1979083 maybe?)

Flags: needinfo?(mozilla) → needinfo?(emilio)
Attached file combine.rs —

I couldn't get to it this week Glenn, any chance you have the cycles next week?

One thing that felt a bit clunky was that the check seems to be needed in two places, one in the slice_builder.rs, one in here, not sure what the best way to avoid it would be?

Flags: needinfo?(emilio) → needinfo?(mozilla)

Unlikely that I'll have a chance this week, I will let you know if I do find some time to work on it though.

Flags: needinfo?(mozilla)

We are getting close to the beta merge and this is marked as S2, will that be fixed before 149 enters beta? Thanks

Flags: needinfo?(mozilla)

(We can also back out the regressing bug from beta, if that buys us some more time)

If it's possible to back out the regressing bug that would be preferable, I don't think I will get to it this week. Emilio, any chance you would have time to look at it before then?

Flags: needinfo?(mozilla) → needinfo?(emilio)

Uploaded a backout patch in bug 2005854

Flags: needinfo?(emilio)
See Also: → 2018860

Emilio, is the following analysis plausible / correct?

  1. Bug 2005854 removed .browser-toolbox-background from #navigator-toolbox and moved those styles (including will-change: background-color) to <body>     
  2. #navigator-toolbox no longer creates a stacking context                                                                                              
  3. .browser-titlebar (child of #navigator-toolbox) has will-change: opacity + opacity: 0.6 when window is inactive                                        
  4. Without the parent SC from navigator-toolbox, .browser-titlebar's opacity SC ends up at the WR root level (sc_stack depth 0)                           
  5. This non-redundant SC pushes onto sc_stack (len becomes 1)                                                                                             
  6. All add_tile_cache_barrier_if_needed calls find sc_stack.is_empty() == false and skip                                                                  
  7. No barriers fire, so all content goes into a single tile cache slice

I don't really understand the frontend CSS to know if that's correct (steps 5-7 are clearly occurring, but not sure if steps 1-4 are correct).

Flags: needinfo?(emilio)

I think it's not. I mean, 1-3 sound right, but the content is not in the .browser-titlebar, so it's not the root cause. You can force #navigator-toolbox to be a stacking context with something like:

diff --git a/browser/themes/shared/tabbrowser/content-area.css b/browser/themes/shared/tabbrowser/content-area.css
index 8279ccf8ebe8..cc43333b40ae 100644
--- a/browser/themes/shared/tabbrowser/content-area.css
+++ b/browser/themes/shared/tabbrowser/content-area.css
@@ -26,6 +26,7 @@
 }

 #navigator-toolbox {
+  translate: 0; /* Force a stacking context in the toolbox */
   border-bottom: 0.01px solid var(--chrome-content-separator-color);

   /* stylelint-disable-next-line media-query-no-invalid */

But that doesn't change behavior. Even display: none-ing doesn't change behavior.

The relevant stacking contexts are the <body>, and #tabbrowser-tabbox (which also has translate: 0 by default, but note it can move during the sidebar animation and so on).

In fact, this fixes it (without the sidebar):

diff --git a/browser/themes/shared/sidebar.css b/browser/themes/shared/sidebar.css
index 785725c8c60a..538ec27336fc 100644
--- a/browser/themes/shared/sidebar.css
+++ b/browser/themes/shared/sidebar.css
@@ -241,7 +241,7 @@ sidebar-main,
   transform-origin: 0 0;
   /* will-change would cause us to be considered animated unconditionally, see
    * bug 1930674 */
-  translate: 0;
+  /* translate: 0; */
 }

 #sidebar-main {

Is it not possible to deal with that in the WR side? If so we can try to work around it but it's a bit footgunny.

Flags: needinfo?(emilio) → needinfo?(mozilla)

OK, so there's actually two things wrong here, from what I can see.

The attached patch gets picture caching working again on Linux and passes try, but it doesn't handle merging multiple rounded clips (so it fixes the regression from this case, but won't handle the second issue when sidebar is enabled, yet).

It does introduce more fuzziness on several tests - where one side now gets a promoted compositor clip and the other doesn't (e.g. it uses an image-mask to implement the clip). I think these are OK.

Effectively we check for a root level stacking context that has a grouped clip, and instead mark it as redundant if the clip-chain can be promoted to a compositor clip. Then, when the clip-tree is built, it now finds the same clip on all prims at the top level and promotes to a compositor clip, instead of the tile cache barriers being inside the stacking context. This is all a fragile hack, but it seems to work for now, and will get much better with some of the longer term work I'm doing (which will change how the clip-tree gets built and how tile cache barriers work).

Thoughts?

Flags: needinfo?(mozilla) → needinfo?(emilio)

Yeah, I agree. The patch looks reasonable, but we should probably file another bug (or attach another one) to do the rounded clip merging, specially since the front-end wants to put extra rounded corners for things like split view and so on.

Flags: needinfo?(emilio)
See Also: → 2023003
Pushed by gwatson@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/70509b17674d https://hg.mozilla.org/integration/autoland/rev/d7f12a140463 Allow stacking contexts with groups clips to be promoted to compositor clips r=layout-reviewers,gfx-reviewers,emilio,lsalzman

https://hg.mozilla.org/mozilla-central/rev/d7f12a140463

Status: NEW → RESOLVED
Closed: 6 months ago
status-firefox150: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 150 Branch
No longer depends on: 1979083
See Also: → 1979083
Duplicate of this bug: 2014122
Regressions: 2023968
No longer blocks: 2014122
See Also: 2018860 →
Regressions: 2024198

(In reply to Pulsebot from comment #35)

Pushed by gwatson@mozilla.com:
https://github.com/mozilla-firefox/firefox/commit/70509b17674d
https://hg.mozilla.org/integration/autoland/rev/d7f12a140463
Allow stacking contexts with groups clips to be promoted to compositor clips
r=layout-reviewers,gfx-reviewers,emilio,lsalzman

Perfherder has detected a devtools performance change from push d7f12a1404635f04c0d345fdbd942cd8b6ac9cfa.

No action is required from the author; this comment is provided for informational purposes only.

Improvement Test Platform Options Absolute values [old vs new]
4% damp screenshot.DAMP (doc) linux2404-64-shippable e10s fission stylo webrender 689.96 ms -> 660.70 ms

Need Help or Information?

If you have any questions, please reach out to afinder@mozilla.com. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

Keywords: perf-alert

(In reply to Pulsebot from comment #35)

Pushed by gwatson@mozilla.com:
https://github.com/mozilla-firefox/firefox/commit/70509b17674d
https://hg.mozilla.org/integration/autoland/rev/d7f12a140463
Allow stacking contexts with groups clips to be promoted to compositor clips
r=layout-reviewers,gfx-reviewers,emilio,lsalzman

Perfherder has detected a mozperftest performance change from push d7f12a1404635f04c0d345fdbd942cd8b6ac9cfa.

No action is required from the author; this comment is provided for informational purposes only.

Improvement Test Platform Options Absolute values [old vs new]
30% test_caching.html No cache again linux2404-64-shippable 2.79 ms -> 1.96 ms
22% test_fetch.html Cold fetch linux2404-64-shippable 6.10 ms -> 4.79 ms
9% test_update.html Main callback linux2404-64-shippable 17.48 ms -> 15.88 ms
9% test_update.html Server update linux2404-64-shippable 17.38 ms -> 15.88 ms

Need Help or Information?

If you have any questions, please reach out to afinder@mozilla.com. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

Attachment #9541744 - Attachment is obsolete: true
QA Whiteboard: [qa-triage-done-c151/b150]
Regressions: 2023445
Blocks: 2004925
Blocks: 1979083
See Also: 1979083 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: