Closed Bug 1998432 Opened 9 months ago Closed 6 months ago

Express quad shader coordinates in a consistent coordinate space

Categories

(Core :: Graphics: WebRender, task, P3)

Product:
Core
Component:
Graphics: WebRender
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
149 Branch
Tracking Flags:
Tracking Status
firefox149 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

Bug 1998432 - Remove unused raster node member in PrimTask. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Remove unused device pixel scale parameter. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Use device space for quads that have simple transforms. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Add a GpuBufferBuilderF/I convenience method to push a single sequence of blocks. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Rename EdgeAaSegmentMask into EdgeMask. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Round non-anti-aliased edges edges when in device space. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Build prim-space clip quad primitive using the primitive's layout space rect. r=gw
48 bytes, text/x-phabricator-request
Details | Review
Bug 1998432 - Rename ClipSpace::Raster into ClipSpace::Device. r=gw
48 bytes, text/x-phabricator-request
Details | Review

Problem/situation

At the moment, quad segments are expressed either in layout or device space which is a source of complexity and confusion:

  • quad segments drawn into an intermediate target use layout coordinates,
  • quad segments drawn into the destination target using the direct strategy use layout space,
  • quad segments drawn into the destination target using nine-patch or tiling strategies use device space.

Nine-patches requires the local to device transform to be axis aligned. Tiled quads don't seem to explicitly require it but accidentally do (See this assertion which mentions nine-patches but actually is in the tiled code path, I suspect that rotated tiled quads currently just crash).

The pattern is always specified in a sort of layout space that is relative to the primitive itself.
To make things work when quad segments are drawn using device space with a pattern specified in layout space we provide to the shader an optional device -> layout transform which is assumed to always be at most a scale and offset.

Currently this means that the same primitive is sometimes written twice in the gpu buffer (one with and one without the transform to fix pattern coordinates).

Quad shaders need to do at least some computation in a coordinate space that is appropriate for sizing and aligning intermediate render tasks. In general it is device space (see get_surface_rect).

It is conceptually simpler if decomposition happens in device space, because we can ensure that snapping the segments is done consistently with the way intermediate render tasks are sized. This enforces decomposition to happen in an axis-aligned coordinate system, which is fine most of the time and not optimal in other cases.
I also want to add a new clipping method (clip paths) that requires a decomposition in device space.

Aspirations

Ideally we'd express quad coordinates in a coordinate space that is a step between layout and device space (I would call it "raster space" if the name wasn't already taken, let's call it "quad space"). It would in general map device space exactly, but would also give us the flexibility to, for example, decompose a rotated primitive before applying the rotation (if we so chose).
The primitive data in the gpu buffer could contain coordinates in quad space as well as a transform from quad space to device space and a transform from quad space to layout space for patterns.
We probably need to support full 2d transforms for the patterns. I don't see how we can do an axis-aligned decomposition of a rotated primitive otherwise.

Duplicate of this bug: 1998434
Assignee: nobody → nical.bugzilla
Status: NEW → ASSIGNED

... At least if the transformation between the destination node and the common ancestor is invertible.

This is needed to express some raster-to-clip transforms.

After this patch the rule becomes:

  • If a quad primitive has a simple transform (scale+offset), the transform
    is applied to the CPU and the shader receives device space coordinates and
    an identity transform.
  • If a quad primitve has a complex transform, the transform is applied in
    the shader which receives layout coordinates.

This removes mismatches that were happening between the transform performed on the CPU
and the one applied on the GPU, which were causing glitches. It also simplifies the
mental model.

// Let's us write:
let address = gpu_buffer.push(&foo);

// equivalent to:
let mut writer = gpu_buffer.write(Foo::NUM_BLOCKS);
foo.write(&mut writer);
let address = writer.finish();

I am going to use this type for things that are similar but not limited to specifying AA edges.

Now that non-axis-aligned primitives are consistently projected to device space in the prepare phase,
we can guarantee that indrect quads cover their render tasks exactly.
This is done by rounding the coordinates of non-aa sides and making setting the task rect to the primitive's
tight clip rect in device space.

Since we do this for indrect quads we have to also do it for other rendering strategies, to prevent cracks
between side-by-side primitves without anti-aliasing.

Primitive space clips used to rely directly on the primitive's blocks with an additional prim->clip transform so to adjust the pattern coordinates of the clip.
This no longer works if the primitve is pre-transformed to device space because the device->clip transform that would be required to fix up the pattern coordinate sof the clip cannot always be expressed.
A example of the problem is with when the primitive and the clip use the same spatial node, which is under a (flattened) perspective transform. The prim/clip -> device transform can be expressed, the prim->clip transform as well (indentity), but the inverse device->clip primitive typically can't with flattened perspective.

A solution could be to produce geometry for the clip relative to its own spatial node. This way clips and regular primitives could work exactly the same way.
However a requirement for the clip is that it entirely covers the primitive. This was conveniently achieved by reusing exactly the geometry of the primitive for the clip, but with geometry psositioned in the clip's space we would have to project the primitive's bounds into the clip's space and inflate the clip until it covers the primitive.

A simpler solution (this patch) is to no longer reuse the primitive's gpu block for the clip (since they may have been pre-transformed), and instead build new clip gpu blocks from the primitive's local coordinates.

Keywords: leave-open

This reflects a change that happened in an previous patch in this series.

Attachment #9542422 - Attachment is obsolete: true
Pushed by amarc@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/dee6525df846 https://hg.mozilla.org/integration/autoland/rev/a90d928f8685 Revert "Bug 1998432 - Rename ClipSpace::Raster into ClipSpace::Device. r=gw" for causing reftest failures @ animate-layer-scale-inherit-1.html

Backed out for causing reftest failures

Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(nical.bugzilla)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
status-firefox149: --- → fixed
Target Milestone: --- → 149 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: