Be consistent about how the algorithm we use to modulate colors. #3010

emilio · 2018-09-03T23:57:21Z

In particular, if the color is black, every browser does its best to infer the
color.

Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1488294

This should require a couple test updates in Gecko I presume.

This change is


        Cleanup the way we compute the darker / lighter colors for inset / ou…

…tset borders. Just so we have a single way to define the coefficients that we use to darken / enlighten a color for these kinds of borders.


        Be consistent about how the algorithm we use to modulate colors.

In particular, if the color is black, every browser does its best to infer the color. Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1488294 This should require a couple test updates in Gecko I presume.

gw3583 · 2018-09-04T00:00:54Z

@emilio Could you do a try run and paste the link?

emilio · 2018-09-04T00:10:10Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85f6a936bf2a8512f87d25fded0a8c05db7cae88 is the try run.

I expect a couple table tests will start failing, that's because right now we're painting both the test and the reference with black borders, which is wrong.

Marking them as expected failures should be fine. They should be fixed over with the work from https://bugzilla.mozilla.org/show_bug.cgi?id=1487407.

emilio · 2018-09-04T00:10:45Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c7f3991d72e4a2323706b3c2ad2433bc1cee85 should be a faster (but linux-only) try run.

Edit: wrong link, has more patches below those.


        Update reftests.


        Normalize small borders to solid.

emilio · 2018-09-04T01:59:00Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85f6a936bf2a8512f87d25fded0a8c05db7cae88 is the try run.

So, quick report on the failures... A bunch of the table-bordercollapse failures are expected, and should be progressions (before we should be rendering black borders instead of groove and co.).

Most of the failures are single-pixel borders. I missed that Gecko has the condition that if it needs two colors for a border but doesn't have two pixels, it'll just render a solid border with the unmangled color.

Can be seen in the following page:

<!doctype html>
<style>
  div {
    border: 1px groove black;
  }
</style>
<div style="border: 1px groove black">Should not have lighter border</div>
<div style="border: 10px groove black">Should have lighter border</div>

The latest commit should address this. The failure in layout/reftests/bugs/186317-1.html is of the same nature.

A bit more worrying is the failure in sticky-legend-1.html. Looks like the rendering is correct, but we're getting the antialiasing slightly differently along the border. That's because (I presume) the weird position: sticky clipping, or something of that sort. I should be able to reduce it, but: @gw3583 / @jrmuizel / @staktrace have you seen something like that before?

emilio · 2018-09-04T02:07:25Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e3afa2058fd3f1ed19af3a2fd9eec0e9e817704 is a more updated try run which should only include the bordercollapse failures and the sticky legend one.

emilio · 2018-09-04T12:39:56Z

r? @kvark or @gw3583

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1488403 to fix the sticky legend reftest to not depend on the AA we do around the groove border, so assuming that gets accepted this should be green on try. I couldn't reproduce that failure locally, but I think I understand the root cause (see the patch there).

We may want to represent groove borders as two different border segments to avoid doing distance AA across them, but it's not clear it's better to me, and in any case it's not the point of neither this change or the test.

emilio · 2018-09-04T12:40:12Z

(or @nical, if you're familiar with this code)

nical · 2018-09-04T13:55:34Z

@bors-servo r+

bors-servo · 2018-09-04T13:55:35Z

📌 Commit 574bb24 has been approved by nical


        Auto merge of #3010 - servo:border-infer-black, r=nical

Be consistent about how the algorithm we use to modulate colors. In particular, if the color is black, every browser does its best to infer the color. Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1488294 This should require a couple test updates in Gecko I presume.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3010)

bors-servo · 2018-09-04T13:55:40Z

⌛ Testing commit 574bb24 with merge d89e290...

bors-servo · 2018-09-04T15:12:52Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: nical
Pushing d89e290 to master...

kvark · 2018-09-04T15:58:14Z

webrender/res/cs_border_segment.glsl

 }

 vec4[2] get_colors_for_side(vec4 color, int style) {
    vec4 result[2];
-    const vec2 f = vec2(1.3, 0.7);
+
+    bool is_black = color.rgb == vec3(0.0, 0.0, 0.0);


This may not be portable. IIRC, vector comparison produces vector of booleans. We should be using dot(color.rgb,color.rgb) or all(color.rgb == vec3(0.0, 0.0, 0.0))

I could be wrong but I think that some_vec3 == other_vec3 returns a boolean in glsl and that the equal function returns a vector of boolean.

Indeed. I just recalled a driver bug with those, hence my radar fired up the red light :)

Also note the relevant record in our own driver bug treasury: https://github.com/servo/webrender/wiki/Driver-issues#1728-1368---shader-compiler-bug-when--is-used-inline-as-function-arg


        Bug 1488403 - Make sticky legend reftest not depend on groove border …

