Use a nicer approximation for anti-aliasing. #1822

nical · 2017-10-06T14:21:39Z

Since changing k*length(fwdith(pos)) to use 0.7 instead of 0.5 greatly improved the visual quality of the corner borders in #1791, I am following up with the same change in the other places where we use distance to edges for the anti-aliasing.

In the process of doing that I had to fix the cs_clip_rectangle shader which had two problems:

the aa step was added to the distance which makes no sense (the distance is in CSS pixels while the aa step can be seen as a factor of the CSS to device pixel ratio) and was the reason reftests/transforms/rotated-clip.yaml looks bad.
smoothstep(0.0, afwidth, 1.0 - current_distance); doesn't make much sense (to me) either, we want to invert the result of the smoothstep rather than what we pass to it.

This change is

nical · 2017-10-06T14:24:39Z

I first need #1791 to land, then I'll update the reference images.

In the mean time as a teaser, on the left what webrender currently gives for the reftest transforms/rotated-clip.yaml, and on the right the same test with this PR applied:

cc @glennw

kvark

Looks fine to me. Might be worth putting some logic into shared?

glennw · 2017-10-08T22:26:46Z

Looks good to me, once reftests are updated.

nical · 2017-10-09T18:02:10Z

Actually I am going to have to re-do this one as well as #1791. I spent today revisiting the maths behind all of this and I found out that my fix in #1791 was not correct: I introduced an arbitrary bias that was compensating for another roughly equivalent bias, but my original assumption about the cause of the original bias was incorrect.
With #1791 border corners look good, the issues is that for them to match the the corner clips I have to introduce the same (arbitrary) fix. And then the same has to be done for transformed clips so that things line up properly at the junction and that fix ends up snowballing into pretty much anything with a distance. We could choose to do that but I first want to present a more correct alternative:

The real cause for the ugliness in #1750

Here we'll assume that the css to device transform is just a scaling transformation (showing that the simple case is wrong is enough to show that the rest is).
We use length(fwidth(position)) to get a css to device pixel ratio. The fwidth function returns the derivatives of the css position in x and y. since the css coordinate system is aligned with the device coordinate system, this means that "1 device pixel to the right is equal to abs(fwidth(position).x) css pixels to the right". That corresponds to the device pixel ratio. By using length(fwidth(position)) we are in fact measuring the diagonal of a square, that has the device pixel ratio on both sides. This introduces an error factor of sqrt(2) so the number afwidth which represents the range in which we look at shades of alpha is larger than it should be. That means we blur out the edge a bit more than we should.

Error in the arithmetic

Then we do something a bit odd, the simplified code is:

// d0 is the distance to the outer ellipse of the border
// d1 is the distance to the inner ellipse of the border

float d = max(d0, 0.5 * afwidth - d1); // adding 0.5 * afwidth shifts the inner ellipse outward
alpha = min(alpha, 1.0 - smoothstep(0.0, 0.5 * afwidth, d));
// this only does anti-aliasing over half the range of interest (in a range
// of [0, 0.5] device pixels away from the curve where it should be [-0.5, 0.5])

The above shifts the the inner ellipse towards the exterior of the corner, which leads to the most noticeable artefact shown in #1750 (in my opinion), where the inner part of the rounded border doesn't line up with the edges of the rectangle.
the fact that we "don't anti-alias" inside the the curve (by forgetting the [-0.5*afwidth, 0] range) has the effect of inflating the curve a bit and and make it look less smooth. that explains while the outer part of the corner looks also shifted outward.

It would make more sense to do:

float d = max(d0, -d1);
alpha = min(alpha, 1.0 - smoothstep(-0.5 * afwidth, 0.5 * afwidth, d));

glitch due to stretched aa range

(edit: remvoed some of the stuff here, better explained a few comments below).
The bottom line is: we can add smoothness by stretching the "aa range", and it tends to look nicer (albeit a bit blurrier), but it produces this ugly artifact at the junction between a straight, non-antialiased edge and the border corner. To avoid this artefact we need to make sure that the range inside of the border where we antialias never extends further than 0.5 device pixels inside he border. In the drawing above, the ugly artefact is linked to the two red bands inside of the border.

