use switch statement for text snapping decision

[srijs]

Fixes #3538.

@lsalzman this implements your suggestion of switching from an if to a switch statement, and it really fixes the issue for some puzzling reason! I'd love to put a better description of the issue into the commit message, but this behaviour thoroughly confuses me. Can you offer more insight into why this particular change alleviates the problem?

---

This change is 

[nical]

Looks like this should be documented in the driver issues wiki page.

[srijs]

It sounds very similar to https://github.com/servo/webrender/wiki/Driver-issues#2864---mac-glsl-compiler-bug-with-integer-comparisons, though not quite the same?

[staktrace]

Thanks for the patch! https://bugzilla.mozilla.org/show_bug.cgi?id=1525032 will merge this to m-c and then it will get synced back to webrender.

[nical]

It sounds very similar to https://github.com/servo/webrender/wiki/Driver-issues#2864---mac-glsl-compiler-bug-with-integer-comparisons, though not quite the same?

Looks very similar indeed, I added https://github.com/servo/webrender/wiki/Driver-issues#2864---mac-glsl-compiler-bug-with-integer-comparisons right next to it, not sure whether they are the same but it's good to keep a history of each workaround.

[kvark]

What's bothering me here is that local_transform isn't always initialized. Maybe we are just hitting the UB in the shader because of that, and turning if into switch just affects what garbage turns out to be placed on local_transform space, thus luckily improving the results?

[lsalzman]

local_transform is only ever used with WR_FEATURE_GLYPH_TRANSFORM, which is only ever used with RasterSpace::Screen

[kvark]

@lsalzman so you are saying that raster_space has to be SCREEN if the WR_FEATURE_GLYPH_TRANSFORM is enabled? It's unfortunate to leave the shader behave UB when this condition isn't fulfilled by the Rust side for some reason.

[srijs]

FWIW, initializing local_transform to the identity matrix on line 74 also fixes the issue on my machine. Happy to push that change instead if you want to go that way.

[lsalzman]

That's fairly surprising that even though local_transform is never used at all that it caused that issue. In any case, keep the switch() I would suggest, but also add in the initialization of mat2 local_transform = mat2(1.0) just to make the compiler supremely happy, I suppose.

…

On Tue, Feb 5, 2019 at 4:58 PM Sam Rijs ***@***.***> wrote: FWIW, initializing local_transform to the identity matrix on line 74 also fixes the issue on my machine. Happy to push that change instead if you want to go that way. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3540 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY60deb8B3J8yz0dxtwXzXNusYXGiDWks5vKf6TgaJpZM4agqA9> .

[kvark]

@lsalzman are you 100% sure that local_transform wasn't used in this case?
@srijs would you mind setting it to something crazy and seeing if it affects anything?

I'm not a fan of switch fix because that's something we can't explain. And the hypothesis that local_transform is used would provide a much cleaner explanation to the root of the problem :)

[lsalzman]

Since this is technically an enum value, the switch is a more natural expression of the intent of the code anyway, so there is no harm in just leaving the switch.

[srijs]

I've pushed the additional commit onto the branch.

@kvark using a bogus initializer for local_transform does not have any effect in my case, but given that I only use screen raster spaces, I wouldn't expect to hit the branch where local_transform is used uninitialized?

[lsalzman]

WR_FEATURE_GLYPH_TRANSFORM will only get triggered if there is a font transform, which can only happen in RasterSpace::Screen. If RasterSpace::Local is used, then the font transform is identity, and WR_FEATURE_GLYPH_TRANSFORM will never be used. So the only possible way for the shader enum to not actually be RASTER_SCREEN is if there was either a driver bug with the uniform value, or a bug in our uniform code. However, given that this problem only manifests on one very limited configuration on macOS and only on specific hardware, and nowhere else, I think this is just a driver bug.

[staktrace]

Sorry about closing the PR while discussion is still ongoing but the syncing process assumes that once the PR is approved it won't be modified. If the additional commit needs landing please open a new PR with that patch. Thanks!