The default bug view has changed. See this FAQ.

Implement getShaderPrecisionFormat in WebGL

RESOLVED FIXED in mozilla11

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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).
Assignee: nobody → dsherk
(Assignee)

Comment 2

5 years ago
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.
Attachment #579630 - Flags: review?(bjacob)
(Assignee)

Comment 3

5 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=453ffdc58580
(Assignee)

Comment 4

5 years ago
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 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?
Attachment #579630 - Flags: review?(bjacob) → review-
(Assignee)

Comment 6

5 years ago
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.
Attachment #579630 - Attachment is obsolete: true
Attachment #581569 - Flags: review?(bjacob)
(Assignee)

Comment 7

5 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=038e5cf4c500
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?
(Assignee)

Updated

5 years ago
Blocks: 711226
(Assignee)

Comment 9

5 years ago
Created attachment 582132 [details] [diff] [review]
Patch v1.2, implement ShaderPrecisionFormat.

Moved getShaderPrecisionFormat to its own list and removed all CONTINUE_ENTRY hacks.
Attachment #581569 - Attachment is obsolete: true
Attachment #581569 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
Attachment #582132 - Flags: review?(bjacob)
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.
Attachment #582132 - Flags: review?(bjacob) → review+
Depends on: 711588
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Comment 12

5 years ago
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 )
Attachment #579625 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9939bcd259a3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.