1489937 - Text runs seem to get an incorrect backface-visible flag in the display list

Status: RESOLVED FIXED

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

Description

[Glenn Watson [:gw]]

This attachment is for an unrelated bug, but demonstrates the issue: https://bug1442915.bmoattachments.org/attachment.cgi?id=8955810

In the WR display list, the text runs have backface_is_visible flag set to true, but I think they should be false.

The background rectangles and images *do* appear to correctly have backface_is_visible flag set to false.

WR also has a backface_is_visible flag on stacking contexts, and it's possible WR is not interpreting that as Gecko expects (WR only considers the owning stacking context for this flag, not the current stacking context hierarchy).

But, given that the rectangle primitives do seem to have the flag correctly set, it sounds like it may be a bug in text run display item handling?

The result is that both the front and back facing text runs get drawn, which causes flickering since the planes in the 3d rendering context are collinear.

Comment 1

[Glenn Watson [:gw]]

This is noticeable on quite a few sites that use 3d transforms. It's also the main cause of noticeable artifacts on sites with 3d transforms (after https://github.com/servo/webrender/pull/3030 lands).

So it's probably worth finding out what the root cause is and where the fix should be applied (I suspect it's probably going to be a quick fix once we determine where the bug is in the code).

Comment 2

[Aria Beingessner [:Gankra]]

Did some debugging on this, we're getting fed a list like:

- stack-backface: visible
  - stack-backface: hidden
    - stack-backface: hidden
      - stack-backface: visible
         - text-backface: visible

