Support backface:hidden in layers-free mode.

RESOLVED FIXED in mozilla57

Status

()

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement
Importance:
P1
normal
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years ago

People

(Reporter: mtseng, Assigned: mtseng)

Tracking

Version:
unspecified
Target:
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected)

Details

(Whiteboard: [wr-mvp])

Attachments

(3 attachments)

Bug 1392200 - Update interfaces for backface-visibility support.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1392200 - Add backface-visibility support for layers-free mode.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1392200 - Turn on some backface-visibility reftests.
59 bytes, text/x-review-board-request
kats
: review+
Details
Assignee

Description

•
2 years ago 
Right now we always show backface:hidden element. This fails some test cases.

Comment 2

•
2 years ago
mozreview-review 
Comment on attachment 8899376 [details]
Bug 1392200 - Update interfaces for backface-visibility support.

https://reviewboard.mozilla.org/r/170614/#review175870

I don't know about the correctness of this but patch looks sane. Please comment on the bug with at least one test name that fails without this change but passes with this change. That way if we later need to check if this change was regressed it will be easier since we'll have a record of which test this is intended to make pass.

::: gfx/layers/wr/WebRenderLayerManager.cpp:246
(Diff revision 1)
>        aDisplayList->AppendToBottom(itemSameCoordinateSystemChildren);
>        item->Destroy(aDisplayListBuilder);
>        continue;
>      }
>  
> +    if (item->Frame()->BackfaceIsHidden() && aSc.IsBackfaceVisible()) {

You can just call item->BackfaceIsHidden() here
Attachment #8899376 - Flags: review?(bugmail) → review+

Comment 5

•
2 years ago 
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8712cfa2588
Check backface:hidden in layers-free mode. r=kats

Comment 6

•
2 years ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/f8712cfa2588
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Comment 7

•
2 years ago 
Current implementation still did not pass for all backface test cases. After some digging, I think we should support backface in webrender side. See issue https://github.com/servo/webrender/issues/1419

I'm going to implement it in webrender.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 10

•
2 years ago 
Comment on attachment 8899376 [details]
Bug 1392200 - Update interfaces for backface-visibility support.

This patch need re-review.
Attachment #8899376 - Flags: review+ → review?(bugmail)

Comment 11

•
2 years ago
mozreview-review 
Comment on attachment 8899376 [details]
Bug 1392200 - Update interfaces for backface-visibility support.

https://reviewboard.mozilla.org/r/170614/#review184984

::: commit-message-7dddb:1
(Diff revision 2)
> +Bug 1392200 - Update interfaces for babckface-visibility support. r=kats

s/babckface/backface/

::: gfx/webrender_bindings/WebRenderAPI.h:220
(Diff revision 2)
>  
>    void End();
>    void Finalize(wr::LayoutSize& aOutContentSize,
>                  wr::BuiltDisplayList& aOutDisplayList);
>  
> -  void PushStackingContext(const wr::LayoutRect& aBounds, // TODO: We should work with strongly typed rects
> +  void PushStackingContext(bool aIsBackfaceVisible,

For consistency I'd prefer making this the second argument rather than the first. Or even move it down next to aTransform or aPerspective since it's more closely related to those arguments.
Attachment #8899376 - Flags: review?(bugmail) → review+

Comment 12

•
2 years ago
mozreview-review 
Comment on attachment 8899376 [details]
Bug 1392200 - Update interfaces for backface-visibility support.

https://reviewboard.mozilla.org/r/170614/#review184988

::: gfx/webrender_bindings/WebRenderAPI.h:220
(Diff revision 2)
>  
>    void End();
>    void Finalize(wr::LayoutSize& aOutContentSize,
>                  wr::BuiltDisplayList& aOutDisplayList);
>  
> -  void PushStackingContext(const wr::LayoutRect& aBounds, // TODO: We should work with strongly typed rects
> +  void PushStackingContext(bool aIsBackfaceVisible,

Actually make it the last argument since that's what you do in the StackingContextHelper constructor in the second patch.

Comment 13

•
2 years ago
mozreview-review 
Comment on attachment 8907397 [details]
Bug 1392200 - Add backface-visibility support for layers-free mode.

https://reviewboard.mozilla.org/r/179080/#review184994

r=me with comments addressed

::: commit-message-c59e6:1
(Diff revision 1)
> +Bug 1392200 - Add backface-visibility support for layers-free mode. r=kats

Update the commit message to say that for layers mode we are just setting all the backfaces to visible because those codepaths are deprecated and will be removed eventually anyway.

::: gfx/layers/wr/WebRenderUserData.cpp:143
(Diff revision 1)
>    // a bunch of the calculations normally done as part of that stacking
>    // context need to be done manually and pushed over to the parent side,
>    // where it will be done when we build the display list for the iframe.
>    // That happens in AsyncImagePipelineManager.
>    wr::LayoutRect r = aSc.ToRelativeLayoutRect(aBounds);
> -  aBuilder.PushIFrame(r, mPipelineId.ref());
> +  aBuilder.PushIFrame(r, true, mPipelineId.ref());

This one is used in layers-free mode so shouldn't we be propagating the visibility from the display item at the call site of this function (in WebRenderLayerManager::CreateImageKey)?

::: layout/generic/nsBulletFrame.cpp:526
(Diff revision 1)
>        PixelCastJustification::WebRenderHasUnitResolution);
>  
>    for (layers::GlyphArray& glyphs : mGlyphs) {
>      aManager->WrBridge()->PushGlyphs(aBuilder, glyphs.glyphs(), mFont,
>                                       glyphs.color().value(),
> -                                     aSc, destRect, destRect);
> +                                     aSc, destRect, destRect, aItem->BackfaceIsHidden());

This should have a ! since PushGlyphs takes aBackfaceVisible

::: layout/painting/nsCSSRendering.cpp:1145
(Diff revision 1)
>                           focusWidths,
>                           focusRadii,
>                           focusColors,
>                           nullptr,
> -                         NS_RGB(255, 0, 0));
> +                         NS_RGB(255, 0, 0),
> +                         true);

Not sure why this is set to true, please add a comment. As it stands, because the nsCSSBorderRenderer takes "hidden" instead of "visible" as the argument you're saying that the backface of this always hidden. Not sure why. See my other comment later about making that constructor take "visible" instead.

::: layout/painting/nsCSSRenderingBorders.h:105
(Diff revision 1)
>                        const Float* aBorderWidths,
>                        RectCornerRadii& aBorderRadii,
>                        const nscolor* aBorderColors,
>                        nsBorderColors* const* aCompositeColors,
> -                      nscolor aBackgroundColor);
> +                      nscolor aBackgroundColor,
> +                      bool aBackfaceIsHidden);

