Use the brush shaders for repeated images/gradient in some cases.

[nical]

Built on top of #2621, this PR makes use of the repetition support added in the brush shaders in the simple case (no tile spacing).

---

This change is 

[kvark]

minus a few small concerns

---

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

webrender/res/brush_image.glsl, line 150 at r1 (raw file):

    vUv.xy = mix(uv0, uv1, f) - min_uv;
    vUv.xy /= texture_size;
    vUv.xy *= repeat.xy;

shouldn't this be something like vUv.xy += repeat.xy * (max_uv-min_uv) / texture_size?

---

webrender/res/brush_radial_gradient.glsl, line 61 at r1 (raw file):

    vRepeatedSize.y *=  ratio_xy;

    vPos;

is that an accidental leftover?

---

webrender/res/brush_radial_gradient.glsl, line 80 at r1 (raw file):

#ifdef WR_FEATURE_ALPHA_PASS
    // Handle top and left inflated edges (see brush_image).
    vec2 local_pos = max(vPos, vec2(0.0));

This piece is sort of repeated 3 times now. Any plans to unify it? At the very least, could put it into an included module

---

Comments from Reviewable

[nical]

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

---

webrender/res/brush_image.glsl, line 150 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't this be something like vUv.xy += repeat.xy * (max_uv-min_uv) / texture_size?

it's really a multiplication. Basically multiplying by repeat stretches the rectange, which is then chopped off by the modulo, whereas an addition would translate it.
Sorry I am not super good at explaining this type of thing without waving hands and drawing on pieces of papers, but we can chat about it on vidyo or something if you want to go into the details.

---

webrender/res/brush_radial_gradient.glsl, line 61 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is that an accidental leftover?

Oops!

---

webrender/res/brush_radial_gradient.glsl, line 80 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

This piece is sort of repeated 3 times now. Any plans to unify it? At the very least, could put it into an included module

Sorta. I don't think that we can use the same code for the image and gradient shaders, but we could look into factoring the code for the two gradient shaders (which are almost the same) when the dust settles.

---

Comments from Reviewable

[kvark]

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

---

webrender/res/brush_image.glsl, line 150 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

it's really a multiplication. Basically multiplying by repeat stretches the rectange, which is then chopped off by the modulo, whereas an addition would translate it.
Sorry I am not super good at explaining this type of thing without waving hands and drawing on pieces of papers, but we can chat about it on vidyo or something if you want to go into the details.

Ok, I think I understand now.

---

Comments from Reviewable

[kvark]

👍 Needs a try push and CI fixed.

[nical]

try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4f0e43b257cd3dfc718f5ea17c5dbfdbb3118c8 most failures should be fuzzed except one that seems to be pixel-snapping related. I am not sure why that is though, looking into it.

[nical]

The pixel-snapping issue on the try run is related to how gecko renders some types of gradients with borders:

In the image above, the transparent red shape is a border. There's a gradient rect in the middle and css mandates that we repeat the gradient under the border, so we setup a gradient rect with extra repetitions (it starts one repetition before the original rect in css and ends one repetition after), and set the local clip rect to clip out the gradient repetitions beyond the border (in this drawing the clip rect isn't represented but it would basically hide the areas not covered by the red border or the rectangle inside of it.

This works with gecko's regular painting path because each gradient repetition is pixel snapped individually, and somehow the pixel snapping is expected to match the way the border itself is snapped.
If we do gradient repetitions in the shader, then we have a problem because pixel snapping is applied at the level of the whole gradient item, and then the way the shader renders repetitions acts like a sort of pixel snapping (there is no blurriness between repetitions), but this repetition snapping doesn't necessarily line up with gecko's snapping applied to the border.

Bummer.

This whole pixel-snapping business is unfortunately affecting a lot of different things and not just this PR so it'd be great if we could come up with a coherent solution (that doesn't involve snapping each repetition on the CPU).

For the specific case of borders and gradients/images, I think that it can be fixed by having gecko manually generate nine gradient primitives so that we can snap them in a way that matches the border, Not sure how complicated this would be.

[nical]

We talked about this during the gfx meeting and decided that it was okay to just snap the stretch size in gecko, which means the error potentially accumulates up to large values when there are a lot of repetitions, but it seems that Firefox is the only browser trying to do this properly right now, so the accumulated error issue would end up matching what Chrome and Safari do.

[kvark]

okay to just snap the stretch size in gecko

That would also fix https://bugzilla.mozilla.org/show_bug.cgi?id=1401665 (relevant - #2615)

