Closed
Bug 708207
Opened 13 years ago
Closed 12 years ago
Implement getShaderPrecisionFormat in WebGL
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: drs, Assigned: drs)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 3 obsolete files)
18.37 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
3.97 KB,
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=453ffdc58580
Assignee | ||
Comment 4•13 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 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=038e5cf4c500
Comment 8•12 years ago
|
||
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 | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
Attachment #582132 -
Flags: review?(bjacob)
Comment 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 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•12 years ago
|
||
Updated ( see http://www.khronos.org/bugzilla/show_bug.cgi?id=563 )
Attachment #579625 -
Attachment is obsolete: true
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9939bcd259a3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 14•9 years ago
|
||
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.
Description
•