For consistency please invert this so that it is aBackfaceIsVisible instead of aBackfaceIsHidden. Invert the argument at the call sites accordingly. Also change the new mBackfaceIsHidden member variable to mBackfaceIsVisible and update the use sites as needed.

::: layout/painting/nsCSSRenderingGradients.h:89
(Diff revision 1)
>                                    const mozilla::CSSIntRect& aSrc,
> -                                  float aOpacity = 1.0);
> +                                  float aOpacity = 1.0,
> +                                  bool aIsBackfaceVisible = true);

It looks like this function has only one call site (in nsImageRenderer::BuildWebRenderDisplayItems) which you have already modified in this patch, so I'd prefer making this new parameter not have a default value. It's a bit of a footgun to leave the default value as true when really the caller should be specifying what it is.

::: layout/tables/nsTableFrame.cpp:6497
(Diff revision 1)
>    int32_t mAppUnitsPerDevPixel;
>    mozilla::Side mStartBevelSide;
>    nscoord mStartBevelOffset;
>    mozilla::Side mEndBevelSide;
>    nscoord mEndBevelOffset;
> +  bool mBackfaceIsHidden;

Flip this to mBackfaceIsVisible as well and update all the getters/setters accordingly.
Attachment #8907397 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

•
2 years ago
mozreview-review 
Comment on attachment 8910151 [details]
Bug 1392200 - Turn on some backface-visibility reftests.

https://reviewboard.mozilla.org/r/181640/#review187150
Attachment #8910151 - Flags: review?(bugmail) → review+

Comment 21

•
2 years ago 
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/928f5840c78d
Update interfaces for backface-visibility support. r=kats
https://hg.mozilla.org/integration/autoland/rev/3844ac09ae6a
Add backface-visibility support for layers-free mode. r=kats
https://hg.mozilla.org/integration/autoland/rev/a2169b64cac2
Turn on some backface-visibility reftests. r=kats

Comment 22

•
2 years ago 
Backed out for Windows bustage at gfx/layers/d3d11/TextureD3D11.cpp(1132):

https://hg.mozilla.org/integration/autoland/rev/c21a648327d2ec27b7ee574c2cc2f1b29a3432ff
https://hg.mozilla.org/integration/autoland/rev/9883a2bd5c76d65da053a1867de867966db2125b
https://hg.mozilla.org/integration/autoland/rev/c5d703bcca5125024da1e02868a962300d907012

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a2169b64cac2b7297dceac1b8219d7be23c585a8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132247688&repo=autoland

z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1132): error C2661: 'mozilla::wr::DisplayListBuilder::PushImage': no overloaded function takes 4 arguments
z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1142): error C2660: 'mozilla::wr::DisplayListBuilder::PushNV12Image': function does not take 6 arguments
z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1367): error C2661: 'mozilla::wr::DisplayListBuilder::PushImage': no overloaded function takes 4 arguments
Flags: needinfo?(mtseng)

Comment 24

•
2 years ago 
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/525ab770ca31
Update interfaces for backface-visibility support. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/28bb2483daae
Add backface-visibility support for layers-free mode. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/9febdbb6447c
Turn on some backface-visibility reftests. r=kats
You need to log in before you can comment on or make changes to this bug.