Add GL feature detection of uniform buffer functions.

Adds GL extension checks for glGetInteger64i_v function.

Comment on attachment 8469812 [details] [diff] [review] Patch 2 - WebGL2 - Implement GetInteger64i_v extension checks. Review of attachment 8469812 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +382,5 @@ > { (PRFuncPtr*) &mSymbols.fGetAttachedShaders, { "GetAttachedShaders", "GetAttachedShadersARB", nullptr } }, > { (PRFuncPtr*) &mSymbols.fGetAttribLocation, { "GetAttribLocation", "GetAttribLocationARB", nullptr } }, > { (PRFuncPtr*) &mSymbols.fGetIntegerv, { "GetIntegerv", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fGetIntegeri_v, { "GetIntegeri_v", "GetIntegerIndexedvEXT", "GetIntegerIndexedvNV", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fGetInteger64i_v, { "GetInteger64i_v", nullptr } }, Huh? We should not be requiring these symbols for GLContext creation.

Comment on attachment 8469813 [details] [diff] [review] Patch 3 - WebGL2 - Implement uniform block/buffer. Review of attachment 8469813 [details] [diff] [review]: ----------------------------------------------------------------- I think I'm going to r- non-thread-safe things from now on. WebGL in workers is coming about on the same timeframe as WebGL2, maybe sooner. ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +183,5 @@ > + > + switch (target) { > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > + { This style is growing on me. It sorta-kinda matches the style for `public:` and friends. I think we should probably keep the `{` on the previous line, though. @@ +218,5 @@ > + if (!ValidateObject("getUniformIndics: program", program)) > + return; > + > + if (!uniformNames.Length()) > + return; I think the early return here is unnecessary, so we should just let it though, knowing it won't execute the loop. @@ +291,5 @@ > + case LOCAL_GL_UNIFORM_BLOCK_NAME_LENGTH: > + case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_VERTEX_SHADER: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_FRAGMENT_SHADER: > + { Bring this `{` scope up one indent. @@ +310,5 @@ > + gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, (GLint*) JS_GetUint32ArrayData(obj)); > + retval.set(JS::ObjectOrNullValue(obj)); > + return; > + } > + } This looks like an error. (It's not, but let's avoid this by indenting case scopes. @@ +317,4 @@ > } > > +#define WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH 256 > +static char nameBuffer[WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH]; We should avoid adding non-thread-safe things at this point. Just put it on the stack. Get* funcs are fine to leave less optimized.

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/54f8ddeb0783 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/eae3e0248ba1 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/f4bde51acbf0

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/1c897f3058ed https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/87dc104c9838 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/597a5cd4b47e

Clean up taking :jgilbert comments. Split out feature checks for GetIntegeri_v and GetInteger64i_v because they don't appear in GL in the same revisions or extensions.

So GL_EXT_texture_storage has the following: In particular, note that OpenGL ES 1.x/2.0 do not have proxy textures, 1D textures, or 3D textures, and thus only the TexStorage2DEXT entry point is required. If OES_texture_3D is supported, the TexStorage3DEXT entry point is also required. So I'm going to remove the GL_EXT_texture_storage check because it's not good enough for what we need.

