Pass dot / dash locations to clip shader in the VAO

[mrobinson]

Instead of loading dot and dash positions into the GPU cache, send them
as VAO arguments. This avoids exceeding the maximum GPU cache block
size with very large border corners with hundreds or thousands of dot /
dashes.

This fixes the first panic from #2469, though performance is still
abysmal. Later changes can address performance issues.

---

This change is 

[mrobinson]

@glennw I'd love to get your feedback on my approach here. In particular I'm curious if I should be doing more to discards dots and dashes in the vertex shader. I'm also curious if my method of modifying the ClipMaskInstance data structure using GLSL preprocessor directives makes sense or if there is a more elegant method. I've decided to use a single data structure (a data blob like primitives use) for both dots and dashes. This seems like it would give a minor improvement to batching and prevent having to compile separate shaders for dots and dashes.

[kvark]

I can't comment on correctness, but I can on the general approach. I like where it's going, and I think we can go even further than that (perhaps, as a follow-up).

The idea is that maybe we don't even need the per-instance data for dots/dashes (and image tiles, for the matter). Perhaps, we could compute all the parameters in vertex shaders based on the vertex and instance ID.

For ellipses, it appears that we can efficiently compute the path across the curve given the E function. Since it doesn't depend on ellipse size, we could just bake it into a shared texture (say, 1024x1) that we linearly sample in VS.

[glennw]

Yup, this seems like a good general approach to me (I haven't looked in super detail).

I'm not sure that we need any of the pre-processor changes you mentioned - each clip shader will know the exact vertex format / kind it's reading (e.g. clip_dot vs clip_rect shader), so it's probably fine to define a GLSL type for each of those?

I think we do need to do some clipping work for dots / dashes. Since dots and dashes are drawn as geometry, it's possible they may extend outside the task rect of the clip mask, I think.

Ideally we can do this in the VS (cull the entire dot if it's outside the task rect), but I think we'll probably need a FS clip as well, to ensure that partial dots don't write outside the task rect. Another option might be to just use a scissor rect (and disable batching) on clip masks that have dots / dashes, since they are quite rare. This wouldn't be ideal, but might be fine for a first pass. The case where we will actually need clipping for this is quite rare, but I'm fairly sure it can occur - e.g. if a border with dots has a clip chain that results in part of the border being removed?

I like the idea of @kvark 's suggestion - but I'm a little wary of it. As we discussed previously, the existing Gecko code for dot / dash placement is absolutely massive. For now, we're assuming we can get away with a fairly simple placement algorithm, which would be fine to evaluate in a VS / texture, but if we end up needing to do something more complex, that won't be feasible.

[bors-servo]

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

[bors-servo]

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

[mrobinson]

@glennw @kvark Thanks for looking at the PR. I've updated it to improve clipping in the fragment shader. Now the mask rect is clamped to the task rect, which I think properly handles partial dots. A good follow-up for this version would be to produce vertices representing a tight bounding rectangle of the dot or dash. I'm working on that now, but this version seems to fix the issues raised in the bug while maintaining the performance of the old approach.

As far as the shader goes, the dot/dash shader shares almost every input variable with the normal clip mask shader, so I used the GLSL preprocessor to reuse the ClipMaskInstance data structure from clip_shared.gllsl. I'm happy to remove this and just duplicate the data structure, but I think we'll still need to use the preprocessor (in order to adjust the list of per-vertex input arguments to the shader) or duplicate some code (distance_to_line if we don't include clip_shared.glsl in the dot/dash shader).

[bors-servo]

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

[mrobinson]

This patch is ready to go, except I'm seeing a timeout during the crashtests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f61787a13f729be243398b316548031ad5cbdb3&selectedJob=174110305

When I run these tests locally, another crashtest times out, both with and without my changes. I'm a bit stumped what is causing this behavior.

[mrobinson]

I'm still having trouble debugging the crashtest timeout (though the Gecko tree is closed). I'm hoping to make some progress once it reopens.

[mrobinson]

@glennw I was finally able to get a passing try run for Gecko (modulo a few intermittent failures):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38bf3c0a42ab4ffb0b520f1325076668be37bb7d&selectedJob=174545800

I had to make some changes in order to make this happen:

• I reverted some of the cleaning up I did around BorderCornerClipSource initialization. This wasn't strictly necessary, though it made the code more efficient. This can be introduced gradually in order to narrow down what was causing the timeouts during the crash tests.
• I had to remove the shader clipping. This is important for performance, but I verified that the old approach was just as bad. Would it be okay to get this landed and then tackle clipping. I'm thinking it might take a couple days to figure out what is causing the fuzziness and I think it would help to get this cleared away first.

Thanks!

[mrobinson]

Looks like I spoke too soon. This does seem to change the details of the fuzziness (an already very fuzzy test) for layout/reftests/css-break/box-decoration-break-with-inset-box-shadow-1.html. It seems reasonable that we can just update the fuzz for this one.

[glennw]

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[glennw]

Looks good to me, once we get sign-off from @staktrace on the fuzziness change.

[staktrace]

I'm ok with the box-decoration-break-with-inset-box-shadow-1.html fuzz since that's just increasing pre-existing fuzziness by a little bit. But the position-sticky/inline-4.html fuzz is new, and has a user-visible difference (one of the dots is off). Is this expected, or is there something wrong with the test, or something else?

[bors-servo]

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

[mrobinson]

@staktrace Sorry, it seems the position-sticky/inline-4.html failure came in later and I thought I had fixed it. It's really mysterious, because it's a Windows only failure. These changes should not change at all the behavior of the shaders. I'll continue investigating, but this is incredibly weird.

[mrobinson]

Okay. After a lot of banging my head against this issue, I think I have figured out what was going on! The first versions of this PR changed the alignment of the VAO for dots and dashes. When I restored the unused element of the ClipMaskInstance the reference test failures disappeared. This also makes the diff much smaller. Here is the latest Gecko try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d8ae92639770c3bd37a8f776742a8ad44b3c862&selectedJob=175108604

I only see two seemingly unrelated intermittent failures.

[mrobinson]

r? @glennw (once again for the new branch)

[glennw]

Reviewed 6 of 7 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

webrender/res/cs_clip_border.glsl, line 52 at r2 (raw file):

};

BorderClipDash fetch_border_clip_dash(ivec2 address, int segment) {

We can remove segment as a param from here.

---

webrender/res/cs_clip_border.glsl, line 61 at r2 (raw file):

};

BorderClipDot fetch_border_clip_dot(ivec2 address, int segment) {

Same

---

webrender/src/batch.rs, line 1788 at r2 (raw file):

                            self.borders.push(ClipMaskBorderCornerDotDash {
                                clip_mask_instance: ClipMaskInstance {
                                    segment: 1,

Does segment get used here at all, or could we just leave it as the base instance?

---

Comments from Reviewable

[glennw]

A couple of minor nits, r=me after those are addressed.

[mrobinson]

Thanks for the review!

---

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

---

webrender/res/cs_clip_border.glsl, line 52 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

We can remove segment as a param from here.

Okay. I'll fix this.

---

webrender/src/batch.rs, line 1788 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

Does segment get used here at all, or could we just leave it as the base instance?

The shader understands the 0 segment as the clear, so this just needs to be != 0. I can add a comment here.

---

Comments from Reviewable

[mrobinson]

@bors-servo r=glennw

[bors-servo]

📌 Commit 429cc01 has been approved by glennw

[bors-servo]

⌛ Testing commit 429cc01 with merge 0758a80...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 0758a80 to master...