Support double, groove, ridge borders in new border brush path.

[gw3583]

This change is 

[gw3583]

Supersedes #2779, so closing that.

[gw3583]

This adds support for double, ridge and groove border styles to run through the new border brush path.

I also moved a few GLSL functions around. This allows us to use them in cache shaders without including prim_shared.glsl. The prim_shared file declares a few unused interpolators.

Avoiding that means we won't run into any shader compiler issues as we add the extra interpolators to this shader needed for dashing / dotting styles in the future.

r? @kvark or @mrobinson

[gw3583]

Try run looks good - there are the expected (fuzziness required) failures from the previous border PR, and a number of new PASS results in R5.

[gw3583]

With actual link to try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b332b7d3f4653cb6e3f138f7e7e5131e40134b&selectedJob=180518780

[gw3583]

Added a follow up commit - this doesn't contain any functional changes, but is prep work for supporting dashing / dotting as border brushes (and it's a small optimization too).

[kvark]

Review status: all files reviewed at latest revision, all discussions resolved.

---

webrender/res/cs_border_segment.glsl, line 9 at r2 (raw file):

// For edges, the colors are the same. For corners, these
// are the colors of each edge making up the corner.
flat varying vec4 vColor0[2];

heh, I wonder if we hit any driver/angle/SPIRV-Cross issues with those arrays. And if we are to use them, wouldn't it be easier to just go vColor[4]?

---

webrender/res/cs_border_segment.glsl, line 12 at r2 (raw file):

flat varying vec4 vColor1[2];

// A point + tangent defining the line where the edge

bonus points for comments! 👍 💯

---

webrender/res/cs_border_segment.glsl, line 106 at r2 (raw file):

    vec2 clip_sign = 1.0 - 2.0 * outer_scale;

    // Determine which side of the edge transition this

what is the "edge transition" here?

---

webrender/res/cs_border_segment.glsl, line 160 at r2 (raw file):

    switch (style1) {
        case BORDER_STYLE_GROOVE:
            vColor1[0] = mod_color(aColor1, f.x);

can we move this switch out in a helper method?

---

webrender/res/cs_border_segment.glsl, line 173 at r2 (raw file):

    }

    vClipCenter_Sign = vec4(outer + clip_sign * aRadii, clip_sign);;

nit: double ";" at the end

---

