Last Comment Bug 708207 - Implement getShaderPrecisionFormat in WebGL
: Implement getShaderPrecisionFormat in WebGL
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on: 711588
Blocks: 711226
  Show dependency treegraph
 
Reported: 2011-12-07 01:09 PST by Doug Sherk (:drs) (inactive)
Modified: 2015-09-30 06:42 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to Khronos test suite to add shader precision testing. (4.69 KB, patch)
2011-12-07 01:11 PST, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.0, implement ShaderPrecisionFormat. (29.64 KB, patch)
2011-12-07 01:39 PST, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.1, implement ShaderPrecisionFormat. (14.17 KB, patch)
2011-12-14 02:29 PST, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.2, implement ShaderPrecisionFormat. (18.37 KB, patch)
2011-12-15 15:45 PST, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Review
Patch to Khronos test suite to add shader precision testing. (3.97 KB, patch)
2011-12-16 15:16 PST, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review

Description Doug Sherk (:drs) (inactive) 2011-12-07 01:09:49 PST
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) 2011-12-07 01:11:38 PST
Created attachment 579625 [details] [diff] [review]
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) 2011-12-07 01:39:59 PST
Created attachment 579630 [details] [diff] [review]
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) 2011-12-07 01:41:19 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=453ffdc58580
Comment 4 Doug Sherk (:drs) (inactive) 2011-12-07 02:22:35 PST
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) 2011-12-13 11:04:55 PST
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) 2011-12-14 02:29:03 PST
Created attachment 581569 [details] [diff] [review]
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) 2011-12-14 02:29:17 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=038e5cf4c500
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-12-15 09:55:47 PST
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) 2011-12-15 15:45:56 PST
Created attachment 582132 [details] [diff] [review]
Patch v1.2, implement ShaderPrecisionFormat.

Moved getShaderPrecisionFormat to its own list and removed all CONTINUE_ENTRY hacks.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-12-16 10:33:05 PST
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) 2011-12-16 13:20:19 PST
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) 2011-12-16 15:16:12 PST
Created attachment 582407 [details] [diff] [review]
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) 2011-12-17 09:24:02 PST
https://hg.mozilla.org/mozilla-central/rev/9939bcd259a3

Note You need to log in before you can comment on or make changes to this bug.