1048747 - WebGL2 - Implement Uniform Buffer Objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.14

Comment 1

[u480271]

Attachment: Patch 1 - WebGL2 - GL symbols for Uniform Buffers.

Add GL feature detection of uniform buffer functions.

Comment 2

[u480271]

Attachment: Patch 2 - WebGL2 - Implement GetInteger64i_v extension checks.

Adds GL extension checks for glGetInteger64i_v function.

Comment 3

[u480271]

Attachment: Patch 3 - WebGL2 - Implement uniform block/buffer.

Comment 4

[Kelsey Gilbert [:jgilbert]]

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 5

[Kelsey Gilbert [:jgilbert]]

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.

Comment 6

[u480271]

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

Comment 7

[u480271]

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

Comment 8

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c0b2156cff

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b3c0b2156cff

Comment 10

[u480271]

Attachment: WebGL2 - GL symbols for GetIntegeri_v and GetInteger64i_v.

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.

Comment 11

[u480271]

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 12

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=e2763315acd8

Comment 13

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6688b3916fc3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6688b3916fc3

Comment 15

[u480271]

Attachment: [WebGL2] Query active uniform blocks

Comment 16

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=5154511552bc

Comment 17

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=1b294c3790ab

Comment 18

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=a84e0bdb8145

Comment 19

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d18e1e60d52

Comment 20

[u480271]

Attachment: [WebGL2] Implement uniform block/buffer

Comment 21

[u480271]

Attachment: [WebGL2] Implement GetTexParameter for all valid WebGL2 pnames

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d18e1e60d52

Comment 23

[Kelsey Gilbert [: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.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2888d03c5e8

Comment 25

[u480271]

(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.

Comment 26

[u480271]

Attachment: review comments

Address comments + wrap long lines at 90 characters.

Comment 27

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=b3a28e9d77ef

Comment 28

[Kelsey Gilbert [: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.

Comment 29

[u480271]

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 30

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 31

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=fb550d9d5609

Comment 32

[u480271]

Attachment: reduce scope of getActiveUniformBlockParameter

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

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

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 34

[Kelsey Gilbert [:jgilbert]]

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.

Comment 35

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee50dac0cb7e

Comment 36

[Carsten Book [:Tomcat]]

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

Comment 37

[u480271]

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

Comment 38

[u480271]

Attachment: Fix rooting hazard in GetUniformBlockActiveUniforms

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 39

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb46363d636

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfb46363d636

Comment 41

[u480271]

Attachment: Cleanup GetUniformBlockIndex.

Comment 42

[Kelsey Gilbert [: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)

Comment 43

[u480271]

(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 44

[u480271]

Comment 43

Comment 45

[u480271]

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

Comment 46

[Kelsey Gilbert [: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)

Comment 47

[u480271]

Attachment: Completely rework how uniform interface blocks are handled. r=jgilbert

Comment 48

[u480271]

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

Comment 49

[u480271]

(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 50

[Kelsey Gilbert [:jgilbert]]

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`?

Comment 51

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #50)
> Prefer `RefPtr` to `nsRefPtr`.

I cribbed from activeAttribs and activeUniforms which are nsRefPtr.

Comment 52

[u480271]

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

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2bf34ef49d

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e2bf34ef49d

Comment 55

[Jeff Muizelaar [:jrmuizel]]

What's left to be done here?

Comment 56

[u480271]

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.

Comment 57

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext#Uniform_buffer_objects