This bug tracks the implementation of bug 1285380 for DirectX backend.

These patches implement rendering of arbitrary polygons with Direct3D 11 compositor backend. Layers with polygon geometry are rendered using new shaders that use dynamic vertex buffer. Rectangular layers are still rendered with the previous rendering path. This feature can be disabled by setting a pref flag layers.geometry.d3d11.enabled to false.

Comment on attachment 8835504 [details] Bug 1323791 - Part 3: Add dynamic vertex shaders https://reviewboard.mozilla.org/r/111208/#review112460 ::: gfx/layers/d3d11/CompositorD3D11.hlsl:44 (Diff revision 1) > Texture2D tMask : register(ps, t5); > Texture2D tBackdrop : register(ps, t6); > > struct VS_INPUT { > float2 vPosition : POSITION; > + float2 vTexCoords : TEXCOORD0; Is there a particular reason we don't use a separate input struct for these shaders? It seems wasteful to add this element to all the other shader types. It's probably worth changing input layouts considering how rarely this is used.

(In reply to Bas Schouten (:bas.schouten) from comment #8) > Comment on attachment 8835504 [details] > Bug 1323791 - Part 3: Add dynamic vertex shaders > > https://reviewboard.mozilla.org/r/111208/#review112460 > > ::: gfx/layers/d3d11/CompositorD3D11.hlsl:44 > (Diff revision 1) > > Texture2D tMask : register(ps, t5); > > Texture2D tBackdrop : register(ps, t6); > > > > struct VS_INPUT { > > float2 vPosition : POSITION; > > + float2 vTexCoords : TEXCOORD0; > > Is there a particular reason we don't use a separate input struct for these > shaders? It seems wasteful to add this element to all the other shader > types. It's probably worth changing input layouts considering how rarely > this is used. No other reason than code simplicity with shared input layouts and vertex types. I'll add a separate input layout and structure for vertices with texture coordinates.

Comment on attachment 8835502 [details] Bug 1323791 - Part 1: Use no-repeat texture rects with polygon layers https://reviewboard.mozilla.org/r/111204/#review112618

Comment on attachment 8835503 [details] Bug 1323791 - Part 2: Add and enable pref flag for DX layer geometry https://reviewboard.mozilla.org/r/111206/#review112620

Comment on attachment 8835507 [details] Bug 1323791 - Part 6: Expect plane intersections to work with D3D11 backend https://reviewboard.mozilla.org/r/111214/#review112622

Comment on attachment 8835504 [details] Bug 1323791 - Part 3: Add dynamic vertex shaders https://reviewboard.mozilla.org/r/111208/#review113718

Comment on attachment 8835505 [details] Bug 1323791 - Part 4: Add recompiled D3D11 shaders https://reviewboard.mozilla.org/r/111210/#review113722

Comment on attachment 8835506 [details] Bug 1323791 - Part 5: Add generalized DrawGeometry() method and use it with DrawQuad() and DrawTriangles() https://reviewboard.mozilla.org/r/111212/#review113728 First pass, I might have more comments on a subsequent patch :). ::: gfx/layers/d3d11/CompositorD3D11.cpp:220 (Diff revision 2) > +CompositorD3D11::UpdateDynamicVertexBuffer(const nsTArray<gfx::TexturedTriangle>& aTriangles) > +{ > + HRESULT hr; > + > + // Resize the dynamic vertex buffer if needed. > + if (aTriangles.Length() > mMaximumTriangles) { Realize that recreating a vertex buffer for each of these is going to make this codepath -slow-. ::: gfx/layers/d3d11/CompositorD3D11.cpp:313 (Diff revision 2) > sizeof(mAttachments), > &mAttachments); > > D3D11_INPUT_ELEMENT_DESC layout[] = > { > - { "POSITION", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0 }, > + { "POSITION", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0 } Don't remove trailing comma here. ::: gfx/layers/d3d11/CompositorD3D11.cpp:327 (Diff revision 2) > if (Failed(hr, "CreateInputLayout")) { > *out_failureReason = "FEATURE_FAILURE_D3D11_INPUT_LAYOUT"; > return false; > } > > - Vertex vertices[] = { {{0.0, 0.0}}, {{1.0, 0.0}}, {{0.0, 1.0}}, {{1.0, 1.0}} }; > + Vertex vertices[] = { Does this change serve a purpose? ::: gfx/layers/d3d11/CompositorD3D11.cpp:959 (Diff revision 2) > + return aUseBlendShaders > + ? mAttachments->mVSQuadBlendShader[aMaskType] > + : mAttachments->mVSQuadShader[aMaskType]; > +} > + > +template<typename Geometry> I'm not super-fond of this template magic but I can see why it was the easiest way to implement this. ::: gfx/layers/d3d11/CompositorD3D11.cpp (Diff revision 2) > CancelFrame(); > *aRenderBoundsOut = IntRect(); > return; > } > > - mContext->IASetInputLayout(mAttachments->mInputLayout); For performance is probably best to still set this here, set the other one only when you need it and reset it after the DrawCall, similarly keep the quad Vertex buffer set. State changes are expensive.

Comment on attachment 8835506 [details] Bug 1323791 - Part 5: Add generalized DrawGeometry() method and use it with DrawQuad() and DrawTriangles() https://reviewboard.mozilla.org/r/111212/#review113728 Thank you for the review! > Realize that recreating a vertex buffer for each of these is going to make this codepath -slow-. If a layer is split once, the resulting rectangular polygon is triangulated to two triangles. The initial mMaximumTriangles is set to 64 (just an arbitrary number), which can present results from very complex splits. Thus I think the cases where this codepath gets called are rare. An alternative approach would be to upload vertices to GPU in multiple passes and use more than one draw call. This solution might not be trivial (I ran to what I assume were concurrency problems when trying it out). > Does this change serve a purpose? Not anymore. > I'm not super-fond of this template magic but I can see why it was the easiest way to implement this. I agree. The main reason for it was to somewhat mimic CompositorOGL and BasicCompositor implementation and to preserve one rendering path instead of branching in multiple places. Would you prefer an approach where geometry type is supplied as an enum argument instead?

Comment on attachment 8835506 [details] Bug 1323791 - Part 5: Add generalized DrawGeometry() method and use it with DrawQuad() and DrawTriangles() https://reviewboard.mozilla.org/r/111212/#review118302

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s c4cb109f8e38 -d 20075fb33fc7: rebasing 379528:c4cb109f8e38 "Bug 1323791 - Part 1: Use no-repeat texture rects with polygon layers r=mattwoodrow" rebasing 379529:560ae420ec17 "Bug 1323791 - Part 2: Add and enable pref flag for DX layer geometry r=mattwoodrow" merging gfx/thebes/gfxPrefs.h merging modules/libpref/init/all.js rebasing 379530:02dc074b6dd9 "Bug 1323791 - Part 3: Add dynamic vertex shaders r=bas" rebasing 379531:7d8ca3775a94 "Bug 1323791 - Part 4: Add recompiled D3D11 shaders r=bas" merging gfx/layers/d3d11/CompositorD3D11Shaders.h warning: conflicts while merging gfx/layers/d3d11/CompositorD3D11Shaders.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Rebased the changes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d24460bbb225e3a30db80a72e0718e973a4fbea

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb02c1d5d211 Part 1: Use no-repeat texture rects with polygon layers r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/619f4bab301a Part 2: Add and enable pref flag for DX layer geometry r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/63608bcf9f43 Part 3: Add dynamic vertex shaders r=bas https://hg.mozilla.org/integration/autoland/rev/88dd8c893e26 Part 4: Add recompiled D3D11 shaders r=bas https://hg.mozilla.org/integration/autoland/rev/5bf82944a617 Part 5: Add generalized DrawGeometry() method and use it with DrawQuad() and DrawTriangles() r=bas https://hg.mozilla.org/integration/autoland/rev/c9f09b670a27 Part 6: Expect plane intersections to work with D3D11 backend r=mattwoodrow

https://hg.mozilla.org/mozilla-central/rev/bb02c1d5d211 https://hg.mozilla.org/mozilla-central/rev/619f4bab301a https://hg.mozilla.org/mozilla-central/rev/63608bcf9f43 https://hg.mozilla.org/mozilla-central/rev/88dd8c893e26 https://hg.mozilla.org/mozilla-central/rev/5bf82944a617 https://hg.mozilla.org/mozilla-central/rev/c9f09b670a27

If I understand bug 1328020, we may want to consider requesting Aurora approval on this?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45) > If I understand bug 1328020, we may want to consider requesting Aurora > approval on this? These changes modify the rendering code that is always called on Windows, and as such it is a bit risky to uplift this so early.

