Handle gradients with multiple color stops at 0.0 or 1.0 #1189

eqrion · 2017-05-02T07:00:25Z

When a clamped gradient has multiple color stops at 0.0, the last one
will fill GradientData[0]. When a clamped gradient has multiple color
stops at 1.0, the first one will fill GradientData[N-1]. Both of these
conditions will cause the area outside of the gradient to not be filled
with the correct clamped color, which is the first or last gradient
color stop.

Filling the first or last color entry of GradientData with the first or
last color stop isn't exactly correct either because the first or last
color stop entry is only needed in this case for the range outside of
[0, 1].

This commit changes the GradientData so that the first color entry is
used for offset < 0, the last color entry is used for offset > 1, and
the color entries in between are for the range in between. The first and
last color entries are then filled with the correct colors for those
ranges.

This change is

eqrion · 2017-05-02T07:04:46Z

r? @kvark @glennw

This PR makes changes to GradientData so it collides with #1181 . I can rebase on that if it will be merged soon, or help rebase #1181 on this.

I hope this PR is clear, I got confused a few too many times by indices conversions and whatnot.

Gankra · 2017-05-02T13:27:26Z

I could split my gradient changes out to a different pr?

eqrion · 2017-05-02T15:10:53Z

If that's feasible that would work, otherwise this can wait.

kvark · 2017-05-02T18:52:11Z

webrender/src/prim_store.rs

@@ -393,6 +400,10 @@ impl GradientData {
            ColorF::new(0.0, 0.0, 0.0, 0.0)
        };

+        // Fill in the first gradient entry with the first color stop
+        self.fill_colors(cur_idx, cur_idx + 1, &cur_color, &cur_color);


this is a bit confusing. Perhaps, do fill_colors(0, 1, ...) followed by cur_idx = 1;?

Yes, I did something similar when I rebased.

kvark · 2017-05-02T18:52:11Z

webrender/src/prim_store.rs

