Use a better approximation for the border corner shader. #1791

nical · 2017-10-02T17:50:44Z

This change is

nical · 2017-10-02T17:53:19Z

This is an approximation, itself on top of another approximation. I tried the "real fix" and the difference is hardly noticeable with code that is a lot messier so I am inclined to go with this solution that is imperfect but already much better than what we currently have.
I'd argue that if we want a better solution we might as well go for an better approximation of the pixel coverage as suggested by @pcwalton.

r? @glennw

glennw · 2017-10-02T21:03:11Z

Thanks for looking into this!

A couple of comments (well, questions actually, since I haven't looked at this stuff for a while):

The comments suggest that the coordinate is snapped to pixel boundaries - is that actually true? It certainly is for device pixels, but I thought the local space values were not snapped?
It seems like the 0.5 offset you apply needs to take into account zoom factor and/or device pixel ratio?
We'd need to update the reftest images too.

nical · 2017-10-02T22:50:23Z

The comments suggest that the coordinate is snapped to pixel boundaries - is that actually true?

I think it is. Not quite from looking at the code but it's visible from the screen capture from issue #1750 where I drew some circles on top of Markus's original picture, the distance looks very much like it is sampled from the corner of a pixel rather than the sample, and my interpretation is that it is because we are interpolating the positions from a varying rect that is snapped in the vertex shader (need to check).

It seems like the 0.5 offset you apply needs to take into account zoom factor and/or device pixel ratio?

Hm. I am not 100% certain but I think that we don't need a scaling factor since we are operating in device pixels there, or if we do I think that other code around it also need to be adjusted. I'll double check.

We'd need to update the reftest images too.

Yeah, I meant to submit this for feedback rather than final review (I wasn't 100% confident about the popularity of the approximation), I'll come back with some screenshots and update reftests tomorrow.

glennw · 2017-10-03T02:13:30Z

All the SDF operations for clipping etc always operate in local (CSS) space. So those distance calculations you modified are not in device space.

nical · 2017-10-03T13:43:59Z

All the SDF operations for clipping etc always operate in local (CSS) space. So those distance calculations you modified are not in device space.

Right, I managed to wrap my head around this and indeed, this is all in css pixels with fwitdh used to get the step factor in device pixels. Cranking up the device pixel ratio with my original patch messed the corner up pretty noticeably.

This version of the patch fixes that by using the the average of the width and height of fwidth(pos) which is (I think) a better approximation of the pixel size (length(fwidth) is closer to the diagonal of a square describing the pixel ratio in x and y) and gave subjectively better results.

I kept k * length(fwidth) for the aa step, but instead of using 0.5 I changed the factor to 0.7 to compensate for the shape of the smoothstep function which is steeper in its center and smooth towards the end-points. I actually took this constant from the OpenGL insight book, but I saw it in various SDF text shaders I found online as well. Changing this from 0.5 to 0.7 gave a very noticeable improvement in look of the anti-aliasing.

In the image below, before/after this PR, what firefox renders for one of the corners as a reference and the same corner with a pixel ratio of 8 to show that it doesn't break this time.

nical · 2017-10-03T15:35:42Z

There are still a few reftest failures, apparently from the change in how a side's color transitions into the other. I'm looking into that.

nical · 2017-10-05T17:32:07Z

The groove and ridge border reftest failures were really weird: the tests were already not passing on my computer and started not passing on try with this PR I have no idea why. The modulated colors (vColor00/01/11/10) of the border had their components between -0.3 and 1.3 instead of 0.0, 1.0, which meant that while the final color would be clamped to the proper range, intermediate computations like the transition between these colors were saturating which produced wrong results.

The last commit fixes that, even though it should not have been related to the aliasing of the border radius.

kvark · 2017-10-05T17:41:25Z

webrender/res/ps_border_corner.glsl

-    vColor01 = vec4(color0.rgb * modulate.y, color0.a);
-    vColor10 = vec4(color1.rgb * modulate.z, color1.a);
-    vColor11 = vec4(color1.rgb * modulate.w, color1.a);
+    vColor00 = vec4(clamp(color0.rgb * modulate.x, vec3(0.0), vec3(1.0)), color0.a);


wouldn't it be simpler to clamp the modulate vec?

It's not necessarily incorrect for modulate to be out of the 0..1 range if we want the modulation to brighten the color. I am not sure whether we need that (I assume we do, otherwise maybe the bug is in the values we pass to the shader)

right, I see now. Why are you clamping the colors here anyway?

Under the assumption that we can pass modulate components greater than 1, this is the place that makes most sense to me since this is where the base colors are computed. Any later computation can "get it wrong" by assuming they are in the range that goes according to their semantic (colors). Where would you have done the clamping?

It largely depends on whether the semantics of border colors is defined for vertices as opposed to pixels. I assumed that W3C would care about pixel colors, while the fact that GPU have vertices and interpolate between them is an implementation detail. In this case, the clamping should be done for the pixels, not here.

These are flat varying variables so there's no interpolation at all. Even if there was, it would be incorrect to interpolate between the the non-clamped values (you'd end up with colors constant over a certain range as they are capped at the brightest value until the result of the interpolation gets to the correct range and then it would start interpolating. This is actually the equivalent of one of the bugs this PR is fixing but at the vertex interpolation level instead of the mix between two sides of the border.

nical · 2017-10-05T20:35:03Z

I'll update the reference images to match the adjusted transitions between the sides.


        Use a better approximation for the border corner shader.


        Clamp the modulated border colors to valid values.


        Update the reference images.

glennw · 2017-10-06T06:10:24Z

Wow, those reference images look so much better!

nical · 2017-10-06T06:32:23Z

@glennw the PR is ready for a real review this time.

kvark

a few more annoying questions for you

kvark · 2017-10-06T15:37:54Z

webrender/res/ps_border_corner.glsl

+
+        // A half device pixel in css pixels (using the average of width and height in case
+        // there is any kind of transform applied).
+        float half_px = 0.25 * (fw.x + fw.y);


it sounds to me that 0.5 * length(fw) would be slightly more appropriate, which is almost similar to aa_step

if you consider fw to be "kinda like a device pixel ratio in x and y", length(fw) gives something that is the diagonal of the rectangle described as the pixel ratio whereas here we want something that is equal to a side of that square (the aa math here makes the approximation that fw.x and fw.y are somewhat similar). There is an average of the two as an effort to not do something bad with some transforms.
Since this is all on top of approximations I first felt like it wouldn't matter too much and tried to just use 0.5*length(fw) (even if it overshoots a little), and the results weren't as good so I ended up with this instead.

kvark · 2017-10-06T15:37:54Z

webrender/res/ps_border_corner.glsl


        // SDF subtract main radii
-        float d_main = max(d0, 0.5 * afwidth - d1);
+        float d_main = max(d0, aa_step - d1);


seems a bit weird that we add a half pixel to distances only to then subtract the aa_step from them. Isn't it a bit redundant? Shouldn't we instead just work with pure distances and have a single step constant that takes into account the difference between the current aa_step and half_px?

I agree that we can probably rethink this code and make it a bit simpler.

kvark

Thanks for detailed responses! I don't have any major concerns, leaving for @glennw to merge.

nical · 2017-10-06T15:58:36Z

a few more annoying questions for you

Don't worry, this stuff is subtle, we'd better make sure it is well understood.

glennw · 2017-10-08T21:17:48Z

There's a clear visual improvement in all the updated reftests.

@bors-servo r+

bors-servo · 2017-10-08T21:17:49Z

📌 Commit 7a336bd has been approved by glennw

glennw · 2017-10-08T21:18:02Z

Thanks @nical for looking into this!

bors-servo · 2017-10-08T21:59:19Z

⌛ Testing commit 7a336bd with merge 59502ed...


        Auto merge of #1791 - nical:corner, r=glennw

Use a better approximation for the border corner shader. Fixes #1750.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1791)

bors-servo · 2017-10-08T22:48:49Z

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 59502ed to master...


        Auto merge of #1822 - nical:aa-distance, r=kvark

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)

nical force-pushed the nical:corner branch from b564cb5 to 504a8a2 Oct 3, 2017

kvark reviewed Oct 5, 2017

View changes

nical added 3 commits Oct 2, 2017

Use a better approximation for the border corner shader.

ab09be1

Clamp the modulated border colors to valid values.

159da59

Update the reference images.

Loading status checks…

7a336bd

nical force-pushed the nical:corner branch from 71baecc to 7a336bd Oct 5, 2017

nical mentioned this pull request Oct 6, 2017

Use a nicer approximation for anti-aliasing. #1822

Merged

kvark reviewed Oct 6, 2017

View changes

kvark approved these changes Oct 6, 2017

View changes

nical deleted the nical:corner branch Oct 9, 2017

servo / webrender

Use a better approximation for the border corner shader. #1791

Use a better approximation for the border corner shader. #1791

nical commented Oct 2, 2017 •

edited by larsbergstrom

nical commented Oct 2, 2017

glennw commented Oct 2, 2017

nical commented Oct 2, 2017

glennw commented Oct 3, 2017

nical commented Oct 3, 2017 •

edited

nical commented Oct 3, 2017

nical commented Oct 5, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Oct 5, 2017

glennw commented Oct 6, 2017

nical commented Oct 6, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark left a comment

nical commented Oct 6, 2017

glennw commented Oct 8, 2017

bors-servo commented Oct 8, 2017

glennw commented Oct 8, 2017

bors-servo commented Oct 8, 2017

bors-servo commented Oct 8, 2017

servo / webrender

Join GitHub today

Use a better approximation for the border corner shader. #1791

Use a better approximation for the border corner shader. #1791

Conversation

nical commented Oct 2, 2017 • edited by larsbergstrom

nical commented Oct 2, 2017

glennw commented Oct 2, 2017

nical commented Oct 2, 2017

glennw commented Oct 3, 2017

nical commented Oct 3, 2017 • edited

nical commented Oct 3, 2017

nical commented Oct 5, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Oct 5, 2017

glennw commented Oct 6, 2017

nical commented Oct 6, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark left a comment

nical commented Oct 6, 2017

glennw commented Oct 8, 2017

bors-servo commented Oct 8, 2017

glennw commented Oct 8, 2017

bors-servo commented Oct 8, 2017

bors-servo commented Oct 8, 2017

nical commented Oct 2, 2017 •

edited by larsbergstrom

nical commented Oct 3, 2017 •

edited