Closed Bug 872701 Opened 7 years ago Closed 7 years ago

Content breakage with "'highp' : precision is not supported in fragment shader"

Categories

(Core :: Canvas: WebGL, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: webgl-test-needed)

Attachments

(2 files, 5 obsolete files)

After the ANGLE update in bug 861039, we're seeing a huge amount of broken content, with the following error:
'highp' : precision is not supported in fragment shader

Some of this is improper responses to the errors generated on GLES when the precision for float is not specified.
Incorrect:
  precision highp float;

Incorrect:
  #ifdef GL_ES
  precision highp float;
  #endif

Correct:
  #ifdef GL_ES
  #ifdef GL_FRAGMENT_PRECISION_HIGH
  precision highp float;
  #else
  precision mediump float;
  #endif
  #endif

Correct:
  #ifdef GL_FRAGMENT_PRECISION_HIGH
  precision highp float;
  #else
  precision mediump float;
  #endif

However, notable, while three.js appears to suffer from this, three.js actually is checking GetShaderPrecisionFormat to query whether or not highp in frag shaders is supported. GetShaderPrecisionFormat should return zeros if a shaderType+precisionType is unsupported. Unfortunately, ANGLE is currently unconditionally giving non-zero precision back for FRAGMENT_SHADER+HIGH_FLOAT.

ANGLE now only allows compiling with `highp` in frag shaders if `context->fragmentPrecisionHigh` is true, which is set via `ShBuiltInResources::FragmentPrecisionHigh`, which we don't set.
Easiest fix seems to be to set that flag?
Yes, doing the patch now.

It'd be a one-liner if I didn't want to allow us to disable highp frag shaders for testing.
Blocks: 872307
Comment on attachment 750033 [details] [diff] [review]
Tell ANGLE to allow highp in frag shaders, unless disabled.

Review of attachment 750033 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't introduce a new pref just for that! Use min-capability-mode if you want.
Attachment #750033 - Flags: review?(bjacob) → review-
Alright, good point.

We should also make sure that:
1) MinCapMode functions.
2) MinCapMode actually limits properly. (bjacob says it doesn't do so consistently)

I'll file bugs for these, but they're unrelated to this bug.
Attachment #750033 - Attachment is obsolete: true
Attachment #750084 - Flags: review?(bjacob)
Comment on attachment 750084 [details] [diff] [review]
Tell ANGLE to allow highp in frag shaders, unless disabled.

Review of attachment 750084 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, another issue:

I'm uncomfortable about having two separate places in the code that have the knowledge that we allow highp in fragment shaders if and only if !MinCapabilityMode().

Could you add a bool member to WebGLContext, initialize it e.g. in InitAndValidate() (or whatever that function is named), and use it?
Attachment #750084 - Flags: review?(bjacob) → review-
Attachment #750084 - Attachment is obsolete: true
Attachment #750089 - Flags: review?(bjacob)
Attachment #750089 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/ca7e571932c3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Was this issue ANGLE-only? I've seen many of my WebGL demos that use CubicVR break with this error. Changing highp to lowp fixed things.
Yes, ANGLE-only. highp precision in the fragment shader language is not universally supported, and ANGLE now defaults to not exposing it unless explicitly asked to. mediump precision is universally supported in the fragment shader language, so if you want a WebGL program to run everywhere in this respect, you just need to lower fragment shader precision qualifiers to mediump, no need to go all the way down to lowp.
Benoit is correct.

I'll note that if you can get away with using mediump, you always should. (highp might be slower). If you can't without degrading quality, but want to at least render (poorly) on mediump-only machines, use the last fallback in my first post.

Checking that GetShaderPrecisionFormat returns non-zero for HIGH_FLOAT should also work, though notable did NOT work with ANGLE. I'm going to report this small bit to ANGLE.

On the other hand, if ANGLE didn't have this issue, we would have been silently downgraded to mediump without knowing for a while. I suppose the silver lining is that this broke.

I should also add some tests for this.
Flags: needinfo?(jgilbert)
Forgot to add the error hook for better failure detection.
Updated webgl-util.js to differentiate absolute errors (bad element ids) from possibly-desired errors (such as linking failure).
Attachment #806219 - Attachment is obsolete: true
Attachment #806219 - Flags: review?(bjacob)
Attachment #806235 - Flags: review?(bjacob)
Comment on attachment 806235 [details] [diff] [review]
patch: Add a test for that frag shader highp floats work if getShaderPrecisionFormat claims they do

Review of attachment 806235 [details] [diff] [review]:
-----------------------------------------------------------------

Please explain why this conformance test is not a conformance test?

::: content/canvas/test/webgl/non-conf-tests/test_highp_fs.html
@@ +40,5 @@
> +  ok(gl, 'Should have a WebGL context.');
> +
> +  var format = gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_FLOAT);
> +  if (!format)
> +    return;