@@ -381,7 +385,10 @@ impl GradientData {
    // Compute an entry index based on a gradient stop offset.
    #[inline]
    fn get_index(offset: f32) -> usize {
-        (offset.max(0.0).min(1.0) * GRADIENT_DATA_RESOLUTION as f32).round() as usize
+        let clamped = offset.max(0.0).min(1.0);
+        let scaled = clamped * (GRADIENT_DATA_LAST - GRADIENT_DATA_FIRST) as f32 + GRADIENT_DATA_FIRST as f32;


So this function returns indices between the GRADIENT_DATA_FIRST and GRADIENT_DATA_LAST, inclusive. It seems a little inconsistent that we have a border element to the left of FIRST (under index 0), but no border to the right of LAST.

Yes it returns from [GRADIENT_DATA_FIRST, GRADIENT_DATA_END] but the fill_colors code will only fill from [start_idx, next_idx). So we should have a border element before GRADIENT_DATA_FIRST, and starting at GRADIENT_DATA_END. I've added some comments, and they might make things clearer. Or less clear.

bors-servo · 2017-05-02T19:10:02Z

☔ The latest upstream changes (presumably #1191) made this pull request unmergeable. Please resolve the merge conflicts.

eqrion · 2017-05-03T04:44:03Z

Rebased this and added some comments around GradientData, it needed some. Whew, lots of fun exclusive and inclusive ranges.

Gankra · 2017-05-03T16:23:12Z

webrender/src/prim_store.rs

+            self.fill_colors(GRADIENT_DATA_LAST_STOP, GRADIENT_DATA_LAST_STOP + 1, &cur_color, &cur_color);
+
+            // Fill in the center of the gradient table, generating a color ramp between each consecutive pair
+            // of gradient stops. Each iteration of a loop will fill the indices in [cur_idx, next_idx). The


Should this be (next_idx, cur_idx]?

Yes I forgot to reverse that comment, it should be [next_idx, cur_idx)

bors-servo · 2017-05-03T17:31:10Z

☔ The latest upstream changes (presumably #1181) made this pull request unmergeable. Please resolve the merge conflicts.

eqrion · 2017-05-03T17:54:29Z

I've rebased and updated the comment. I just ran the gecko reftests with this patch and noticed that there are a couple of failures fixed but some new failures. So I'd like to investigate those first now.

kvark · 2017-05-03T18:22:00Z

Thanks @rlhunt ! I marked the PR as WIP for now.


        Handle gradients with multiple color stops at 0.0 or 1.0

When a clamped gradient has multiple color stops at 0.0, the last one will fill GradientData[0]. When a clamped gradient has multiple color stops at 1.0, the first one will fill GradientData[N-1]. Both of these conditions will cause the area outside of the gradient to not be filled with the correct clamped color, which is the first or last gradient color stop. Filling the first or last color entry of GradientData with the first or last color stop isn't exactly correct either because the first or last color stop entry is only needed in this case for the range outside of [0, 1]. This commit changes the GradientData so that the first color entry is used for offset < 0, the last color entry is used for offset >= 1, and the color entries in between are for the range in between. The first and last color entries are then filled with the correct colors for those ranges.

eqrion · 2017-05-04T01:56:37Z

The issue was that I took 2 of the 128 gradient data entries (leaving 126 left), and it looks like some reftests are sensitive to the resolution of the table. I tried 256 (254 for the table, 2 for the border) and that fixed a few but not all, so I've settled with doing 130 (128 for the table, 2 for the border) and that seems to work well.

glennw · 2017-05-05T02:57:33Z

@rlhunt Have you run this against the Servo reftests? If not, let me know and I'll run them locally here.

kvark · 2017-05-05T03:09:54Z

I tried 256 (254 for the table, 2 for the border) and that fixed a few but not all

That doesn't sound good

eqrion · 2017-05-05T18:44:54Z

I'll run the Servo reftests now. I've tried this on the Gecko reftests and it doesn't cause any regressions and fixes a few tests.

I tried 256 (254 for the table, 2 for the border) and that fixed a few but not all

That doesn't sound good

I can confirm that without this PR, if you change the table size to be 254 from 128, these tests fail. So I suppose these tests are sensitive to the inner table of GradientData size being a power of two.

kvark · 2017-05-05T19:29:35Z

@rlhunt do you see why increasing the table size could make the quality worse?

eqrion · 2017-05-05T19:41:48Z

Increasing the table size does actually seem to improve the quality, it's just seems to be a problem with non power of two sizes (or something else). 126 and 254 causes failures, 128 and 256 seem fine.

I ran some experiments here, noticeable increasing the inner table to 1024 actually resolved a test, so quality does improve, but I think 1024 is overkill.

eqrion · 2017-05-05T19:43:26Z

I've successfully ran the servo wpt-tests and css-tests with this change. I think it's interesting that the inner table size is very sensitive and we should do more follow up work with it, but it's a preexisting condition unrelated to this PR.

pyfisch · 2017-05-06T20:29:20Z

It seems like I duplicated some of the work done here on the servo side. 😞

servo/servo#16666 also contains a function to handle first and last stops and it is called fix_gradient_stops. If this is merged please make sure this passes testcases 2, 11, 12 and 14.

glennw · 2017-05-08T23:09:56Z

@rlhunt Is this ready for review / merge based on your comments above about running the Servo tests? Or is there more testing required based on the comment from @pyfisch above?

eqrion · 2017-05-09T15:03:53Z

@glennw This should be ready for review / merge. I haven't checked whether it passes those Servo tests, but it passes more Gecko tests without regressions so I'd prefer a followup investigation.

eqrion · 2017-05-11T17:44:46Z

@pyfisch I ran your test case with a Servo build with this patch applied and it seems like it passes all the test cases (except for gradients with transparency which is related to #1219).

The only iffy test case is 12, where it seems that Gecko/Blink are rendering it incorrectly according to the spec and we differ with this patch.

glennw · 2017-05-17T05:42:44Z

@bors-servo r+

Thanks! Sorry for the delay in reviewing this one.

bors-servo · 2017-05-17T05:42:44Z

📌 Commit b0af541 has been approved by glennw

bors-servo · 2017-05-17T05:42:47Z

⌛ Testing commit b0af541 with merge 997111a...


        Auto merge of #1189 - rlhunt:gradient/clamp-bounds, r=glennw

Handle gradients with multiple color stops at 0.0 or 1.0 When a clamped gradient has multiple color stops at 0.0, the last one will fill GradientData[0]. When a clamped gradient has multiple color stops at 1.0, the first one will fill GradientData[N-1]. Both of these conditions will cause the area outside of the gradient to not be filled with the correct clamped color, which is the first or last gradient color stop. Filling the first or last color entry of GradientData with the first or last color stop isn't exactly correct either because the first or last color stop entry is only needed in this case for the range outside of [0, 1]. This commit changes the GradientData so that the first color entry is used for offset < 0, the last color entry is used for offset > 1, and the color entries in between are for the range in between. The first and last color entries are then filled with the correct colors for those ranges.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1189)

bors-servo · 2017-05-17T06:12:15Z

☀️ Test successful - status-travis
Approved by: glennw
Pushing 997111a to master...


        Remove now unnecessary fix_gradient_stops function

Webrender solved the issue with servo/webrender#1189.


        Remove now unnecessary fix_gradient_stops function

Webrender solved the issue with servo/webrender#1189.


        Remove now unnecessary fix_gradient_stops function

Webrender solved the issue with servo/webrender#1189.


        Remove now unnecessary fix_gradient_stops function

Webrender solved the issue with servo/webrender#1189.


        Remove now unnecessary fix_gradient_stops function

Webrender solved the issue with servo/webrender#1189.

kvark reviewed May 2, 2017

View changes

eqrion force-pushed the eqrion:gradient/clamp-bounds branch from 00f1408 to e383899 May 3, 2017

Gankra reviewed May 3, 2017

View changes

eqrion force-pushed the eqrion:gradient/clamp-bounds branch from e383899 to eef3942 May 3, 2017

kvark changed the title ~~Handle gradients with multiple color stops at 0.0 or 1.0~~ [WIP] Handle gradients with multiple color stops at 0.0 or 1.0 May 3, 2017

eqrion force-pushed the eqrion:gradient/clamp-bounds branch from eef3942 to b0af541 May 4, 2017

eqrion changed the title ~~[WIP] Handle gradients with multiple color stops at 0.0 or 1.0~~ Handle gradients with multiple color stops at 0.0 or 1.0 May 4, 2017

pyfisch mentioned this pull request May 7, 2017

Improvements to gradients. servo/servo#16666

Merged

2 of 2 tasks complete

bors-servo merged commit b0af541 into servo:master May 17, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

eqrion deleted the eqrion:gradient/clamp-bounds branch May 17, 2017

pyfisch added a commit to pyfisch/servo that referenced this pull request Feb 12, 2018

Remove now unnecessary fix_gradient_stops function

52a7f3c

Webrender solved the issue with servo/webrender#1189.

pyfisch added a commit to pyfisch/servo that referenced this pull request Feb 15, 2018

Remove now unnecessary fix_gradient_stops function

3cb4b5f

Webrender solved the issue with servo/webrender#1189.

pyfisch added a commit to pyfisch/servo that referenced this pull request Feb 24, 2018

Remove now unnecessary fix_gradient_stops function

8d06111

Webrender solved the issue with servo/webrender#1189.

nupurbaghel added a commit to paavininanda/servo that referenced this pull request Mar 14, 2018

Remove now unnecessary fix_gradient_stops function

e9ec752

Webrender solved the issue with servo/webrender#1189.

pandusonu2 added a commit to pandusonu2/servo that referenced this pull request Mar 15, 2018

Remove now unnecessary fix_gradient_stops function

79ae6bd

Webrender solved the issue with servo/webrender#1189.

servo / webrender

Handle gradients with multiple color stops at 0.0 or 1.0 #1189

Handle gradients with multiple color stops at 0.0 or 1.0 #1189

eqrion commented May 2, 2017 •

edited by larsbergstrom

eqrion commented May 2, 2017

Gankra commented May 2, 2017

eqrion commented May 2, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 2, 2017

eqrion commented May 3, 2017

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 3, 2017

eqrion commented May 3, 2017

kvark commented May 3, 2017

eqrion commented May 4, 2017

glennw commented May 5, 2017

kvark commented May 5, 2017

eqrion commented May 5, 2017

kvark commented May 5, 2017

eqrion commented May 5, 2017

eqrion commented May 5, 2017

pyfisch commented May 6, 2017

glennw commented May 8, 2017

eqrion commented May 9, 2017

eqrion commented May 11, 2017

glennw commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 17, 2017

servo / webrender

Join GitHub today

Handle gradients with multiple color stops at 0.0 or 1.0 #1189

Handle gradients with multiple color stops at 0.0 or 1.0 #1189

Conversation

eqrion commented May 2, 2017 • edited by larsbergstrom

eqrion commented May 2, 2017

Gankra commented May 2, 2017

eqrion commented May 2, 2017

This comment has been minimized.

kvark May 2, 2017

This comment has been minimized.

eqrion May 3, 2017

This comment has been minimized.

kvark May 2, 2017

This comment has been minimized.

eqrion May 3, 2017

bors-servo commented May 2, 2017

eqrion commented May 3, 2017

This comment has been minimized.

Gankra May 3, 2017

This comment has been minimized.

eqrion May 3, 2017

bors-servo commented May 3, 2017

eqrion commented May 3, 2017

kvark commented May 3, 2017

eqrion commented May 4, 2017

glennw commented May 5, 2017

kvark commented May 5, 2017

eqrion commented May 5, 2017

kvark commented May 5, 2017

eqrion commented May 5, 2017

eqrion commented May 5, 2017

pyfisch commented May 6, 2017

glennw commented May 8, 2017

eqrion commented May 9, 2017

eqrion commented May 11, 2017

glennw commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 17, 2017

eqrion commented May 2, 2017 •

edited by larsbergstrom