Comment on attachment 8527392 [details] [diff] [review] [WebGL2] Implement uniform block/buffer Review of attachment 8527392 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +340,5 @@ > + switch (target) { > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > + { > + GLint data; Init to zero for safety. @@ +351,5 @@ > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_SIZE: > + case LOCAL_GL_UNIFORM_BUFFER_START: > + case LOCAL_GL_UNIFORM_BUFFER_SIZE: > + { > + GLint64 data; Init to zero for safety. @@ +385,5 @@ > + > + for (size_t n = 0; n < count; n++) { > + const nsString& name = uniformNames[n]; > + GLuint index = 0; > + const GLchar* glname = NS_LossyConvertUTF16toASCII(name).get(); You have to have an active local instance of NS_LossyConvertUTF16toASCII. NS_LossyConvertUTF16toASCII foo(name); const char* bar = foo.get(); @@ +463,5 @@ > + case LOCAL_GL_UNIFORM_BLOCK_NAME_LENGTH: > + case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_VERTEX_SHADER: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_FRAGMENT_SHADER: > + { case FOO: { // bar } @@ +483,5 @@ > + gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, (GLint*) JS_GetUint32ArrayData(obj, nogc)); > + retval.set(JS::ObjectOrNullValue(obj)); > + return; > + } > + } Ick. @@ +490,4 @@ > } > > +#define WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH 256 > +static char nameBuffer[WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH]; Don't use non-threadsafe statics. Just use an nsAutoCString and SetLength.

(In reply to Jeff Gilbert [:jgilbert] from comment #23) > Comment on attachment 8527392 [details] [diff] [review] > [WebGL2] Implement uniform block/buffer > > Review of attachment 8527392 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGL2ContextUniforms.cpp > @@ +340,5 @@ > > + switch (target) { > > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > > + { > > + GLint data; > > Init to zero for safety. I hate the { } inside a case clause. I'm going to pull them out. > @@ +483,5 @@ > > + } > > + } This one a bit harder to remove, so I indented the block.

Address comments + wrap long lines at 90 characters.

Comment on attachment 8531809 [details] [diff] [review] review comments Review of attachment 8531809 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +31,5 @@ > } > > bool > +WebGL2Context::ValidateAttribPointerType(bool integerMode, GLenum type, > + GLsizei* alignment, const char* info) If `alignment` is an out-var, please prefix with `out_`, especially since `info` comes after it, and is an in-var. @@ +392,5 @@ > MakeContextCurrent(); > > for (size_t n = 0; n < count; n++) { > + NS_LossyConvertUTF16toASCII name(uniformNames[n]); > + const GLchar* glname = name.get(); I think BeginReading() is preferable, but get() may work too.

Olli, Require a review of webidl change to getActiveUniformBlockParameter. getActiveUniformBlockParameter allocates a JS array for result which can fail and throw. There is precedence in WebGL1, where we have adorned some IDL with [throw] which isn't in the public spec.

Comment on attachment 8527392 [details] [diff] [review] [WebGL2] Implement uniform block/buffer Why we need to return 'any'? Would the following work? [Throws] (long or sequence<{some sane type here}>)? getActiveUniformBlockParameter(WebGLProgram? program, GLuint uniformBlockIndex, GLenum pname); Similar question about GetIndexedParameter. The less we use JSAPI the better.

Implemented Olli's advice about the functions that return "any". Found and fixed bug in getIndexedParameter.

Comment on attachment 8534803 [details] [diff] [review] reduce scope of getActiveUniformBlockParameter (at some point it would be nice to get this all converted to use Mozilla coding style. {} with ifs and 2 space indentation, aFoo naming for arguments, etc. Especially lack of aFoo naming makes reading the code harder.) > GLuint progname = program->GLName(); > GLint param = 0; >+ JS::RootedObject array(cx); Why you define this here > case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES: >+ if (!GetUniformBlockActiveUniforms(gl, cx, this, progname, uniformBlockIndex, >+ &array)) When you use it only within this case: So, move it to above the 'if' ? > { >- GLint length = 0; >- gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, >- LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS, &length); >- JSObject* obj = Uint32Array::Create(cx, this, length, nullptr); >- if (!obj) { >- rv = NS_ERROR_OUT_OF_MEMORY; >- } >- JS::AutoCheckCannotGC nogc; >- gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, >- (GLint*) JS_GetUint32ArrayData(obj, nogc)); >- retval.set(JS::ObjectOrNullValue(obj)); >+ rv = NS_ERROR_OUT_OF_MEMORY; > return; Not clear to me why you created a helper method for this, but I guess it is fine. >+++ b/dom/webidl/WebGL2RenderingContext.webidl Btw, what does the spec say? Should the .webidl in the spec be changed too?

Comment on attachment 8534803 [details] [diff] [review] reduce scope of getActiveUniformBlockParameter Review of attachment 8534803 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +488,4 @@ > void > WebGL2Context::GetActiveUniformBlockParameter(JSContext* cx, WebGLProgram* program, > GLuint uniformBlockIndex, GLenum pname, > + Nullable<dom::OwningUnsignedLongOrUint32ArrayOrBoolean>& retval, This is what magic gets us, I guess.

sorry had to back this out since this might have caused https://treeherder.mozilla.org/logviewer.html#?job_id=4642792&repo=mozilla-inbound at least with this push

Alright, I've fixed the rooting hazard. http://tbpl.mozilla.org/?tree=Try&rev=cfdd17bde752

I was extracting the pointer to Uint32Array data the wrong way. I hope this is the correct way. It does fix the rooting hazard.

Comment on attachment 8579115 [details] [diff] [review] Cleanup GetUniformBlockIndex. Review of attachment 8579115 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +10,5 @@ > #include "WebGLVertexArray.h" > #include "WebGLVertexAttribData.h" > #include "mozilla/dom/WebGL2RenderingContextBinding.h" > > +namespace mozilla { Thanks! ::: dom/canvas/WebGLProgram.cpp @@ +510,5 @@ > + if (!ParseName(userName, &baseUserName, &isArray, &arrayIndex)) > + return LOCAL_GL_INVALID_INDEX; > + > + const WebGLActiveInfo* activeInfo; > + if (!LinkInfo()->FindUniform(baseUserName, &activeInfo)) { Woah, LinkInfo() might be null here. See below in GetUniformLocation, where the second tests is `if (!IsLinked) => INVALID_OPERATION`. @@ +520,5 @@ > + > + nsAutoCString mappedName(baseMappedName); > + if (isArray) { > + mappedName.AppendLiteral("["); > + mappedName.AppendInt(uint32_t(arrayIndex)); I think we need to check that arrayIndex is valid, as well. (I think this info is also hanging off the ActiveInfo object)

(In reply to Jeff Gilbert [:jgilbert] from comment #42) > ::: dom/canvas/WebGLProgram.cpp > @@ +510,5 @@ > > + if (!ParseName(userName, &baseUserName, &isArray, &arrayIndex)) > > + return LOCAL_GL_INVALID_INDEX; > > + > > + const WebGLActiveInfo* activeInfo; > > + if (!LinkInfo()->FindUniform(baseUserName, &activeInfo)) { > > Woah, LinkInfo() might be null here. See below in GetUniformLocation, where > the second tests is `if (!IsLinked) => INVALID_OPERATION`. I swear I put that check in... > @@ +520,5 @@ > > + > > + nsAutoCString mappedName(baseMappedName); > > + if (isArray) { > > + mappedName.AppendLiteral("["); > > + mappedName.AppendInt(uint32_t(arrayIndex)); > > I think we need to check that arrayIndex is valid, as well. (I think this > info is also hanging off the ActiveInfo object) I'm not sure. I don't think that arrays are valid for uniform block names. I was mostly just cribbing from GetUniformLocation.

Can you elaborate on what we need to do check that arrayIndex is valid? Is the array count kept in WebGLActiveInfo?

(In reply to Dan Glastonbury :djg :kamidphish from comment #45) > Can you elaborate on what we need to do check that arrayIndex is valid? Is > the array count kept in WebGLActiveInfo? We talked about this, but it's worth dumping in the bug. The array count would be in WebGLActiveInfo. We need to generated the list of these for the program when we create the program's LinkInfo. (by querying the driver for info about them)

try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0626bcc72e

(In reply to Dan Glastonbury :djg :kamidphish from comment #48) > try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0626bcc72e Well that's a bust.

Comment on attachment 8593655 [details] [diff] [review] Completely rework how uniform interface blocks are handled. r=jgilbert Review of attachment 8593655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLProgram.cpp @@ +113,5 @@ > + (GLint*)&maxUniformBlockLenWithNull); > + if (maxUniformBlockLenWithNull < 1) > + maxUniformBlockLenWithNull = 1; > + } > + whitespace! ::: dom/canvas/WebGLProgram.h @@ +55,5 @@ > std::map<nsCString, const WebGLActiveInfo*> attribMap; > std::map<nsCString, const WebGLActiveInfo*> uniformMap; > std::map<nsCString, const nsCString>* fragDataMap; > > + std::vector<nsRefPtr<UniformBlockInfo>> uniformBlocks; Prefer `RefPtr` to `nsRefPtr`. @@ +85,5 @@ > return true; > } > > + bool FindUniformBlock(const nsCString& baseUserName, > + const UniformBlockInfo** const out_info) const `RefPtr<const UniformBlockInfo>* const out_info`?

(In reply to Jeff Gilbert [:jgilbert] from comment #50) > Prefer `RefPtr` to `nsRefPtr`. I cribbed from activeAttribs and activeUniforms which are nsRefPtr.

try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=21167facbca8

What's left to be done here?

I was keeping this open to add more patches on an `as needed` basis. I see a follow up about getting to pass UBO conformance tests, so I'll close this and deal with each, specific failure in a separate bug.