1933527 - Picture caching doesn't work when the content area has a border radius clip

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Steps to reproduce:

1. Using the Browser toolbox, find a <stack class="browserStack"> element.
2. In the .browserStack rule, add the properties overflow: clip; border-top-left-radius: 50px;
3. Navigate to a page that has an HDR video (e.g. this youtube video) on an HDR-capable screen, for example the built-in screen of a modern Macbook Pro
4. Alternatively, set gfx.webrender.debug.picture-caching to true.

Expected results:
HDR videos should still be HDR.
The picture-caching overlay should look normal when scrolling simple web pages.

Actual results:
HDR videos become non-HDR.
The picture-caching overlay is all-red when scrolling simple web pages.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Sample patch:

diff --git a/browser/themes/shared/tabbrowser/content-area.css b/browser/themes/shared/tabbrowser/content-area.css
index 5c721ee07f0be..d9ab771b77f5c 100644
--- a/browser/themes/shared/tabbrowser/content-area.css
+++ b/browser/themes/shared/tabbrowser/content-area.css
@@ -53,6 +53,8 @@
   position: relative;
   z-index: var(--browser-area-z-index-tabbox);
   margin: 0;
+  overflow: clip;
+  border-top-left-radius: 10px;

   /* stylelint-disable-next-line media-query-no-invalid */
   @media (-moz-bool-pref: "sidebar.revamp") {

HDR videos become non-HDR.

So with the patch above (which uses a smaller radius) you can see that HDR videos only become non-HDR iff they are near the top (for some definition of near), e.g. if they intersect the rounded rect. I think something acceptable for realistic radii would be to promote them as long as they are not near any of the corners, perhaps?

Comment 2

[Markus Stange [:mstange]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

So with the patch above (which uses a smaller radius) you can see that HDR videos only become non-HDR iff they are near the top (for some definition of near), e.g. if they intersect the rounded rect.

Oh phew, that's reassuring! I was testing with a larger radius and this made me think it affected the entire page. But I can confirm that with a radius of 10px it only affects the video if it's intersecting it. With a radius of 25px the danger zone grows to 106px for me, not sure why.

I think something acceptable for realistic radii would be to promote them as long as they are not near any of the corners, perhaps?

Isn't that what seems to be happening, based on these observations?

To make it work even when the corner is intersecting the video, WR would need to tell the native compositor about rounded corner clipping. On macOS CoreAnimation can do rounded corner clipping. On Windows, DirectComposition seems to support this too, at least there is a method called IDCompositionRectangleClip::SetTopLeftRadiusX.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

(In reply to Markus Stange [:mstange] from comment #2)

I think something acceptable for realistic radii would be to promote them as long as they are not near any of the corners, perhaps?

Isn't that what seems to be happening, based on these observations?

Not quite, right? right now it becomes non-HDR if it intersects the rounded rect (at any point, even in places where the right clipping for the layer could be done with a regular non-rounded rect).

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Rejiggering to match bug 1933527. I'll move the HDR bits to bug 1933297 since it was filed earlier.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

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

:nsharpley, since you are the author of the regressor, bug 1908471, could you take a look?

For more information, please visit BugBot documentation.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

This will be disabled by pref on release as per bug 1934640.

Comment 7

[Bob Hood [:bhood]]

We will likely be addressing these types of issues in Q1 2025 with the sidebar support.

Comment 8

[Glenn Watson [:gw] | PTO until 6th Jan]

I have hacked together a prototype to evaluate how best to support this functionality without (significant) performance regressions. The prototype seems to work, I'm adding selected notes about it here so that I remember all this when working on a production implementation.

Problem Overview

• We need the content region (with rounded rect clip) to be a valid picture cache slice, which means:
  • It can't be inside a nested stacking context in the display list (e.g. a stacking context with a mask / filter), as that forces an off-screen target. In future, we may support tiling picture cache slices on child stacking contexts, but we don't currently. Once the content goes to an off-screen target, we no longer get per-tile invalidation, and we can get blurriness since the content is rendered to a single large target (restricted by max texture size).
  • This is not actually a problem in the existing sidebar implementation, as the rounded clip gets placed on the iframe display item, which doesn't introduce a stacking context with a mask. However, that leads to the next problem:
  • When we create a picture cache slice, we find the lowest common ancestor we can support in the clipping tree, and hoist that (and any parents) out to be applied as a clip on the picture cache slices during compositing, rather than the individual child display items. This means that clips that are in the root coordinate space (e.g. display port clips) don't cause unwanted invalidations of the content, due to the relative position of the root clips changing as the underlying content is scrolled. Similarly, this means that the valid_rect of the scrolling picture cache tiles is not clipped by the display port clip(s), allowing us to render content outside the current content clip region when rasterizing tile content.
  • However, when we find the lowest ancestor common clip, we currently skip clips that are complex, since we don't support applying those during compositing of picture cache tiles. The result is that when a complex clip exists in this scenario, it gets applied to all the child elements, which means lots of invalidations as content scrolls (in addition to requiring a separate mask render task for each piece of content that intersects with the root complex clip).

Solution

• Add support for a single rounded rect clip to be applied during compositing of picture cache tiles (both via platform specific native compositor and draw compositor).
• Modify the code that searches for lowest common ancestor clip to allow finding an ancestor that has a rounded rect clip.
  • Need to check other cases that we can't handle correctly (e.g. elliptical radii due to CoreAnimation limitations, non-axis-aligned transforms, more than one rounded clip etc).
• Build a clip-chain instance during picture cache updates, so that we have information available to mask the tiles (if using draw compositing).
• Add support for compositing picture cache tiles as a nine-patch primitive.
  • This means that tiles that are primarily opaque can still be drawn with blend disabled for better GPU performance, only blending at corners.
  • We need to extend the way the rectangle_occlusion module works to handle nine-patch rendering correctly, so that we still occlude areas where the majority of a tile is opaque.

Caveats

• For the Draw compositor implementation, there are a few options to apply the masking that we need to consider - rendering a mask and applying during composite, applying mask directly on top of the picture cache tiles, calculating the mask directly in the composite shader. Each has a few pros and cons.
• Although DirectComposition and CoreAnimation do support the clipping options we want on native surfaces, we could avoid spending the time implementing this if we ship layer-compositor mode everywhere first. If we do that, we can support the picture cache tile masking in the Draw compositor only, and it will work everywhere.

Comment 9

[Glenn Watson [:gw] | PTO until 6th Jan]

All the relevant patches above have landed in nightly, so this should be working correctly now (and I've verified that I don't see any unexpected picture cache invalidation when scrolling content with sidebar.revamp enabled).

I'll leave this open for a few more days while we deal with any follow up bugs or performance regressions, then we can hopefully close this out.