1237193 - [WebGL2] pass getActiveUniforms in gl-object-get-calls.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

Test getActiveUniforms
PASS gl.getProgramParameter(program, gl.LINK_STATUS) is true
PASS gl.getError() is gl.NO_ERROR
FAIL typeof rowMajors[i] should be boolean. Was number.
FAIL typeof rowMajors[i] should be boolean. Was number.
FAIL typeof rowMajors[i] should be boolean. Was number.
FAIL getActiveUniforms returned -454761244,-454761244,-454761244 instead of null for invalid pname enum: NO_ERROR

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Fix getActiveUniforms return type

By spec [1], the getActiveUniforms return type will vary with different pname. So I change the return type to any and set correct gl error when the pname is not in the table.

[1] https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.16

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8704618 [details] [diff] [review]
Fix getActiveUniforms return type

Review of attachment 8704618 [details] [diff] [review]:
-----------------------------------------------------------------

Does this JS look right?

::: dom/canvas/WebGL2Context.h
@@ +360,5 @@
>      void GetUniformIndices(WebGLProgram* program,
>                             const dom::Sequence<nsString>& uniformNames,
>                             dom::Nullable< nsTArray<GLuint> >& retval);
> +    void GetActiveUniforms(JSContext* cx,
> +                          WebGLProgram* program,

Fix the indents here.

::: dom/canvas/WebGLContext.h
@@ +1287,5 @@
>                                uint32_t* const out_width, uint32_t* const out_height);
>  
>      bool ValidateQueryEnum(GLenum pname, const char* info);
>  
> +    bool ValidateUniformEnum(GLenum pname, const char* info);

Why does this need to be a member of WebGLContext, and not just a static that takes a WebGLContext?

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8704618 [details] [diff] [review]
Fix getActiveUniforms return type

Review of attachment 8704618 [details] [diff] [review]:
-----------------------------------------------------------------

All the JSAPI stuff looks fine to me.

Comment 4

[Ethan Lin[:ethlin]]

Comment on attachment 8704618 [details] [diff] [review]
Fix getActiveUniforms return type

Review of attachment 8704618 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGL2Context.h
@@ +360,5 @@
>      void GetUniformIndices(WebGLProgram* program,
>                             const dom::Sequence<nsString>& uniformNames,
>                             dom::Nullable< nsTArray<GLuint> >& retval);
> +    void GetActiveUniforms(JSContext* cx,
> +                          WebGLProgram* program,

Okay.

::: dom/canvas/WebGLContext.h
@@ +1287,5 @@
>                                uint32_t* const out_width, uint32_t* const out_height);
>  
>      bool ValidateQueryEnum(GLenum pname, const char* info);
>  
> +    bool ValidateUniformEnum(GLenum pname, const char* info);

Originally I thought it's some kind of rule to put all ValidateSomething here. I will move this to  WebGL2ContextUniforms.cpp as a static function.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Fix getActiveUniforms return type (carry r+: jgilbert)(carry r+: efaust)

apply jgilbert's comments.

Comment 6

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ad28c837a74

Comment 7

[Carsten Book [:Tomcat]]

this needs dom peer review

Comment 8

[Ethan Lin[:ethlin]]

Comment on attachment 8705487 [details] [diff] [review]
Fix getActiveUniforms return type (carry r+: jgilbert)(carry r+: efaust)

Could you help review WebIDL changes? Thanks.

Comment 9

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

Comment on attachment 8705487 [details] [diff] [review]
Fix getActiveUniforms return type (carry r+: jgilbert)(carry r+: efaust)

>+    JS::Rooted<JSObject*> array(cx, JS_NewArrayObject(cx, count));
>+    if (!array) {
>+        return;
>+    }
>+
>+    switch (pname) {
>+    case LOCAL_GL_UNIFORM_TYPE:
>+    case LOCAL_GL_UNIFORM_SIZE:
>+    case LOCAL_GL_UNIFORM_BLOCK_INDEX:
>+    case LOCAL_GL_UNIFORM_OFFSET:
>+    case LOCAL_GL_UNIFORM_ARRAY_STRIDE:
>+    case LOCAL_GL_UNIFORM_MATRIX_STRIDE:
>+        for (uint32_t i = 0; i < count; ++i) {
>+            JS::RootedValue value(cx);
>+            value = JS::Int32Value(samples[i]);
>+            if (!JS_DefineElement(cx, array, i, value, JSPROP_ENUMERATE)) {
>+                return;
>+            }
>+        }
>+        break;
>+    case LOCAL_GL_UNIFORM_IS_ROW_MAJOR:
>+        for (uint32_t i = 0; i < count; ++i) {
>+            JS::RootedValue value(cx);
>+            value = JS::BooleanValue(samples[i]);
>+            if (!JS_DefineElement(cx, array, i, value, JSPROP_ENUMERATE)) {
>+                return;
>+            }
Sad to see so much JSAPI usage here. But I guess ToJSValue wouldn't have worked here since samples 
contains different type of values than what LOCAL_GL_UNIFORM_IS_ROW_MAJOR expects for the js array.


r+ to the .webidl

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

caused bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=20551706&repo=mozilla-inbound and so got backedout

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f509b7156c7

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Fix getActiveUniforms return type (carry r+: jgilbert, efaust, smaug)

Fix the warning problem.

Comment 14

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37e3163614e4

Comment 15

[Ethan Lin[:ethlin]]

Attachment: Fix getActiveUniforms return type (carry r+: jgilbert, efaust, smaug)

Transfer the patch to hg format.

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

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