my *guess* is that in gecko a higher backface-visibility is supposed to take precedent and make the children moot, but how backface-visibility is supposed to work is a bit muddy to me. Possibly related to how preserve-3d is processed? (at least in css, backface-visibility of a node is only handled if preserve-3d is present, and that isn't inherited, so if preserve-3d is "high enough" then the children could be backface-visible but still invisible)

I think mattwoodrow needs to chime in here, as no one else seems to know how this code in gecko works

Comment 3

[Aria Beingessner [:Gankra]]

matt says this page is weird enough that he needs to do some in-depth debugging, wants to take over

Comment 4

[Matt Woodrow (:mattwoodrow)]

My reading of the testcase is that we have cq-flipbox-flipper (that gets the flip animation), with preserve-3d and backface-visibility:hidden. We have a transformed child with cq-flipbox-back (with the permanent flip transform), so it takes part in the preserve-3d context. We also have an untransformed child for cq-flipbox-front, which doesn't take part in the preserve-3d context.

So I think the 3d context (in Gecko's interpretation) has two planes: One for the cq-flipbox-back, that has the combined transform of cq-flipbox-back+cq-flipbox-flipper, and one for the rest, that just has cq-flipbox-flipper's transform.

Both of these planes have backface-visiblity:hidden, and are exactly 180degrees rotated from one another, so we only display one of them at a time.

Comment 5

[Glenn Watson [:gw]]

That's my understanding of the test case too - but in the WR display list, the rectangles have is_backface_visible: false, but the text runs get is_backface_visible: true, and so WR thinks that the text runs are always visible, although it culls the backfacing rectangles as expected.

Comment 6

[Matt Woodrow (:mattwoodrow)]

I think that's somewhat expected, the text itself doesn't have backface-visibility:hidden set on it (and it's not inherited).

The flip transform (which is an ancestor of the text) creates a stacking context though (which should be rendered as if it were drawn to an intermediate surface, even if it's not implemented that way), and that stacking context/plane/surface has a hidden backface, which results in us not drawing any of the rectangles/text to the screen.

Comment 7

[Glenn Watson [:gw]]

OK, that makes sense. I think the problem is that we seem to have an extra "stacking context" in the WR display list. This is the relevant subset from a WR DL dump:

PushStackingContext (bf visible: false)
Rectangle (bf visible: false)
PushReferenceFrame + PushStackingContext (translate-y: -14.5, bf visible: true)
Text (bf visible: true)
PopReferenceFrame + PopStackingContext
PopStackingContext

Comment 8

[Aria Beingessner [:Gankra]]

pr: https://github.com/servo/webrender/pull/3040

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=386d8adb469cb458af4b5359de63c9a067345e22

additional discussion on irc:

> <gw> Gankro: mattwoodrow: also replied to that :) so I think the issue is that there is this extra "stacking context" in the DL due to the transform / reference frame, for some reason
> 
> <gw> Gankro: mattwoodrow: and that doesn't have backface_visible: true 
> 
> <Gankro> gw: do you mean the extra stacking context *does* have bf:visible?
> 
> <gw> Gankro: yea, it does - which I think (?) is probably the issue
> 
> <Gankro> right
> 
> <gw> Gankro: (WR does take both the prim and current stacking context flags into account)
> 
> <gw> Gankro: (but not the whole stack of stacking contexts - WR can do that, easily, if that's the right behaviour - just not sure which is right / wrong)
> 
> <mattwoodrow> gw: So I see the nested stacking context, but it’s not part of the same preserve-3d context
> 
> <mattwoodrow> so I think the text should be visible within the inner stacking context, and the inner stacking context is visible within the flip stacking context, but the flip itself is hidden
> 
> <Gankro> gw: is there a way to express that bf-visibility should be inherited from the parent in wr, as matt is describing?
> 
> <gw> Gankro: not currently, but it's trivial to implement, if that's the behavior we want
> 
> <mattwoodrow> Gankro: gw: It should be a container effect on the stacking context, so it hides everything under it, even if those things are more stacking contexts
> 
> <Gankro> StackingContextHelper could also probably track this to munge it into wr format if desirable; not sure what's better
> 
> <gw> Gankro: mattwoodrow: yep, it sounds like that's the right solution - and it doesn't bother me if it's in WR or in SCHelper. It's certainly easy to add to WR, and I can write up a few sentences on how, if you want to do that and kick off a try?
> 
> <Gankro> gw: my intuition is this should be easier to do wr-side, if only to minimize the number of places we have to "remember" to check for parent visibility
> 
> <gw> Gankro: sure, sounds good - have you got cycles to prototype it?
> 
> <Gankro> gw: sure, with those few sentences :)
> 
> <gw> Gankro: cool :) https://github.com/servo/webrender/blob/217cfd09eb65ae3b082ae60c98c25dbf0e754ad2/webrenderGankro: we maintain a stack during DL flattening of 'FlattenedStackingContext' structs and that already has is_backface_visible, for the actual stacking context. just including the previous is_backface_visible flag during push_stacking_context should probably do the trick, I think? (from the last entry on that sc_stack)
> 
> <Gankro> gw: as in just && them?
> 
> <gw> Gankro: I think so, yea. It could be as simple as a one line change, hopefully!
> 
> <mattwoodrow> || them?
> 
> <Gankro> we want !visible to win, right?
> 
> <gw> It's true if it should be visible even in the presence of a backfacing transform
> 
> <mattwoodrow> Oh right, it’s the reverse flag from what gecko does, sorry
>
> <Gankro> gw: would it be possible to ever have bf-visible content inside bf-invisible content with this change?
> 
> <gw> Gankro: no - but I think mattwoodrow is suggesting that's what we want?
> 
> <Gankro> I believe gecko currently allows that, as long as there's more preserve-3ds in the chain?
> 
> <gw> Gankro: mattwoodrow: oh, hmm - in that case you may need to also consult the establishes_3d_context bool in the same function, to know when to consider previous stacking context flags?
> 
> <Gankro> gw: see e.g. http://jsfiddle.net/9yv8f2eL/1/ in vanilla gecko
> the green background of the div is hidden, but the child which overrides backface-visible is visible
> but if you comment out the last line of css, the child loses preserve-3d and the parent wins
> 
> <mattwoodrow> Gankro: I think we want to consider the flag on the leaves of the 3d context
> and then that applies to all descendants of that leaf
> 
> <gw> Gankro: ok - push_stacking_context does check for preserve-3d and if a new 3d context is created, so can probably infer from that how to consider the backface flag?
> 
> <Gankro> ok so if !sc.preserve_3d { sc.bf_visible &= parent_sc.bf_visible; }
> right?
> and then the visibility on the text item itself should be irrelevant in this case?
> 
> <mattwoodrow> That seems right I think
> 
> <gw> Gankro: I think so, yea - a try run will confirm :)
> 
> <Gankro> mattwoodrow: what should be the outcome if the child sets backface-visible=true, but *not* preserve-3d? Is it ignored still?
> 
> <mattwoodrow> Gankro: Yeah I think it is
> 
> <Gankro> mattwoodrow: ok so rather it should be sc.preserve_3d ? sc.bf_visible : parent.bf_visible
>  
> <mattwoodrow> Gankro: Yeah, that sounds right
> 
> <Gankro> mattwoodrow: and what should we do if we aren't preserve_3d *and* we're the root stacking context?
> 
> <mattwoodrow> Gankro: I guess we can just use the local value? I’m 99% confident it’ll always be visible anyway

Comment 9

[Aria Beingessner [:Gankra]]

Only two failures, which seem to be of the form:

<body>
  <div style="transform: rotatex(180deg); backface-visibility: hidden; width: 100px; height: 100px;">
    Test Text
  </div>
</body>

For some reason this is supposed to have a hidden backface, in spite of the absence of any preserve-3d annotation.

Comment 10

[Darkspirit]

(In reply to Glenn Watson [:gw] from comment #0)
> This attachment is for an unrelated bug, but demonstrates the issue:
> https://bug1442915.bmoattachments.org/attachment.cgi?id=8955810

The current state of bug 1480914 looks similar to me, but wouldn't be fixed by servo/webrender#3040.