This means that instead of using 0.7*length(fwidth(pos)), we need something more like 0.4 to compensate for the sqrt(2) factor introduced by length. So that's the opposite from what I was going for in this PR(!).

The reason why a lot of people are usually widening the aa range is because it looks better as long as you don't have a junction between anti-aliased and non-antialiased edges. So that's typically good for distance field text which is most of literature about this kind of shader math online.

nical · 2017-10-10T16:53:27Z

I updated the PR to fix our aa code with more grounded math (and no more hacky bias to correct the bugs).

another thing that came up while fixing this is that there were places where we would compute the aa only on a half pixel in the exterior of the shape like this:

alpha = smoothstep(0.0, 0.5*length(fwidth(pos)), signed_distance)

Which is incorrect, we should instead smooth half a pixel in and half a pixel out like this:

float aa_range = 0.5 * length(fwidth(pos)); // we actually want 0.4 instead of 0.5 for other reasons.
alpha = smoothstep(-aa_range, aa_range, signed_distance)

kvark · 2017-10-10T19:30:59Z

@nical it's not easy to catch up with your thought progress, but one thing would help: what is the difference (on the picture) between "in theory" and "in practice"?
Also, perhaps you could attempt to make a condensed summary of the findings.

nical · 2017-10-10T21:44:47Z

@kvark I am talking a lot about a particular artifact so a good start is to look at the this image and focus on the artifact that is visible at the junction between the corver and the straight edges.

I made another drawing which better shows that particular bug:

What this shows is a function where y represents the alpha (white is totally outside and black totally inside), and x a signed distance to a border. In theory we want a 1 device pixel wide aa range that goes from 0.5px inside the curve to 0.5px outside the curve.
In practice, because our approximation uses length(fwidth) we have a slightly larger which leads to the second drawing where the aa ranges are stretched in and out of the shape.
In the ideal scenario above, pixels that have their boundaries exactly at the edge of the cruve (the edge of the curve is represented by the red dotted line), then the center of the pixel will sample exactly 0 or exactly 1, which matches the non-antialiased case at the bottom. If we stretch the aa range, however, these same pixels will start sampling shades of alpha between 0 and 1, and that's exactly what is producing that ugly pixel that should be black but is gray in the picture with the wrong borders.

So that was one of the bugs and it is fixed by using 0.4 as the coeffecient to compensate for the sqrt(2) inherited from using length to get the aa range.

Another bug was when we'd do smoothstep(0.0, 0.5*length(fwidth), signed_distance), that's the 4th drawing on the right. let me know if you want to delve into this one.

There was a few other bugs in some computations that were just wrong, it's hard to represent them graphically because the computation didn't quite make sense. Some of those in particular were the ones that made the curve look offset towards the exterior of the corner by half a pixel, which I originally misinterpreted by a pixel snapping related thing but in fact was not.

kvark · 2017-10-11T04:06:45Z

@nical thanks for explanation!
This concerns me most:

So that was one of the bugs and it is fixed by using 0.4 as the coeffecient to compensate for the sqrt(2) inherited from using length to get the aa range.

Why use fwidth followed by length in the first place? Shouldn't we instead just use dFdx and dFdy directly and separately?

nical · 2017-10-11T07:01:20Z

Why use fwidth followed by length in the first place? Shouldn't we instead just use dFdx and dFdy directly and separately?

I think that there is two sides to this question. The whole aa approximation is based on the evaluating a single distance between the pixel and the shape so the math isn't separable in x and y unless we reinvent it differently. As to why using fwidth instead of, say, the average of abs(dFdx) and abs(dFdy), I don't know for sure. This is a very common technique so there is certainly a reason, perhaps it is faster that way. Another way could be to just pass the css to device pixel ratio to the shader, but that would not work as well if a perspective transformation is applied.

kvark · 2017-10-11T14:05:23Z

@nical here is what I'd like to see us doing:

instead of returning the distance to occluder, we'd have a vector to the edge plus a boolean saying if we are inside or outside
that distance is transformed into pixel space by using the local space derivatives
AA is computed from the resulting pixel space vector, where it's fine to take the length

pseudo-code:

