Closed
Bug 1392200
Opened 6 years ago
Closed 6 years ago
Support backface:hidden in layers-free mode.
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox56 | --- | unaffected |
firefox57 | --- | unaffected |
People
(Reporter: mtseng, Assigned: mtseng)
References
Details
(Whiteboard: [wr-mvp])
Attachments
(3 files)
Right now we always show backface:hidden element. This fails some test cases.
Comment hidden (mozreview-request) |
Comment 2•6 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+
Assignee | ||
Comment 3•6 years ago
|
||
Sure, the test case is https://searchfox.org/mozilla-central/source/layout/reftests/transform-3d/animate-backface-hidden.html
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69cd9f19c1c6d96e9eca1e3a599533b2a1a2f60
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f8712cfa2588
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 7•6 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 → ---
Assignee | ||
Updated•6 years ago
|
See Also: → https://github.com/servo/webrender/issues/1419
Updated•6 years ago
|
Blocks: stage-wr-nightly
Updated•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Status: REOPENED → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 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•6 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•6 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•6 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) |
Assignee | ||
Comment 17•6 years ago
|
||
All comments are addressed. Here is try https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a25a44a404174c9fe79cedf68620559b1df3bbf
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
Latest try https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03a63f93e3eb4252d4cf3f454f156e8731ba75d
Comment 20•6 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•6 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•6 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)
Assignee | ||
Comment 23•6 years ago
|
||
new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed3d87c3a8f5b9cd0165d6efa2795b56d26cfdc2
Flags: needinfo?(mtseng)
Comment 24•6 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
![]() |
||
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/525ab770ca31 https://hg.mozilla.org/mozilla-central/rev/28bb2483daae https://hg.mozilla.org/mozilla-central/rev/9febdbb6447c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•