Closed Bug 1048747 Opened 10 years ago Closed 9 years ago

WebGL2 - Implement Uniform Buffer Objects

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: u480271, Assigned: u480271)

References

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 2 obsolete files)

10.79 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
10.60 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
10.48 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.02 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
11.07 KB, patch
jgilbert
: review-
smaug
: review-
Details | Diff | Splinter Review
1.69 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
15.10 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
12.62 KB, patch
jgilbert
: review+
smaug
: review+
Details | Diff | Splinter Review
2.82 KB, patch
jgilbert
: review+
smaug
: review+
Details | Diff | Splinter Review
25.71 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
Blocks: webgl2
Keywords: leave-open
Add GL feature detection of uniform buffer functions.
Attachment #8469811 - Flags: review?(jgilbert)
Adds GL extension checks for glGetInteger64i_v function.
Attachment #8469812 - Flags: review?(jgilbert)
Attachment #8469813 - Flags: review?(jgilbert)
Attachment #8469811 - Flags: review?(jgilbert) → review+
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.
Attachment #8469812 - Flags: review?(jgilbert) → review-
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.
Attachment #8469813 - Flags: review?(jgilbert) → review-
Attachment #8469812 - Attachment is obsolete: true
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.
Attachment #8494842 - Flags: review?(jgilbert)
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.
Attachment #8494842 - Flags: review?(jgilbert) → review+
Attachment #8524368 - Flags: review?(jgilbert)
Attachment #8524368 - Flags: review?(jgilbert) → review+
Attachment #8527392 - Flags: review?(jgilbert)
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.
Attachment #8527392 - Flags: review?(jgilbert) → review-
Attachment #8527394 - Flags: review?(jgilbert) → review+
(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.
Attached patch review commentsSplinter Review
Address comments + wrap long lines at 90 characters.
Attachment #8531809 - Flags: review?(jgilbert)
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.
Attachment #8531809 - Flags: review?(jgilbert) → review+
Attachment #8527392 - Flags: review?(bugs)
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8527392 - Flags: review?(bugs) → review-
Implemented Olli's advice about the functions that return "any". Found and fixed bug in getIndexedParameter.
Attachment #8534803 - Flags: review?(jgilbert)
Attachment #8534803 - Flags: review?(bugs)
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?
Attachment #8534803 - Flags: review?(bugs) → review+
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.
Attachment #8534803 - Flags: review?(jgilbert) → review+
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.
Attachment #8537005 - Flags: review?(jgilbert)
Attachment #8537005 - Flags: review?(bugs)
Attachment #8537005 - Flags: review?(bugs) → review+
Blocks: 1107685
Attachment #8537005 - Flags: review?(jgilbert) → review+
Attached patch Cleanup GetUniformBlockIndex. (obsolete) — Splinter Review
Attachment #8579115 - Flags: review?(jgilbert)
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)
Attachment #8579115 - Flags: review?(jgilbert) → review-
(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.
Comment 43
Can you elaborate on what we need to do check that arrayIndex is valid? Is the array count kept in WebGLActiveInfo?
Flags: needinfo?(jgilbert)
(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)
Flags: needinfo?(jgilbert)
(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.
Attachment #8579115 - Attachment is obsolete: true
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`?
Attachment #8593655 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #50)
> Prefer `RefPtr` to `nsRefPtr`.

I cribbed from activeAttribs and activeUniforms which are nsRefPtr.
Blocks: 1156589
What's left to be done here?
Flags: needinfo?(dglastonbury)
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: needinfo?(dglastonbury)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.