Fix an issue on some nvidia cards with texelFetch.

[gw3583]

In the do_clip() function, texelFetch is sometimes returning
invalid results on some nVidia GPUs.

To work around that, use UVs as normalized float coordinates
and fetch via texture(). This fixes the problems on nVidia
cards.

This exposed a bug in how we handle vertex snapping with clips.
Previously, we just cast the float UVs to an int before passing
to texelFetch. Now, pass in the snap offset and apply that to
the UVs for the clip mask.

---

This change is 

[gw3583]

r? @kvark

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1491911

Surprisingly enough, this doesn't seem to have broken anything on a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1b06cde8bb6406aae65ddd0d29776ebfd69ab35

It's still pending but so far no changes on Linux and a new PASS in OSX.

[gw3583]

Yep, try run looks good.

[nical]

I added an entry in the driver issues page. The entry is a bit hand-wavey and it'd be sad if we have to give up on texelFetch. Don't hesitate to add more information.

[kvark]

@nical that entry link is broken, FYI

[kvark]

snap_offset needs to be multiplied by world_pos.w to be homogeneous

[gw3583]

Fixed

[kvark]

this is really really sad 😿

[gw3583]

Yes 😢

[gw3583]

@kvark Thanks for the review, this is ready to go now, I think.

[kvark]

, one suggestion left on the table

Reviewed 8 of 9 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gw3583)

---

webrender/res/prim_shared.glsl, line 268 at r2 (raw file):

    //           unconditionally.
    mask_uv = clamp(mask_uv, vClipMaskUvSampleBounds.xy, vClipMaskUvSampleBounds.zw);
    return texture(sCacheA8, vec3(mask_uv, vClipMaskUv.z)).r;

we can at least use the LOD-selecting variant

[gw3583]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gw3583)

---

webrender/res/prim_shared.glsl, line 268 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we can at least use the LOD-selecting variant

Does that make any difference, when we know the sCacheA8 texture never has mips? Maybe it's a theoretical performance hint to the driver?

[gw3583]

@bors-servo r=kvark

[bors-servo]

📌 Commit 171be5b has been approved by kvark

[bors-servo]

⌛ Testing commit 171be5b with merge 01ec3e7...

[bors-servo]

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

[kvark]

@jrmuizel found out that this PR fixes a few reftests on Gecko side that aren't related to NVidia, and are reproducible on both OSX and Windows. It means that we weren't doing the texelFetch correctly in the first place, and it's not (or at least, not only) the driver bug in play.

Re-reading the patch, I noticed a few things. First, the new code doesn't sample the same texels: since we are using the nearest sampling now, we are rounding up the sampling coordinate (e.g. 0.7 -> 1.0).

Secondly, the old code wasn't properly clamping the upper sampling bounds:

     bvec4 inside = lessThanEqual(
         vec4(vClipMaskUvBounds.xy, mask_uv),
vec4(mask_uv, vClipMaskUvBounds.zw));

Notice the second part of comparison - it would pass even if mask_uv == vClipMaskUvBounds.zw. If that is an integer number, we'd fetch a texel outside of the permitted area.

With this in mind, there is a hope that we aren't really seeing a driver bug here. I'll prepare a patch to test things out.

[kvark]

@gw3583 I missed this one:

Does that make any difference, when we know the sCacheA8 texture never has mips? Maybe it's a theoretical performance hint to the driver?

The texture doesn't have mips, right, so the texture unit knows it doesn't need to load them at run-time. But the shader compiler doesn't know that, and can't possibly (since the texture can be changing at run-time). In modern GPUs, the LOD selection appears to be computed with shader instructions, according to Humus:

Since gradient are needed by the texture units
anyway, it made sense in the past to let them handle it; however, now that GCN has a
generic lane swizzle, the ALUs has all the tools to do the work itself, so now it’s done in the
ALUs again.

It would be great to confirm with something like a shader analyzer, but the reasoning is sound to me, and I see no downside to use textureLod whenever we can anyway.

Also, interesting (distantly related) tip from a similar document:

GCN Performance Tip 23: GetDimensions() is a TEX instruction; prefer storing
texture dimensions in a Constant Buffer if TEX-bound.
Notes: The use of a TEX instruction can lead to lower shader performance if the shader
is TEX instructions - limited. It m ay also be subject to additional latency. It is therefore preferred to store the desired texture dimensions into a constant buffe