webrender/res/cs_border_segment.glsl, line 182 at r2 (raw file):

    switch (segment) {
        case SEGMENT_TOP_LEFT:
            edge_reference = outer;

can we combine this switch with the one earlier?

---

webrender/res/cs_border_segment.glsl, line 222 at r2 (raw file):

            float d_radii_b = distance_to_ellipse(
                clip_relative_pos,
                clip_radii.xy - 2.0 * vPartialWidths.xy,

could you explain this bit?

---

webrender/res/cs_border_segment.glsl, line 245 at r2 (raw file):

                    break;
                case SEGMENT_TOP_RIGHT:
                    c0 = mix(color[1], color[0], mix_factor);

perhaps, we can take only the "mix_factor" away from this switch and then mix uniformly afterwards?

---

webrender/res/cs_border_segment.glsl, line 280 at r2 (raw file):

            float d0 = -1.0;
            float d1 = -1.0;
            if (vPartialWidths[edge_axis] > 1.0) {

I'm worried a bit about using the vector indexing here. We need to check the HW requirements, also check if this doesn't affect shader performance (e.g. by throwing it under GPU ShaderAnalyzer).

---

webrender/res/cs_border_segment.glsl, line 313 at r2 (raw file):

    int segment = vConfig.x;
    int style0 = vConfig.y & 0xffff;
    int style1 = vConfig.y >> 16;

nit: could do ivec2 style = the same way for consistency

---

webrender/res/cs_border_segment.glsl, line 351 at r2 (raw file):

        );
    } else {
        color0 = evaluate_color_for_style_in_edge(

The branching looks fairly heavy for the pixel shader, must be blowing up the register use. Not saying we should address this now, but surely keep an eye on border benchmarks.

---

webrender/src/border.rs, line 1402 at r2 (raw file):

    segment: BorderSegment,
    edge_flags: EdgeAaSegmentMask,
    border_segments: &mut Vec<BorderSegmentInfo>,

any reason for not calling it just BorderSegment?

---

Comments from Reviewable

[gw3583]

Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

webrender/res/cs_border_segment.glsl, line 9 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

heh, I wonder if we hit any driver/angle/SPIRV-Cross issues with those arrays. And if we are to use them, wouldn't it be easier to just go vColor[4]?

I think having them as two separate fields makes sense conceptually - each field is the 2 colors that make up each edge. Not sure if we'll see any shader compiler bugs :)

---

webrender/res/cs_border_segment.glsl, line 12 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

bonus points for comments! 👍 💯

Done.

---

webrender/res/cs_border_segment.glsl, line 106 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what is the "edge transition" here?

Comment was out of date - updated.

---

webrender/res/cs_border_segment.glsl, line 160 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we move this switch out in a helper method?

Done.

---

webrender/res/cs_border_segment.glsl, line 182 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we combine this switch with the one earlier?

Done.

---

webrender/res/cs_border_segment.glsl, line 222 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could you explain this bit?

Yup, added a comment to describe what's going on here.

---

webrender/res/cs_border_segment.glsl, line 245 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

perhaps, we can take only the "mix_factor" away from this switch and then mix uniformly afterwards?

It's not clear to me what this would look like - could you expand on that?

---

webrender/res/cs_border_segment.glsl, line 280 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'm worried a bit about using the vector indexing here. We need to check the HW requirements, also check if this doesn't affect shader performance (e.g. by throwing it under GPU ShaderAnalyzer).

I've used this a lot in the past and not hit any problems with it so far (it's also been in the GLSL specification since at least 2009 - see section 5.5 of https://www.khronos.org/files/opengles_shading_language.pdf), so hopefully we won't run into any problems with it. But you're probably right - we may well find some driver / gpu combination that doesn't like it.

---

webrender/res/cs_border_segment.glsl, line 351 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

The branching looks fairly heavy for the pixel shader, must be blowing up the register use. Not saying we should address this now, but surely keep an eye on border benchmarks.

Yep, it's definitely an expensive shader. Luckily it's not cached in the texture/render task cache, which should mitigate this. If we do ever see it in profiles, an easy fix is probably to have a "simple" border shader that handles the 99% common cases, and a complex border shader for the unlikely border styles.

---

webrender/src/border.rs, line 1402 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

any reason for not calling it just BorderSegment?

We have a BorderSegment enum already, unfortunately. It should be possible to tidy up a lot of the naming once I remove all the old border code (which I will be doing once I port dash + dot styles).

---

Comments from Reviewable

[kvark]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

webrender/res/cs_border_segment.glsl, line 245 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

It's not clear to me what this would look like - could you expand on that?

Sure:

float swizzled_factor;
switch (segment) {
  case SEGMENT_TOP_LEFT: swizzled_factor = 0.0; break;
  case SEGMENT_TOP_RIGHT: swizzled_factor = mix_factor;
  case SEGMENT_BOTTOM_RIGHT: swizzled_factor = 1.0; break;
  case SEGMENT_BOTTOM_LEFT: swizzled_factor = 1.0 - mix_factor; break;
  default: swizzled_factor = 0.0;
};
vec4 c0 = mix(color[1], color[0], swizzled_factor);
vec4 c1 = mix(color[0], color[1], swizzled_factor);

---

Comments from Reviewable

[gw3583]

Review status: 11 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

webrender/res/cs_border_segment.glsl, line 245 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Sure:

float swizzled_factor;
switch (segment) {
  case SEGMENT_TOP_LEFT: swizzled_factor = 0.0; break;
  case SEGMENT_TOP_RIGHT: swizzled_factor = mix_factor;
  case SEGMENT_BOTTOM_RIGHT: swizzled_factor = 1.0; break;
  case SEGMENT_BOTTOM_LEFT: swizzled_factor = 1.0 - mix_factor; break;
  default: swizzled_factor = 0.0;
};
vec4 c0 = mix(color[1], color[0], swizzled_factor);
vec4 c1 = mix(color[0], color[1], swizzled_factor);

Ah, I thought you meant to somehow combine it with the mix below. Updated now :)

---

Comments from Reviewable

[kvark]

thank you!
@bors-servo r+

[bors-servo]

📌 Commit 43c4743 has been approved by kvark

[bors-servo]

⌛ Testing commit 43c4743 with merge ffd9596...

[bors-servo]

💔 Test failed - status-taskcluster

[kvark]

@bors-servo retry

[bors-servo]

⌛ Testing commit 43c4743 with merge 52373cb...

[bors-servo]

💔 Test failed - status-taskcluster

[gw3583]

@bors-servo retry

[bors-servo]

⌛ Testing commit 43c4743 with merge f198c23...

[gw3583]

Rebased to include rawtest fixes.

@bors-servo r=kvark

[bors-servo]

📌 Commit 9985a99 has been approved by kvark

[bors-servo]

⌛ Testing commit 9985a99 with merge 72c09b7...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 72c09b7 to master...