…antialising. r=nical This reftest fails with my changes from servo/webrender#3010. I tried to debug locally but it passes here. The reason it fails is because WebRender does distance AA between the segments of a ridge / groove border, and there's a subpixel difference so we get the AA slightly different in the test. Gecko on the other hand represents these borders as different composed solid segments, so it can't have this problem, since it doesn't do AA across the segments. We may want to change that, but it's not clear to me it's wanted, since rounded corners for these look much better on WR, for example. Rather than fuzzing the test or something like that, make the test not rely on that, given it's testing the position of the legend. Differential Revision: https://phabricator.services.mozilla.com/D4934 --HG-- extra : moz-landing-system : lando


        Bug 1488403 - Make sticky legend reftest not depend on groove border …

…antialising. r=nical This reftest fails with my changes from servo/webrender#3010. I tried to debug locally but it passes here. The reason it fails is because WebRender does distance AA between the segments of a ridge / groove border, and there's a subpixel difference so we get the AA slightly different in the test. Gecko on the other hand represents these borders as different composed solid segments, so it can't have this problem, since it doesn't do AA across the segments. We may want to change that, but it's not clear to me it's wanted, since rounded corners for these look much better on WR, for example. Rather than fuzzing the test or something like that, make the test not rely on that, given it's testing the position of the legend. Differential Revision: https://phabricator.services.mozilla.com/D4934


        Bug 1488403 - Make sticky legend reftest not depend on groove border …

…antialising. r=nical This reftest fails with my changes from servo/webrender#3010. I tried to debug locally but it passes here. The reason it fails is because WebRender does distance AA between the segments of a ridge / groove border, and there's a subpixel difference so we get the AA slightly different in the test. Gecko on the other hand represents these borders as different composed solid segments, so it can't have this problem, since it doesn't do AA across the segments. We may want to change that, but it's not clear to me it's wanted, since rounded corners for these look much better on WR, for example. Rather than fuzzing the test or something like that, make the test not rely on that, given it's testing the position of the legend. Differential Revision: https://phabricator.services.mozilla.com/D4934 UltraBlame original commit: e4f3b85bc61bded27b7ddbdc18fea4ba55121d21


        Bug 1488403 - Make sticky legend reftest not depend on groove border …

…antialising. r=nical This reftest fails with my changes from servo/webrender#3010. I tried to debug locally but it passes here. The reason it fails is because WebRender does distance AA between the segments of a ridge / groove border, and there's a subpixel difference so we get the AA slightly different in the test. Gecko on the other hand represents these borders as different composed solid segments, so it can't have this problem, since it doesn't do AA across the segments. We may want to change that, but it's not clear to me it's wanted, since rounded corners for these look much better on WR, for example. Rather than fuzzing the test or something like that, make the test not rely on that, given it's testing the position of the legend. Differential Revision: https://phabricator.services.mozilla.com/D4934 UltraBlame original commit: e4f3b85bc61bded27b7ddbdc18fea4ba55121d21


        Bug 1488403 - Make sticky legend reftest not depend on groove border …

…antialising. r=nical This reftest fails with my changes from servo/webrender#3010. I tried to debug locally but it passes here. The reason it fails is because WebRender does distance AA between the segments of a ridge / groove border, and there's a subpixel difference so we get the AA slightly different in the test. Gecko on the other hand represents these borders as different composed solid segments, so it can't have this problem, since it doesn't do AA across the segments. We may want to change that, but it's not clear to me it's wanted, since rounded corners for these look much better on WR, for example. Rather than fuzzing the test or something like that, make the test not rely on that, given it's testing the position of the legend. Differential Revision: https://phabricator.services.mozilla.com/D4934 UltraBlame original commit: e4f3b85bc61bded27b7ddbdc18fea4ba55121d21

emilio added 2 commits Sep 3, 2018

emilio added 2 commits Sep 4, 2018

Update reftests.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

28c4b4f

Normalize small borders to solid.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

574bb24

emilio mentioned this pull request Sep 4, 2018

Border arrows are rendered incorrectly #2828

Closed

emilio deleted the border-infer-black branch Sep 4, 2018

kvark reviewed Sep 4, 2018

View changes

pyfisch mentioned this pull request Sep 16, 2018

Update Webrender servo/servo#21725

Merged

3 of 3 tasks complete

servo / webrender

Be consistent about how the algorithm we use to modulate colors. #3010

Be consistent about how the algorithm we use to modulate colors. #3010

emilio commented Sep 3, 2018 •

edited by larsbergstrom

gw3583 commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018 •

edited

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

nical commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / webrender

Join GitHub today

Be consistent about how the algorithm we use to modulate colors. #3010

Be consistent about how the algorithm we use to modulate colors. #3010

Conversation

emilio commented Sep 3, 2018 • edited by larsbergstrom

gw3583 commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018 • edited

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

emilio commented Sep 4, 2018

nical commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

This comment has been minimized.

kvark Sep 4, 2018

This comment has been minimized.

nical Sep 4, 2018

This comment has been minimized.

kvark Sep 4, 2018

This comment has been minimized.

kvark Sep 5, 2018

emilio commented Sep 3, 2018 •

edited by larsbergstrom

emilio commented Sep 4, 2018 •

edited