1615614 - brush_conic_gradient.glsl doesn't compile on GLES

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jamie Nicol [:jnicol]]

When compiling the conic gradient shader on gles we run in to the following error:

error: could not implicitly convert operands to arithmetic operator

complaining about this line. Changing the "2" on this line and the next to a "2.0" seems to satisfy it.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Why didn't we catch this in CI?

Comment 2

[Jamie Nicol [:jnicol]]

Do we have a test which compiles all shaders? If so, I don't think we don't run it on android. If I run with the precompile all shaders flag there are texture_rect compilation errors too (it's not supported on gles) which I presume have failed for a while. I'll file a bug for that too.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jamie Nicol [:jnicol] from comment #2)

Do we have a test which compiles all shaders? If so, I don't think we don't run it on android. If I run with the precompile all shaders flag there are texture_rect compilation errors too (it's not supported on gles) which I presume have failed for a while. I'll file a bug for that too.

The angle shader https://github.com/servo/webrender/blob/master/webrender/tests/angle_shader_validation.rs should be running on them all. I guess it doesn't catch this issue?

Comment 4

[Kelsey Gilbert [:jgilbert]]

We should checking+asserting link status in debug builds, if we're not already.
Additionally, it doesn't look like angle_shader_validation checks link status, only compile validation. In actual spec-compliant GL/GLSL, compile status is a hint, and can always return success, whereas link status is the only reliable place to check, even for basic compilation problems:
https://hackmd.io/lvtOckAtSrmIpZAwgtXptw?view#Don%E2%80%99t-check-shader-compile-status-until-linking-fails

Comment 5

[Jamie Nicol [:jnicol]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

(In reply to Jamie Nicol [:jnicol] from comment #2)

Do we have a test which compiles all shaders? If so, I don't think we don't run it on android. If I run with the precompile all shaders flag there are texture_rect compilation errors too (it's not supported on gles) which I presume have failed for a while. I'll file a bug for that too.

The angle shader https://github.com/servo/webrender/blob/master/webrender/tests/angle_shader_validation.rs should be running on them all. I guess it doesn't catch this issue?

The conic gradient shader isn't defined in that file. The joys of having things defined in multiple places...

Furthermore, we added a few conic gradient yaml files to the wrench reftests, but didn't add them to the reftest.list (nor add reference images/yamls). Hence why we don't compile this shader during the wrench reftests.

Tim, would you like to try fixing up these issues? Ping me on matrix if you need any help! :)

Comment 6

[Tim Nguyen :ntim]

(In reply to Jamie Nicol [:jnicol] from comment #5)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

(In reply to Jamie Nicol [:jnicol] from comment #2)

Do we have a test which compiles all shaders? If so, I don't think we don't run it on android. If I run with the precompile all shaders flag there are texture_rect compilation errors too (it's not supported on gles) which I presume have failed for a while. I'll file a bug for that too.

The angle shader https://github.com/servo/webrender/blob/master/webrender/tests/angle_shader_validation.rs should be running on them all. I guess it doesn't catch this issue?

The conic gradient shader isn't defined in that file. The joys of having things defined in multiple places...

Furthermore, we added a few conic gradient yaml files to the wrench reftests, but didn't add them to the reftest.list (nor add reference images/yamls). Hence why we don't compile this shader during the wrench reftests.

I was planning to leave this for a different bug where I would add even more testcases, since I needed to generate reference images for the test, which I wasn't able to do yet since I need Linux (there's a bug on macOS regarding color spaces). I filed bug 1616106 for this.

Happy to fix the other issues though!

Tim, would you like to try fixing up these issues? Ping me on matrix if you need any help! :)

Thanks! Will do.

Comment 7

[Kelsey Gilbert [:jgilbert]]

We must also assert that program linking succeeded. That's a much more reliable safety check, and would have caught this immediately.

Comment 8

[Tim Nguyen :ntim]

Attachment: Bug 1615614 - Make brush_conic_gradient.glsl compile on GLES.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc4f43236bcf
Make brush_conic_gradient.glsl compile on GLES. r=gw

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/dc4f43236bcf