1759526 - WebRender's anti-aliasing doesn't work very well with non-uniform scaling

Status: ASSIGNED

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

Description

[Nicolas Silva [:nical]]

This becomes more evident when running reftests with SVG rectangles enabled.

The way our aa work is via a distance evaluation in layout space. This distance is then multiplied by a factor that uses length(fwitdh(local_pos)) to compute an approximation of how to get from that distance in local space to the distance in screen space (see compute_aa_range in shared.glsl).

Unfortunately that factor boils down to a sort of average of the scaling factor in x and y so when the scale is very different in x and y, the aa gets stretched in one axis and cut on the other.

For example take the css/css-transforms/scale/svg-scale-007.html wptest and increase the non-uniformity of the scaling transform to make the problem more evident.

Comment 1

[Nicolas Silva [:nical]]

A few thoughts:

• Doing the distance computation in screen space would be complicated because we would not be dealing with an arbitray quadrilateral instead of a simple rectangle distance.
• Maybe we could have a separate code path aa with simple transforms (no perspective, no shearing), but it'd still be complicated and slow to set up in the shader.
• Or maybe there's a way to separate the aa_range on each axis in a way that would still not be great for rotated shapes but at least look good in the axis-aligned case.
• Another option is to look into doing AA differently altogether.
  • For example switch to vertex-aa (nine-patching the aa part of the rectangle and computing the position of the extruded nine patch in the vertex shader using transformed positions and normals).
  • Or pass 4 clip lines in screen space and do the aa based on that.

Comment 2

[Nicolas Silva [:nical]]

I played around with shadertoy, adapting https://www.shadertoy.com/view/XsXSz4# to get the signed distance of an arbitrary quad in screen space.

float cross2(vec2 a, vec2 b) {
    return a.x * b.y - a.y * b.x;
}

float quad_distance(vec2 p, vec2 p0, vec2 p1, vec2 p2, vec2 p3)
{
    // Vectors along each edge.
    vec2 e0 = p1 - p0;
    vec2 e1 = p2 - p1;
    vec2 e2 = p3 - p2;
    vec2 e3 = p0 - p3;

    // Vectors from the point to each vertex.
    vec2 v0 = p - p0;
    vec2 v1 = p - p1;
    vec2 v2 = p - p2;
    vec2 v3 = p - p3;

    vec2 pq0 = v0 - e0 * clamp(dot(v0, e0) / dot(e0, e0), 0.0, 1.0);
    vec2 pq1 = v1 - e1 * clamp(dot(v1, e1) / dot(e1, e1), 0.0, 1.0);
    vec2 pq2 = v2 - e2 * clamp(dot(v2, e2) / dot(e2, e2), 0.0, 1.0);
    vec2 pq3 = v3 - e3 * clamp(dot(v3, e3) / dot(e3, e3), 0.0, 1.0);

    float d = min(
        min(dot(pq0, pq0), dot(pq1, pq1)),
        min(dot(pq2, pq2), dot(pq3, pq3))
    );

    float c0 = cross2(v0, e0);
    float c1 = cross2(v1, e1);
    float c2 = cross2(v2, e2);
    float c3 = cross2(v3, e3);
    // We could simplify the in/out test below with this if we could ensure
    // consistent vertex winding, but we probably can't with transforms
    // like scale(-1, 1).
    //float s = sign(max(max(c0, c1), max(c2, c3)));
    float s = sign(
        max(max(c0, c1), max(c2, c3)) * max(max(-c0, -c1), max(-c2, -c3))
    );

    // We can probably use a cheap sqrt approximation since we are only interested in
    // the distance in the range [-0.5, 0.5]. The following one is probably good enough:
    // return d * 2.0 * s
    return sqrt(d) * s;
}

That would require passing 8 float varyings instead of 4 (not too bad), but also requires a lot more alu than the current implementation.

Comment 3

[Nicolas Silva [:nical]]

Glenn what do you think?