//calculate vector to the edge for XY
// Z=+1 for outside and Z=-1 for inside
vec3 edge_vec_inside = <>;
vec2 ddx = dFdx(local_pos);
vec2 ddy = dFdy(local_pos);
// vector to the closest edge in pixel space
vec2 pixel_edge_vec = vec2(dot(edge_vec_inside.xy, ddx) / dot(ddx, dd), dot(edge_vec_inside.xy, ddy), dot(ddy, ddy));
// pixel distance of 0.0 gives us alpha 0.5, distance of 0.5 gives us alpha 0.0 for the outside part, etc
float alpha = clamp(0.5 - edge_vec_inside.z * length(pixel_edge_vec), 0.0, 1.0);

nical · 2017-10-11T16:39:12Z

I don't think that's worth it. I'd rather take the common approach since it's well documented online, easier to understand (IMO), cheaper and should the give the same results in the vast majority of cases (no complicated transforms).
I agree that projecting a vector in screen space and then computing the distance has the advantage of being potentially more accurate in the (less common) case where a perspective is applied, but at the end of the day both methods sit on top of computing the alpha based on a distance which is already a rather crude approximation compared to exact coverage.
The aa looks pretty good with this PR already (even with transforms). If we want to improve on it I'd rather do something that benefits the common case like figure out the exact coverage analytically (that might not be too hard with circles and ellipses, although I can imagine perspective transforms being an issue there as well).

kvark · 2017-10-11T16:59:38Z

I'd rather take the common approach since it's well documented online

If it's well documented online, why do we struggle with it? Could you point me to that documentation explaining how the approximation was derived from a more precise AA model?

nical · 2017-10-12T07:52:48Z

There was two main sources of struggle:

we didn't properly implement it at first,
and we need to maintain a coherent look when transitioning between anti-aliased and not antialiased pieces of the same border. SDF text implementations tend to increase the aa range to give a smoother look (just a tweak of the constant you multiply with length(fwidth(pos))). Whether doing that is desirable is subjective, but it works when all of the connected part of a shape are smoothed the same way and we don't have that property in the way we draw corners. That one is the simpler issue but the easiest to explain visually so I ended up spending more time making little drawings about it.

My biggest struggle was that I took it for granted that the math was right and looked for causes of the artefacts where they were not (my initial theory about pixel snapping), it's only after I looked at other implementations and spent some time simplifying the code that I found out it was incorrect (thanks to your suggestion to simplify the code btw!).

nical · 2017-10-12T11:12:10Z

@glennw and @kvark, the reference images are updated and tests pass, this is ready for merge if you are ok with keeping the current approach.

kvark · 2017-10-12T14:31:20Z

webrender/res/cs_clip_border.glsl

    // Get AA widths based on zoom / scale etc.
-    vec2 fw = fwidth(local_pos);
-    float afwidth = length(fw);
+    float aa_range = 0.4 * length(fwidth(local_pos));


can we have 0.4 as a constant defined somewhere with a proper name + comments?

kvark · 2017-10-12T14:32:02Z

webrender/res/cs_clip_rectangle.glsl

+    // See comment in ps_border_corner about the choice of constants.
+    float aa_range = 0.4 * length(fwidth(pos));

+    return 1.0 - smoothstep(-aa_range, aa_range, current_distance);


would be great to share these 2 lines between shaders

kvark · 2017-10-12T14:33:10Z

Also, would you want to start a try push for Gecko?

kvark · 2017-10-12T17:09:47Z

webrender/res/ps_border_corner.glsl


        // Get the groove/ridge mix factor.
-        color_mix_factor = smoothstep(-aa_range, aa_range, -d2);
+        color_mix_factor = distance_aa(aa_range, d2);


shouldn't this be 1.0 - distance_aa(aa_range, -d2) instead?

kvark · 2017-10-12T17:10:32Z

webrender/res/ps_border_corner.glsl

    // reference line, and then apply AA along the edge.
    float ld = distance_to_line(vColorEdgeLine.xy, vColorEdgeLine.zw, local_pos);
-    float m = smoothstep(-aa_range, aa_range, ld);
+    float m = distance_aa(aa_range, -ld);


