Relax use of constant buffers in Advanced Layers

RESOLVED FIXED in Firefox 56

Status

()

Core
Graphics: Layers
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

40 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 2 obsolete attachments)

Early on Advanced Layers did not aggressively use constant buffers, but it had an API to share a single buffer across multiple shader slots and draw calls. It uses a feature called "constant buffer offsetting", provided by ID3D11DeviceContext1. It seemed to work well so I moved a lot of vertex data into constant buffers, similar to what WebRender did when it had tiling.

It turns out constant buffer offsetting is either not well specified or not well supported by drivers. bug 1377866 is a good example. We've also had to workaround obscure bugs on our own infrastructure.

As a result, I'd like to move vertex shader constant data back into vertex buffers. I think this is more normal anyway, and it will simplify our shaders. On systems where the buffer sharing API is completely broken, we won't have to allocate as many ID3D11Buffers.

We'll still use shared buffers for pixel shaders, and for anything complicated like layer transforms.
Whiteboard: [gfx-noted]
Created attachment 8884571 [details] [diff] [review]
part 1, use unit triangles

Currently, our shaders use either a triangle list, or a unit quad with vertex information in a separate instance buffer.

The triangle case is not instanced, so each vertex contains all of the data that would normally be in the instance buffer. As we move more fields out of constant buffers and into vertices, triangle buffers will be duplicating more data.

This patch converts all the triangle-based shaders to use a unit triangle instead, so we can take advantage of instancing. There is no loss in performance as far as I can tell. It's also a modest code cleanup, eliminating the logic that had to decide how to build and bind a separate kind of buffer.
Attachment #8884571 - Flags: review?(matt.woodrow)
Created attachment 8884573 [details] [diff] [review]
part 2, refactor helpers

Build vertex buffers is really annoying, since there is a large matrix of how the data is formatted. For example, a unit quad PaintedLayer needs to attach a Rect to the vertex, but a unit triangle PaintedLayer needs to attach two 3-tuples of coordinates instead. And a PaintedLayer with geometry needs to attach an arbitrary list of texture coordinates.

Meanwhile Color layers only have to attach a Color, no matter what the geometry is.

Currently each shader must expose this logic through a Traits class, but it is encapsulated too well. Each shader has to re-implement a ton of boilerplate which is evident in ShaderDefinitionsMLGPU.cpp.

This patch simplifies everything so shaders only have to expose the exact functionality they need. All input layouts now share a common prefix, so Traits can also share a base vertex structure. Each derived Traits can define its ancillary vertex information, as well as methods that build this information based on the geometry.

For example, ColorTraits doesn't care about its geometry, so its "MakeVertexData" function ignores the vertex type. It also does not need to override how polygons are handled. TexturedTraits however must export textured coordinates, so it overloads "MakeVertexData" for every geometry type, as well as the "GenerateTriangles" function. Note that we can now share a generic version of the base vertex logic.

This patch is purely a refactoring, which is why ColorTraits still uses a constant buffer index as its ancillary data. That will be removed in the next patch.
Attachment #8884573 - Flags: review?(matt.woodrow)
Created attachment 8884574 [details] [diff] [review]
part 3, drop constant buffer use

This patch expands upon part 2. First, it removes the "items" constant buffer that was used to store ancillary vertex data. Batches can still use a constant buffer if they want to, we just no longer force it. This required some shader updates.

Second, we now use Traits to attach unit quad geometry as well. In the ColorTraits case there is no change, since it doesn't care about geometry. In the TexturedTraits case, it attaches the texture uv-rect.
Attachment #8884574 - Flags: review?(matt.woodrow)
Comment on attachment 8884571 [details] [diff] [review]
part 1, use unit triangles

Review of attachment 8884571 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/MLGDeviceD3D11.cpp
@@ +831,5 @@
> +      { 1.0f, 0.0f, 0.0f }, { 0.0f, 1.0f, 0.0f }, { 0.0f, 0.0f, 1.0f }
> +    };
> +    mUnitTriangleVB = CreateBuffer(
> +      MLGBufferType::Vertex,
> +      sizeof(Vertex3D) * 4,

3?

::: gfx/layers/mlgpu/ShaderDefinitionsMLGPU.cpp
@@ +12,5 @@
>  
>  using namespace gfx;
>  
>  // Helper function for adding triangle vertices to shader buffers.
>  struct TriangleVertex

Maybe rename this (and the textured version) since it's now all the vertices for a given triangle, rather than a single one?
Attachment #8884571 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8884571 [details] [diff] [review]
> part 1, use unit triangles
>
> ::: gfx/layers/mlgpu/ShaderDefinitionsMLGPU.cpp
> @@ +12,5 @@
> >  
> >  using namespace gfx;
> >  
> >  // Helper function for adding triangle vertices to shader buffers.
> >  struct TriangleVertex
> 
> Maybe rename this (and the textured version) since it's now all the vertices
> for a given triangle, rather than a single one?

It gets moved around in the second patch, so I'll rename it there.
Created attachment 8884991 [details] [diff] [review]
part 2, refactor helpers

Same patch, w/ name change.
Attachment #8884573 - Attachment is obsolete: true
Attachment #8884573 - Flags: review?(matt.woodrow)
Attachment #8884991 - Flags: review?(matt.woodrow)
Created attachment 8884992 [details] [diff] [review]
part 3, drop constant buffer use

Same patch, rebased.
Attachment #8884574 - Attachment is obsolete: true
Attachment #8884574 - Flags: review?(matt.woodrow)
Attachment #8884992 - Flags: review?(matt.woodrow)
Attachment #8884991 - Flags: review?(matt.woodrow) → review+
Attachment #8884992 - Flags: review?(matt.woodrow) → review+

Comment 8

7 months ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e72eadd699
Use unit triangles for full polgyon shaders. (bug 1379314 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f2592fdf69
Refactor how vertices are added to unit-triangle shaders. (bug 1379314 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f20e3d7ed6
Don't use constant buffers for ancillary vertex data. (bug 1379314 part 3, r=mattwoodrow)

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30e72eadd699
https://hg.mozilla.org/mozilla-central/rev/09f2592fdf69
https://hg.mozilla.org/mozilla-central/rev/81f20e3d7ed6
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.