I would prefer if you also tested that, when highp is not available, it is really not available (i.e. the converse).
Attachment #806235 - Flags: review?(bjacob) → review+
...also, did you forget to update a makefile?
(In reply to Benoit Jacob [:bjacob] from comment #16)
> ...also, did you forget to update a makefile?

Quite possibly. I'll double-check that it's updated.


(In reply to Benoit Jacob [:bjacob] from comment #15)
> Comment on attachment 806235 [details] [diff] [review]
> patch: Add a test for that frag shader highp floats work if
> getShaderPrecisionFormat claims they do
> 
> Review of attachment 806235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please explain why this conformance test is not a conformance test?
> 
> ::: content/canvas/test/webgl/non-conf-tests/test_highp_fs.html
> @@ +40,5 @@
> > +  ok(gl, 'Should have a WebGL context.');
> > +
> > +  var format = gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_FLOAT);
> > +  if (!format)
> > +    return;
> 
> I would prefer if you also tested that, when highp is not available, it is
> really not available (i.e. the converse).

Alright.
Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
Both backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/dc74db9e511e since that still left b2g busted. The good thing is, that wall of orange will tell you which varied suites you need to run on Try to get the test to run on every platform, for the next patch.
More churn.
Attachment #806235 - Attachment is obsolete: true
Attachment #815671 - Flags: review?(bjacob)
No need to special case ignoring android 2.2 tests, if we're just going to `todo` on webgl context creation failure.
Attachment #815671 - Attachment is obsolete: true
Attachment #815671 - Flags: review?(bjacob)
Attachment #815679 - Flags: review?(bjacob)
Comment on attachment 815679 [details] [diff] [review]
patch: Add test for highp

Review of attachment 815679 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I still don't get why this can't be a regular conformance test.
Attachment #815679 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #23)
> Comment on attachment 815679 [details] [diff] [review]
> patch: Add test for highp
> 
> Review of attachment 815679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but I still don't get why this can't be a regular conformance test.

It can be, but we'd need to submit it and then pull down whatever ends up making it into the suite.
Also, it would only go in trunk, not 1.0.1, or even 1.0.2 (which we don't even have yet).
And that's fine! This way, we're good citizens, and we get it back the next time we sync our test suite with upstream (from my recent testing, 1.0.3 trunk is in plenty good shape so we'd likely prefer it over 1.0.2 at this point anyway).
(In reply to Benoit Jacob [:bjacob] from comment #25)
> And that's fine! This way, we're good citizens, and we get it back the next
> time we sync our test suite with upstream (from my recent testing, 1.0.3
> trunk is in plenty good shape so we'd likely prefer it over 1.0.2 at this
> point anyway).

The problem is we don't have a timeframe for this happening. There are certainly a number of more important things for me to do before getting to this, but we should have *some* tests in the mean time.
OK, so at least, let's keep flagging our bugs with webgl-test-needed.

(Done.)
Whiteboard: webgl-test-needed
Cool, thanks.
You need to log in before you can comment on or make changes to this bug.