Support border corners with differing styles. #1199

glennw · 2017-05-04T03:30:15Z

This handles all border corners styles except those with
dashed and/or dotted styles. Those require changes to the
border clip mask shader, which will be done in a follow up.

The basic idea is to draw the corner twice, once for each style
and mask out the other side of the corner. This results in more
pixels being drawn than necessary, and can sometimes result in the
AA between the corner segments being slightly incorrect. Those
issues can be fixed at a later time.

This change is

glennw · 2017-05-04T03:30:21Z

r? @kvark

kvark

Looks good!
Just a few minor things, if you want perfection ;)
Anyhow, r=me

kvark · 2017-05-04T19:21:35Z

webrender/res/ps_border_corner.vs.glsl

@@ -95,14 +123,16 @@ void main(void) {
            color1 = border.colors[1];
            vClipCenter = corners.tl_outer + border.radii[0].xy;
            vClipSign = vec2(1.0);
-            set_radii(border.style.x,
+            style = select_style(prim.user_data.x, border.style.y, border.style.x);


nit: we could have select_style accepting vec2 styles

kvark · 2017-05-04T19:21:35Z

webrender/res/ps_border_corner.vs.glsl

                      border.radii[0].zw,
                      border.widths.zy,
                      adjusted_widths.zy);
            set_edge_line(border.widths.zy,
                          corners.tr_outer,
                          vec2(-1.0, 1.0));
-            style = int(border.style.y);
            edge_distances = vec4(p1.x - adjusted_widths.z,
                                  p0.y + adjusted_widths.y,
                                  p1.x - border.widths.z + adjusted_widths.z,


this line could use some inv_adjusted_widths too

kvark · 2017-05-04T19:21:35Z

webrender/res/ps_border_corner.vs.glsl

                      border.radii[1].xy,
                      border.widths.zw,
                      adjusted_widths.zw);
            set_edge_line(border.widths.zw,
                          corners.br_outer,
                          vec2(-1.0, -1.0));
-            style = int(border.style.z);
            edge_distances = vec4(p1.x - adjusted_widths.z,
                                  p1.y - adjusted_widths.w,
                                  p1.x - border.widths.z + adjusted_widths.z,


similarly, this could use inv_adjusted_widths, even in a vectorized (short) form

kvark · 2017-05-04T19:21:35Z

webrender/src/tiling.rs

-                                    batch.add_instance(base_instance.build(border_segment, 0, 0));
+                                for (i, instance_kind) in border_cpu.corner_instances.iter().enumerate() {
+                                    let sub_index = i as i32;
+                                    match *instance_kind {


nit: this match could be simplified if it only returned &[BorderCornerSide]


        Support border corners with differing styles.

This handles all border corners styles except those with dashed and/or dotted styles. Those require changes to the border clip mask shader, which will be done in a follow up. The basic idea is to draw the corner twice, once for each style and mask out the other side of the corner. This results in more pixels being drawn than necessary, and can sometimes result in the AA between the corner segments being slightly incorrect. Those issues can be fixed at a later time.

glennw · 2017-05-04T20:22:02Z

@kvark Thanks - fixed the select_style function - will fix the other nits up as follow ups to tidy that shader code up :)

@bors-servo r=kvark

bors-servo · 2017-05-04T20:22:03Z

📌 Commit c1efd22 has been approved by kvark

bors-servo · 2017-05-04T20:34:31Z

⌛ Testing commit c1efd22 with merge e2e07a0...


        Auto merge of #1199 - glennw:corners5, r=kvark

Support border corners with differing styles. This handles all border corners styles except those with dashed and/or dotted styles. Those require changes to the border clip mask shader, which will be done in a follow up. The basic idea is to draw the corner twice, once for each style and mask out the other side of the corner. This results in more pixels being drawn than necessary, and can sometimes result in the AA between the corner segments being slightly incorrect. Those issues can be fixed at a later time.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1199)

bors-servo · 2017-05-04T21:19:20Z

☀️ Test successful - status-travis
Approved by: kvark
Pushing e2e07a0 to master...

staktrace · 2017-05-11T17:57:27Z

@glennw I think this patch does actually trigger all those test failures in the gecko try push we were talking about yesterday (tracked in bug 1363683). It looks like possibly an infinite loop somewhere inside the render function - or at least something that causes the render function take upwards of 5 minutes to run, at which point the timeout kills the test.

I was able to reproduce locally on the gecko layout/reftests/abs-pos/table-3.html reftest. I interrupted with gdb while it was inside the render function and got a backtrace: https://gist.github.com/staktrace/22ea886d20afb334c3b80920ec7c9d0b

kvark · 2017-05-11T18:43:06Z

@staktrace that is worrisome... I re-inspected the code and didn't see a potential cause of endless loops. Perhaps, the hang/delay is coming from the driver or the hardware doesn't like the new code for some reason. I think we should back the PR out and try reproducing your case on Gecko locally.

glennw · 2017-05-11T20:22:35Z

@staktrace @kvark Hmm, that backtrace does indeed suggest the shader change is causing some kind of issue inside llvmpipe. I'm guessing that the shader change has triggered a bug in mesa / llvmpipe :(

glennw · 2017-05-11T20:35:58Z

@staktrace @kvark I will try to reproduce this locally on llvmpipe today.

glennw · 2017-05-11T20:36:58Z

I imagine it will just be some tweak to the shader needed to avoid triggering the bug in mesa / llvm - hopefully.

glennw · 2017-05-12T04:35:59Z

@staktrace @kvark

I reproduced this locally. It appears to be a bug in the shader compiler in mesa / llvm. I've opened a PR here #1242 which works around the hang by slightly tweaking the structure of the shader.

It shouldn't have any functional effect, but fixes the problem for me. Is the simplest way to test this just to get it merged and try to do an update?

staktrace · 2017-05-12T14:38:53Z

If the PR can be applied (without too much rebasing) directly onto the copy of webrender in mozilla-central or graphics, that would be the best thing to push to try - that way we start from a known good revision and just test this change in isolation. However if it depends on other things that landed since the last WR update into gecko, or is otherwise problematic to rebase, then it's probably easiest to just get it merged and my next simulated WR update will test it.

glennw force-pushed the glennw:corners5 branch from d46988a to c168641 May 4, 2017

glennw mentioned this pull request May 4, 2017

Remove old border code path. #1200

Merged

kvark approved these changes May 4, 2017

View changes

glennw force-pushed the glennw:corners5 branch from c168641 to c1efd22 May 4, 2017

bors-servo merged commit c1efd22 into servo:master May 4, 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

kvark mentioned this pull request May 8, 2017

WR update: preserve-3d support servo/servo#16775

Closed

3 of 5 tasks complete

kvark mentioned this pull request May 11, 2017

[WIP] Revert "Support border corners with differing styles" #1239

Closed

glennw deleted the glennw:corners5 branch May 12, 2017

servo / webrender

Support border corners with differing styles. #1199

Support border corners with differing styles. #1199

glennw commented May 4, 2017 •

edited by larsbergstrom

glennw commented May 4, 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.

glennw commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

staktrace commented May 11, 2017

kvark commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 12, 2017 •

edited

staktrace commented May 12, 2017

servo / webrender

Join GitHub today

Support border corners with differing styles. #1199

Support border corners with differing styles. #1199

Conversation

glennw commented May 4, 2017 • edited by larsbergstrom

glennw commented May 4, 2017

kvark left a comment

This comment has been minimized.

kvark May 4, 2017

This comment has been minimized.

kvark May 4, 2017

This comment has been minimized.

kvark May 4, 2017

This comment has been minimized.

kvark May 4, 2017

glennw commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

staktrace commented May 11, 2017

kvark commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 11, 2017

glennw commented May 12, 2017 • edited

staktrace commented May 12, 2017

glennw commented May 4, 2017 •

edited by larsbergstrom

glennw commented May 12, 2017 •

edited