Longer term my preferred solution would be to nine-patch all anti-aliased primitives and have the vertex shader compute the transformed normal in device pixels to position the aa bands around the primitive correctly. That way we only have interpolated per-vertex opacity instead of doing math in the fragment shader, but that would probably require a bit of work to integrate with other forms of primitive segmentations and the SVG work is blocked on this.

The shader in my previous comment is probably not too bad, especially without the square root. SWGL is likely going to have a harder time.

Comment 4

[Glenn Watson [:gw]]

It does seem unfortunate to introduce a lot (?) of extra shader work for the 0.1% use case where the existing approximation falls over. If we can come up with a way to only pay that cost in a small number of cases it's probably fine?

I guess the only way to know is to do some profiling on various GPUs. If, for example, we're still generally limited by memory bandwidth on lower-end GPUs, then adding extra ALU ops is probably going to have negligible impact.

I agree that long term nine-patching all prims with AA sounds like the right plan.

Comment 5

[Nicolas Silva [:nical]]

We should not pay the cost of AA when no primitive within the batch need anti-aliasing. I'll check that it's indeed the case.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Ensure the anti-aliasing code is only used when the AA feature is requested. r=gw

Also move the init_transform_vs call into write_vertex. That doesn't affect the behavior, but since the other init_transform_vs call is in write_transform_vertex I find it easier to follow what code path does what this way.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Remove dead intersect_lines. r=gw

Depends on D141476

Comment 8

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Reorganize write_vertex and write_transform_vertex. r=gw

Reoriganize the parts that do the same things so that they are identical. This will make it easier to refactor in a followup (and should make the refactor easier to review).

Depends on D141477

Comment 9

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Refactor write_transform_vertex. r=gw

This patch separates the antialiasing part of write_transform_vertex out into a new function that applies the clip rect and deals with antialiasing if need be. write_transform_vertex is now removed and the brush shader always calls clip_and_init_antialiasing and the write_vertex.

Internally clip_and_init_antialiasing does the ifdef dance for the anitaliasing feature and handles special casses such as axis-aligned transforms.

This patch should not introduce any behavior change, but the resulting code is easier to understand.

Depends on D141478

Comment 10

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Properly separate the ALPHA_PASS and ANTIALIASING shader features. r=gw

This turned out to be quite hard, beause of how the shader definition and selection code works. In particular it was hard to add a feature that only applies to the alpha pass without adding an extra useless opaque shader to each brush in the process. This whole system is begging to be rewritten from scratch. Until then this patch lets brush shaders take alpha-specific features and have most of the brushes explicitly have anti-aliasing in their alpha variants. The opacity brush no longer has both opaque with aa and opaque without aa. The image and opacity brushes which had a no-aa version don't have the aa code running in the vertex shader.

Depends on D141479

Comment 11

[Nicolas Silva [:nical]]

Attachment: Bug 1759526 - Restore the antialiasing defines in the shaders. r=gw

This patch restores the behavior prior to patch https://phabricator.services.mozilla.com/D141476. To get it to work there's some work needed on the management of shader permutations, that I might go back to later.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/775fd57ed8aa
Refactor write_transform_vertex. r=gw
https://hg.mozilla.org/integration/autoland/rev/caac77c8c2c1
Remove dead intersect_lines. r=gw

Comment 13

[Nicolas Silva [:nical]]

Hm these patches should have gone into another bug.

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/775fd57ed8aa
https://hg.mozilla.org/mozilla-central/rev/caac77c8c2c1

Comment 15

[Nicolas Silva [:nical]]

We can also do AA in the fragment shader with less instructions than the approach I described in comment 9, by passing either the screen space line coefficient of edge edge as varryings, or each edge's normal in screen space and two opposite corners as varyings (in both cases 3 varrying vec4), with some math done in the vertex shader to compute them.

That would reduce the fragment shader work to few dot products. That's what we did in the old layers code in https://hg.mozilla.org/mozilla-central/rev/f3d1111cf166085d23bb101dc56fb18f662658a0 except the line coefficients were computed on the CPU.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:nical, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.