708207 - Implement getShaderPrecisionFormat in WebGL

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Doug Sherk (:drs) (inactive)]

From OpenGL ES 2.0, we don't implement this yet and it was a recent addition to the WebGL spec. See the following document:

http://www.khronos.org/registry/webgl/specs/latest/#5.14.9

This is a peculiar problem. It is specific to OpenGL ES 2.0, but not all implementations of WebGL support this, thus the functionality of this function is unclear when ES 2.0 is not supported. I am assuming that returning a blank object of type WebGLShaderPrecisionFormat (i.e. all zeroed members) is acceptable.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch to Khronos test suite to add shader precision testing.

I don't really know if this is an adequate set of tests or not, but based on what I've read, this seems to be reasonable. Will submit to Khronos too (obviously).

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, implement ShaderPrecisionFormat.

I had to make a really bad change to GLContext.cpp, such that it now has two lists. I feel like I have to be missing something, but apparently this GetShaderPrecisionFormat function only exists in OpenGL ES 2.0, but it has to go between GetShaderInfoLog and GetShaderSource or it doesn't get detected. Perhaps I should have loaded it in a separate symbol table instead, although this doesn't make sense since this isn't part of an extension.

Additionally, my concern in the initial post is still valid. I don't know how we're supposed to handle implementations that don't have Open GL ES 2.0. My assumption right now is to just go with what I originally said. Before I ask on Public WebGL, I would like input from bjacob.

Comment 3

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=453ffdc58580

Comment 4

[Doug Sherk (:drs) (inactive)]

Khronos bug to add "Patch to Khronos test suite to add shader precision testing." to their repo:
http://www.khronos.org/bugzilla/show_bug.cgi?id=563

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 579630 [details] [diff] [review]
Patch v1.0, implement ShaderPrecisionFormat.

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +4542,5 @@
> +        case LOCAL_GL_FRAGMENT_SHADER:
> +        case LOCAL_GL_VERTEX_SHADER:
> +            break;
> +        default:
> +            return ErrorInvalidEnumInfo("getShaderPrecisionFormat: invalid shadertype", shadertype);

We usually don't write 'invalid' in these error messages, it's implied.

@@ +4554,5 @@
> +        case LOCAL_GL_MEDIUM_INT:
> +        case LOCAL_GL_HIGH_INT:
> +            break;
> +        default:
> +            return ErrorInvalidEnumInfo("getShaderPrecisionFormat: invalid precisiontype", precisiontype);

same

::: gfx/gl/GLContext.cpp
@@ +358,5 @@
>  
> +    SymLoadStruct symbols_GLES2[] = {
> +        { (PRFuncPtr*) &mSymbols.fActiveTexture, { "ActiveTexture", "ActiveTextureARB", NULL } },
> +        { (PRFuncPtr*) &mSymbols.fAttachShader, { "AttachShader", "AttachShaderARB", NULL } },
> +        { (PRFuncPtr*) &mSymbols.fBindAttribLocation, { "BindAttribLocation", "BindAttribLocationARB", NULL } },

Whoa, what's happening here? Seems like a big unrelated refactoring. Please split into a separate bug/patch.

::: gfx/gl/GLContext.h
@@ +2326,5 @@
> +        BEFORE_GL_CALL;
> +        if (mIsGLES2) {
> +            mSymbols.fGetShaderPrecisionFormat(shadertype, precisiontype, range, precision);
> +        } else {
> +            // No sensible fallback.

Really? We need to check that. Ask the mailing list?

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, implement ShaderPrecisionFormat.

Addressed code review comments.

Also, in retrospect, I think the patch should check if the range min/max are > 0 instead of >= 0.

Comment 7

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=038e5cf4c500

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 581569 [details] [diff] [review]
Patch v1.1, implement ShaderPrecisionFormat.

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

::: gfx/gl/GLContext.cpp
@@ +165,5 @@
> +        // Special case a 0x1 pointer to mean that we want it to continue.
> +        if (ss->symPointer == CONTINUE_ENTRY) {
> +            ss++;
> +            continue;
> +        }

How about instead have a separate list of symbols for GLES-specific symbols?

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, implement ShaderPrecisionFormat.

Moved getShaderPrecisionFormat to its own list and removed all CONTINUE_ENTRY hacks.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 582132 [details] [diff] [review]
Patch v1.2, implement ShaderPrecisionFormat.

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

r=me with a comment nit:

::: gfx/gl/GLContext.cpp
@@ +404,5 @@
>          }
>  #endif
>  
> +        // Specially load this in because it's placed in a bizarre position in
> +        // the ES2 symbol table (between shaderInfoLog and shaderSource).

Is this comment still current? I thought we had traced the problem to LoadSymbols stopping on NULL i.e. the requirement to put any NULL at the end of the list. So a more accurate comment would be: load separately the ES-specific symbols because we can't pass NULL without stopping the loading of the current symbol list. or something.

Comment 11

[Doug Sherk (:drs) (inactive)]

The comment was wrong, but bug 711226 deletes this code fragment anyway and I'm landing them together.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9939bcd259a3

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Patch to Khronos test suite to add shader precision testing.

Updated ( see http://www.khronos.org/bugzilla/show_bug.cgi?id=563 )

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9939bcd259a3

Comment 14

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getShaderPrecisionFormat
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShaderPrecisionFormat
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShaderPrecisionFormat/rangeMin
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShaderPrecisionFormat/rangeMax
https://developer.mozilla.org/en-US/docs/Web/API/WebGLShaderPrecisionFormat/precision