Webrender: Blur filter not clipped properly

Created on 27 Jul 2018 · 14Comments · Source: servo/webrender

I've been noticing for some time weird blur under Facebook videos when running Firefox with Webrender. I've been able to track down the issue and create a minimal testcase:

<!DOCTYPE html>
<html>
<head>
    <title>Test</title>
</head>
<body>
    <div style="height:200px;width:200px;overflow:hidden">
        <img src="http://via.placeholder.com/200x200" style="filter:blur(20px)">
    </div>
</body>
</html>

Side by side comparison with Opera:

soon bug

Source

Pryx

👍4

Most helpful comment

This was fixed in the relevant Gecko bug instead.

emilio on 31 Aug 2018

❤1 🎉1

All 14 comments

We create a statcking context which refers to the correct clip chain and has a blur filter applied to it, however the clip attached to the stacking context is applied to the content of the stacking context before the blur is applied.
@glennw is this the correct semantic of the stacking context clip? if so I guess we should wrap it in another stacking context that has the clip and probably make it so that the blur's stacking context doesn't get the clip attached.

My guess is that the intent was for the clip to be applied to the stacking context (rather than the source of its filter) but since it doesn't matter for most filters we need to handle blurs differently. We don't seem to have any code attempting to apply clips to a picture primitive though.

nical on 8 Aug 2018

Equivalent yaml testcase:

---
root:
  items:
    - type: stacking-context
      bounds: [100, 100, 300, 300]
      filters: blur(10)
      complex-clip:
        rect: [200, 200, 100, 100]
      items:
      - image: "reftests/filters/firefox.png"
        bounds: 20 20 256 256

nical on 8 Aug 2018

https://bugzilla.mozilla.org/show_bug.cgi?id=1459065 (has a regression range)
A blur breaks out of an iframe: Testcase

Darkspirit on 22 Aug 2018

Regressed by 5e422431.

emilio on 23 Aug 2018

As way of background, I think the following is true:

A long time ago, WR supported clips only on primitives, and not on stacking contexts.
To work around this, Gecko has some fairly complex logic to convert gecko clip chains to primitive clip chains that include any clips from parents that would normally apply to the stacking context.
We then added some functionality to WR to set a clip id on a stacking context. This was initially used for image masks that need to apply to the whole stacking context. At the time, we added an extra parameter to push_stacking_context since otherwise we'd get double clips with the way Gecko currently sets clip chains on primitives.
The Gecko code to do image masks appears to have changed, and the explicit clip id on a stacking context is currently not used at all by Gecko (@emilio was looking at this today).

There are two possible ways to resolve this, if my understanding above is correct:

(1) Ideally, we'd change WR to inherit the clip when pushing a stacking context, and change Gecko to not do the workarounds mentioned above. We'd also need to modify WR to be a bit smarter about handling clips on stacking contexts (it currently just allocates an off-screen surface if there's a clip on a stacking context - it should be smarter about skipping this if the clip doesn't require a surface - this should be reasonably simple to do).

(2) Perhaps there is a way to do a quicker / hackier fix by Gecko doing more workarounds to fix this bug by specifying the clip id explicitly for the stacking context. I'm not sure how easy / feasible this is.

So I think (1) is the "correct" fix, and is also a potential performance win overall (the clip chains for primitives would typically end up being simpler) but more work than (2).

(I discussed this with @emilio on vidyo. Posting a summary of the discussion here so we can get comments from @staktrace @jrmuizel @nical @mstange ). Does that sound about right? Any thoughts on which is the right approach, or if there's a third option that is preferable?

gw3583 on 24 Aug 2018

👍1

Since this type of effect causes the stacking context to generate a picture primitive that ends up rendered as an image brush, I was thinking of getting the stacking context code to set the local clip rect for that primitive according to the stacking context's own clip (right now the local clip rect on the picture prim is not used).
For more complex clips like rounded corners I assume that it already works since we build a mask on top of the blurred intermediate target (I should actually check though).

nical on 24 Aug 2018

@gw3583 Supposing with go with (1), which looks reasonably to me. Can we than retain the clip_node_id parameter that is given to push_stacking_context to have the semantic of being applied to the resulting (filtered/shadowed/etc) contents?

kvark on 24 Aug 2018

@kvark I think we don't want / need the extra clip_node_id parameter at all, in the case of (1). If we encounter a stacking context with filters on it, we would know that any clip from the clip stack should implicitly be applied to the filter list?

gw3583 on 24 Aug 2018

I fairly sure that making stacking contexts clip their children will make certain CSS use cases impossible. An example of this is when an item inside a stacking context is a child of a different containing block, which can affect its clipping, but not how it is stacked.

mrobinson on 24 Aug 2018

In Gecko we handle that case by not clipping the stacking context, and instead clipping only the children that should be affected by the clip. WR doesn't need to think about this case because the API can already express it.

mstange on 24 Aug 2018

@mrobinson @mstange How does that work in the case of a blur filter? I guess we would have a clip on the stacking context in that case (which only affects the filter list), and the items would have their normal per-item clips, is that right?

gw3583 on 24 Aug 2018

The CSS case that Martin refers to cannot happen for blur filters. It cannot happen for any filters, even: The CSS filter property causes the creation of a containing block, so elements inside the filter cannot escape from the filter's ancestor clips.

With the blur filter, my expectations for WR would be: per-item clips on the items inside of the stacking context get applied normally, before the blur. Then the blur is applied. And then at the end, the clip on the stacking context is applied to the result.

mstange on 25 Aug 2018

@mstange @mrobinson Ah, right. So, am I understanding you correctly that your suggestion is that we don't need to change anything in WR and Gecko needs to supply the clip info to the push_stacking_context call?

gw3583 on 27 Aug 2018

This was fixed in the relevant Gecko bug instead.

emilio on 31 Aug 2018

❤1 🎉1

Was this page helpful?

0 / 5 - 0 ratings