[mstange]

We talked about this during the gfx meeting and decided that it was okay to just snap the stretch size in gecko

We also decided that Gecko needs to tell WebRender the location of the most important repetition of the image, so that snapping can snap where it actually matters. Otherwise, as in the case shown above, WebRender will snap the top left repetition and end up with a misalignment for the center repetition, even though the center repetition is the most important one.

As for the API, we should probably replace the "stretch size" parameter with a "dest rect" parameter. That's the rectangle that will be snapped and which all repetitions will start from and be aligned with.

[nical]

The change I mentioned in my previous comment is quite impactful, and can't make it before the end of Firefox's soft freeze in may 7th. I propose to do the following in the next few days:

• land Support repetition in brush shaders #2621
• land this
• in the next webrender update, disable reftest layout/reftests/bugs/1291528.html

If the repetition+border issue is too noticeable I can make a quick wberender-only workaround on the gecko side (just round both the prim rect and stretch size when building the webrender commands).

I'll look into changing the repetition behavior in gecko globally, wait until after the end of the soft freeze to land it, and after that reenable the reftest.

@staktrace how does this sound to you?

Patch with most of the reftest annotations for the webrender update in https://bugzilla.mozilla.org/show_bug.cgi?id=1453935

[staktrace]

The soft freeze doesn't start until April 26 - will it take longer than that to make the change? If so then I guess your proposal seems reasonable enough.

[nical]

Oh I misread the email. I'm still keen on breaking the dependency chain because I don't know how long it will take me to figure out the snapping change for all of gecko and I am fed up with rebasing pull requests and landing a gazillion things at once when things finally line up.

[staktrace]

Ok, fair enough. I'm all in favour of landing stuff incrementally where possible, as you might have noticed with the trickle of async-scene-building stuff :)

[glennw]

Reviewed 11 of 11 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[glennw]

Generally looks good to me, there is one CI failure in:

REFTEST reftests/filters/blend-clipped.yaml == reftests/filters/blend-clipped.png
REFTEST TEST-UNEXPECTED-FAIL | reftests/filters/blend-clipped.yaml == reftests/filters/blend-clipped.png | image comparison, max difference: 42, number of differing pixels: 1158

I'm not sure if this is a bug or a reference image update, but we should be a bit careful with this one (it was causing lots of visual artifacts in Gecko when it was broken previously).

[nical]

I updated the reference for the blend-clipped test. The difference is in the bottom row of pixels on the shape which is now slightly lighter. This is most likely due to the change in the anti-aliasing (it's the reason for updating the other reftest references).
I'm not quite sure why there would be aa on a primitive that looks axis-aligned, but the aa was already there before. I also rebased the PR and I think something related to pixel-snapping clips landed recently so I might have to update the reference again, we'll see.

Just to be sure, what kind of breakage happened on this test and were symptomatic of many artifacts in gecko?

[glennw]

The breakage are caused by when the image shader was sampling outside the rendered rect of a render task that is clipped. The most obvious instances of this (at the time) were things like the animations on the buttons in the chrome.

[bors-servo]

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

[nical]

As far as I am concerned this is ready to land. Any objection?

[glennw]

I'm fine with landing this - although it probably makes sense to do a final try run first?

[bors-servo]

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

[nical]

Did another try push and the result haven't changed. @bors-servo r=glennw

[bors-servo]

📌 Commit 39afce5 has been approved by glennw

[bors-servo]

⌛ Trying commit 39afce5 with merge 2496ffe...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
State: approved=glennw try=True

[bors-servo]

⌛ Testing commit 39afce5 with merge 7e66d84...

[nical]

@bors-servo r=glennw

[bors-servo]

📌 Commit 6245557 has been approved by glennw

[bors-servo]

⌛ Testing commit 6245557 with merge 9c5ee50...

[bors-servo]

💔 Test failed - status-appveyor

[jrmuizel]

This failed windows CI because now we have it \o/ Sorry @nical 😀

[nical]

@bors-servo r=glennw

[bors-servo]

📌 Commit b92439c has been approved by glennw

[bors-servo]

⌛ Testing commit b92439c with merge 4440d85...

[bors-servo]

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

[staktrace]

@nical @glennw this PR seems to have introduced some reftest failures in Gecko. See https://bugzilla.mozilla.org/show_bug.cgi?id=1457241#c3 - some of them might be fuzzable but /layout/reftests/bugs/1291528.html at least seems to have a visible difference.