Closed Bug 1237193 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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
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
Attachment #8704618 - Flags: review?(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?
Attachment #8704618 - Flags: review?(jgilbert)
Attachment #8704618 - Flags: review?(efaustbmo)
Attachment #8704618 - Flags: review+
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.
Attachment #8704618 - Flags: review?(efaustbmo) → review+
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.
apply jgilbert's comments.
Attachment #8704618 - Attachment is obsolete: true
Keywords: checkin-needed
this needs dom peer review
Flags: needinfo?(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.
Flags: needinfo?(ethlin)
Attachment #8705487 - Flags: review?(bugs)
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
Attachment #8705487 - Flags: review?(bugs) → review+
Keywords: checkin-needed
caused bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=20551706&repo=mozilla-inbound and so got backedout
Flags: needinfo?(ethlin)
Fix the warning problem.
Attachment #8705487 - Attachment is obsolete: true
Flags: needinfo?(ethlin)
Keywords: checkin-needed
Transfer the patch to hg format.
Attachment #8719387 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a3f46ba96f95
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.