Per our IRC discussion, I wasn't necessary asking if we had to do it right this second vs. just making sure it's on the relevant radars if we intend to do so at all eventually. Happy to let this bake for awhile on Nightly first :)

Track 54+ for directX compositor triangle rendering.

How are we feeling about aurora uplift at this stage?

I think we should uplift to 54 so all platforms have 3D plane splitting in the same release. Anthony: any concerns re: this one?

(In reply to Jet Villegas (:jet) from comment #50) > I think we should uplift to 54 so all platforms have 3D plane splitting in > the same release. > > Anthony: any concerns re: this one? It's the first time I'm seeing this so I don't think uplift would be prudent. As near as I can tell this has had no manual testing apart from sitting on Nightly for a month hoping people would file bugs if they see them. This is not a trivial change and at the very least should be given some QA time before uplift is considered. I would suggest the following: 1) Conduct some focused manual testing over the next few days (I can work on this). 2) If no issues are found, uplift pref'd off by default and let it ride to Beta 3) Conduct an A/B TelEx in Beta 4) Pref on in Beta if no issues are found Does this sound agreeable?

(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #51) > (In reply to Jet Villegas (:jet) from comment #50) > > I think we should uplift to 54 so all platforms have 3D plane splitting in > > the same release. > > > > Anthony: any concerns re: this one? > > It's the first time I'm seeing this so I don't think uplift would be > prudent. As near as I can tell this has had no manual testing apart from > sitting on Nightly for a month hoping people would file bugs if they see > them. This is not a trivial change and at the very least should be given > some QA time before uplift is considered. > > I would suggest the following: > 1) Conduct some focused manual testing over the next few days (I can work on > this). This sounds good. As you test, it would be good to also compare to builds without the pref enabled. Our 3D CSS transformation implementation is really buggy without plane splitting, and the fixes here should really help. The big risk I see is around driver issues. > 2) If no issues are found, uplift pref'd off by default and let it ride to > Beta Miko: Any concerns about layers.geometry.d3d11.enabled == false? > 3) Conduct an A/B TelEx in Beta > 4) Pref on in Beta if no issues are found > > Does this sound agreeable? SGTM, Thx!

(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #51) > (In reply to Jet Villegas (:jet) from comment #50) > > I think we should uplift to 54 so all platforms have 3D plane splitting in > > the same release. > > > > Anthony: any concerns re: this one? > > It's the first time I'm seeing this so I don't think uplift would be > prudent... Same here (not knowing about it.) We really should get better at it. I don't think we should uplift this. We don't have the capacity to deal with a possible fallout. David, how's this related to the plane splitting work you did for advanced layers?

To avoid platform mismatches, we could disable plane splitting on other Fx54 compositors... that might even be advisable so we don't see articles like: "To get this working in Firefox, disable acceleration!" But to play devil's advocate this patch was pretty straightforward, the scary stuff all landed in 54 already. It's a nice bit of web compatibility to have. @Milan Advanced Layers uses Miko's layer sorting/geometry code, but doesn't use the shaders in this patch (I did use them as a reference).

Let this ride the train and won't fix in 54. Mark 54 won't fix.

(In reply to Gerry Chang [:gchang] from comment #55) > Let this ride the train and won't fix in 54. Mark 54 won't fix. OK. (In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #51) > As near as I can tell this has had no manual testing apart from > sitting on Nightly for a month hoping people would file bugs if they see > them. This is not a trivial change and at the very least should be given > some QA time before uplift is considered. > > I would suggest the following: > 1) Conduct some focused manual testing over the next few days (I can work on > this). Does not uplifting negate the feedback re: manual testing?

(In reply to Jet Villegas (:jet) from comment #56) > > Does not uplifting negate the feedback re: manual testing? Sorry for the delay. The simple answer to this is no. The longer answer is that we'll need to cover all the bases to make sure this is Beta-ready in 6 weeks.