shouldn't this be 1.0 - distance_aa(aa_range, ld)?

At a certain point my brain decided that it was equivalent, but now I am very tired and counting on the test run (and/or a good night of sleep) to help coming up with a more definitive answer to this question.
(edit:) I reactivated my poor brain and I am pretty sure it's the same thing.

Actually, I think you are correct here. Will see the try push and merge if it's ok.

nical · 2017-10-12T17:21:22Z

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=336cbc19e4bf8390e416931474f5b995dcc175f9

glennw · 2017-10-12T20:59:05Z

Looks like a reasonable number of unexpected fails that will need to be investigated :(

nical · 2017-10-12T21:54:04Z

All of the try failures have a maximum difference of 2 which is symptomatic of changing anything related to anti-aliasing. Usually when we run into those we just add fuzziness to the tests and move on.

That said they are all in places where we seem to be drawing axis-aligned rectangles (I am surprised that there's any aa happening in that scenario). It could be because the constant factor (0.4 in this PR), should be 0.5/sqrt(2) = 0.35355. I chose 0.4 instead because of how smoothstep barely varies towards the endpoints (and is steeper in the middle) and with a result snapped to 256 shades of alpha I thought the aa would never leak out of the [-0.5, 0.5] pixel range.

kvark · 2017-10-12T22:03:27Z

@nical so you think we should proceed with an assumption that you'll change the tests expectations right upon Gecko's WR update?

glennw · 2017-10-12T22:04:04Z

@nical Cool, if we're happy to update the reftests in Gecko with fuzziness, that's great. My main concern at this point is that we have several tests in Servo / WPT that rely on pixel-perfect results when axis-aligned (i.e. no AA). I will run this PR through WPT today and report back the results!

glennw · 2017-10-12T22:11:58Z

Talking to @nical in IRC, we basically need to either (a) make the AA code pixel-perfect for axis-aligned rects, or (b) use a specific shader / mode when we know the rect is axis-aligned.

nical · 2017-10-12T22:16:28Z

I'll first try with 0.35355 instead of 0.4 (and if it doesn't look as good try the same with a linear interpolation instead of smoothstep, or use the more principled constant for straight lines and 0.4 for ellipses). If the failure were in curvy things i'd just fuzz the tests without hesitation but with straight lines I'm worried we're going to end up fuzzing a lot of tests which is no fun.

glennw · 2017-10-13T01:40:12Z

@nical @kvark Good news! Servo WPT results are:

  ▶ FAIL [expected PASS] /compositing-1_dev/html/mix-blend-mode-intermediate-element-overflow-hidden-and-border-radius.htm

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-3d-rotateY-stair-below-001.htm

The first one is probably related to #1776, and the second one is probably something else.

So from Servo's perspective, this PR is fine to merge.

This is the Gecko try run with your constant change:

https://hg.mozilla.org/try/rev/3152d4985b707a6dd3419552c4762cb77c3e4d59
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3152d4985b707a6dd3419552c4762cb77c3e4d59

I'll leave it up to @nical to decide what we need to do in regards to the Gecko failures, but r=me if we decide to update gecko fuzziness in those tests.

glennw · 2017-10-13T02:36:55Z

@nical @kvark I applied this patch on top of my brush PR (#1841) and it fixes all the test failures. So I'm doubly keen to get this merged once we are happy with the gecko try run. 🚀

nical · 2017-10-13T08:03:14Z

The new gecko run is encouraging: one unexpected failure for two unexpected passes (means we fixed two tests that were not passing before).
(edit: sorry, 2 other failures on corners and well within fuzzy range).

nical · 2017-10-13T09:46:34Z

I updated the PR with reference images etc. This should be good to land, at last.

There are a few places where I would have liked the aa to be a bit smoother, but at this point I think that it is best to first land this since it fixes a number of bugs, and later come back to do some polishing.
I have spent so much to staring at these pixels that I may very well be biased towards liking smoother and exaggerated aa, so it would probably good (at least for me) to wait a bit before fiddling with this again.

For the record, some possible avenues to polish this:

fiddling with the constants for curved shapes
using a linear interpolation instead of smootstep
changing the aa method for some types of shapes and looking into analytically figuring out exact coverage (maybe not worth it if it is expensive).

kvark · 2017-10-13T15:36:11Z

@nical I think you've been tortured enough, and we know the truth now :)
Thank you, great job nailing these pixels down!
@bors-servo r+

bors-servo · 2017-10-13T15:36:12Z

📌 Commit 8c429be has been approved by kvark

nical · 2017-10-13T15:40:12Z

@staktrace heads up there are a few reftest fuzziness values to change when landing this in gecko. I'll make a patch on monday to insert in the next wr update.

bors-servo · 2017-10-13T16:15:40Z

⌛ Testing commit 8c429be with merge db3209e...

Use a nicer approximation for anti-aliasing. Since changing `k*length(fwdith(pos))` to use `0.7` instead of `0.5` greatly improved the visual quality of the corner borders in #1791, I am following up with the same change in the other places where we use distance to edges for the anti-aliasing. In the process of doing that I had to fix the cs_clip_rectangle shader which had two problems: - the aa step was added to the distance which makes no sense (the distance is in CSS pixels while the aa step can be seen as a factor of the CSS to device pixel ratio) and was the reason reftests/transforms/rotated-clip.yaml looks bad. - `smoothstep(0.0, afwidth, 1.0 - current_distance);` doesn't make much sense (to me) either, we want to invert the result of the smoothstep rather than what we pass to it.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1822)

bors-servo · 2017-10-13T17:53:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing db3209e to master...

kvark approved these changes Oct 6, 2017

View reviewed changes

nical force-pushed the aa-distance branch from de274a9 to b636e79 Compare October 9, 2017 08:00

nical force-pushed the aa-distance branch from b636e79 to 6427cd5 Compare October 10, 2017 16:43

nical mentioned this pull request Oct 12, 2017

Switch box-shadows to use brush masks (and add basic brush prim). #1841

Merged

kvark reviewed Oct 12, 2017

View reviewed changes

nical force-pushed the aa-distance branch from f686d29 to 75796a4 Compare October 13, 2017 09:38

nical added 4 commits October 13, 2017 15:23

Fix the math behind antialiasing in the shaders.

9253545

Share the antialiasing code between shaders.

2cad56d

Tweak the aa constant factor.

b87efa9

Update reference images.

8c429be

nical force-pushed the aa-distance branch from 75796a4 to 8c429be Compare October 13, 2017 13:31

bors-servo merged commit 8c429be into servo:master Oct 13, 2017

glennw mentioned this pull request Oct 16, 2017

Update WR (details below): servo/servo#18890

Merged

Use a nicer approximation for anti-aliasing. #1822

Use a nicer approximation for anti-aliasing. #1822

Uh oh!

Conversation

nical commented Oct 6, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nical commented Oct 6, 2017

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

glennw commented Oct 8, 2017

Uh oh!

nical commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The real cause for the ugliness in #1750

Error in the arithmetic

glitch due to stretched aa range

Uh oh!

nical commented Oct 10, 2017

Uh oh!

kvark commented Oct 10, 2017

Uh oh!

nical commented Oct 10, 2017

Uh oh!

kvark commented Oct 11, 2017

Uh oh!

nical commented Oct 11, 2017

Uh oh!

kvark commented Oct 11, 2017

Uh oh!

nical commented Oct 11, 2017

Uh oh!

kvark commented Oct 11, 2017

Uh oh!

nical commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nical commented Oct 12, 2017

Uh oh!

kvark Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

kvark Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

kvark commented Oct 12, 2017

Uh oh!

kvark Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

kvark Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

nical Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

nical commented Oct 12, 2017

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

nical commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Oct 12, 2017

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

nical commented Oct 12, 2017

Uh oh!

glennw commented Oct 13, 2017

Uh oh!

glennw commented Oct 13, 2017

nical commented Oct 6, 2017 •

edited by larsbergstrom

Loading

nical commented Oct 9, 2017 •

edited

Loading

nical commented Oct 12, 2017 •

edited

Loading

nical Oct 12, 2017 •

edited

Loading

nical commented Oct 12, 2017 •

edited

Loading

nical commented Oct 13, 2017 •

edited

Loading