Add compositor support for triangle layers (for DirectX backend)

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: miko, Assigned: miko)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54+ wontfix, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
This bug tracks the implementation of bug 1285380 for DirectX backend.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
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 8

a year ago
mozreview-review
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.
(Assignee)

Comment 9

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8835502 [details]
Bug 1323791 - Part 1: Use no-repeat texture rects with polygon layers

https://reviewboard.mozilla.org/r/111204/#review112618
Attachment #8835502 - Flags: review?(matt.woodrow) → review+

Comment 15

a year ago
mozreview-review
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
Attachment #8835503 - Flags: review?(matt.woodrow) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8835507 [details]
Bug 1323791 - Part 6: Expect plane intersections to work with D3D11 backend

https://reviewboard.mozilla.org/r/111214/#review112622
Attachment #8835507 - Flags: review?(matt.woodrow) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8835504 [details]
Bug 1323791 - Part 3: Add dynamic vertex shaders

https://reviewboard.mozilla.org/r/111208/#review113718
Attachment #8835504 - Flags: review?(bas) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8835505 [details]
Bug 1323791 - Part 4: Add recompiled D3D11 shaders

https://reviewboard.mozilla.org/r/111210/#review113722
Attachment #8835505 - Flags: review?(bas) → review+

Comment 19

a year ago
mozreview-review
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.
Attachment #8835506 - Flags: review?(bas) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
mozreview-review-reply
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 27

a year ago
mozreview-review
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
Attachment #8835506 - Flags: review?(bas) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

a year ago
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)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

a year ago
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
Keywords: checkin-needed
If I understand bug 1328020, we may want to consider requesting Aurora approval on this?
Flags: needinfo?(mikokm)
(Assignee)

Comment 46

a year ago
(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.
Flags: needinfo?(mikokm)
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 :)
status-firefox54: --- → affected
tracking-firefox54: --- → ?
Depends on: 1346253
Track 54+ for directX compositor triangle rendering.
tracking-firefox54: ? → +
How are we feeling about aurora uplift at this stage?
Flags: needinfo?(mikokm)
I think we should uplift to 54 so all platforms have 3D plane splitting in the same release. 

Anthony: any concerns re: this one?
Flags: needinfo?(anthony.s.hughes)
(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?
Flags: needinfo?(anthony.s.hughes)
(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?
Flags: needinfo?(dvander)
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).
Flags: needinfo?(dvander)
Let this ride the train and won't fix in 54. Mark 54 won't fix.
status-firefox54: affected → wontfix
(Assignee)

Updated

a year ago
Flags: needinfo?(mikokm)
(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?
Flags: needinfo?(anthony.s.hughes)
(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.
Flags: needinfo?(anthony.s.hughes)

Updated

a year ago
Blocks: 1354813

Updated

a year ago
Blocks: 1366321
You need to log in before you can comment on or make changes to this bug.