567499 - webgl gl-object-get-call test fails

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Speaking about this test:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/fast/gl-object-get-calls.html

The attached patch fixes half of the failures in that test, but I am stuch with the GetTexParameter failures:

FAIL gl.getTexParameter(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER) should be 9728. Threw exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICanvasRenderingContextWebGL.getTexParameter]" nsresult: "0x80004005 (NS_ERROR_FAILURE)"

Here's what happens: in WebGLContext::GetTexParameter, at the first line we have

    NativeJSContext js;

Which calls the default constructor, which does:


        error = nsContentUtils::XPConnect()->GetCurrentNativeCallContext(&ncc);
        if (NS_FAILED(error))
            return;

        if (!ncc) {
            error = NS_ERROR_FAILURE;
            return;
        }

What's happening is that ncc is null, so we return there, with error = NS_ERROR_FAILURE. Any idea what's happening?

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Pass part of Khronos gl-object-get-calls test

Not yet asking for review, let's fix the rest first...

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

So, the line NativeJSContext.h:20,

    error = nsContentUtils::XPConnect()->GetCurrentNativeCallContext(&ncc);

if where things go wrong, inside of the GetCurrentNativeCallContext() call. Here's the code of that function:

  NS_IMETHODIMP
  nsXPConnect::GetCurrentNativeCallContext(
    nsAXPCNativeCallContext * *aCurrentNativeCallContext)
  {
      NS_ASSERTION(aCurrentNativeCallContext, "bad param");

so far, so good, aCurrentNativeCallContext is non-null.

    XPCPerThreadData* data = XPCPerThreadData::GetData(nsnull);

here, XPCPerThreadData::GetData returns sMainThreadData which is non-null.

    if(data)
    {

data is non-null, so we enter that branch.

        *aCurrentNativeCallContext = data->GetCallContext();

here we enter GetCallContext(), which returns mCallContext, which is null!

        return NS_OK;

So we return here, and on the caller side, ncc is null.


What is going wrong?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Pass most of Khronos gl-object-get-calls test

OK, here's a patch ready for review: it makes us pass all of that test, except the following:
1) the GetTexParameter bug discussed above: I'm really clueless about it.
2) the following failure:

FAIL gl.getUniform(boolProgram, bvalLoc) should be true (of type boolean). Was 1 (of type number).

Should we add full support for boolean uniforms? I have a hard time interpreting the spec: in 5.14.10 there's a table mentioning boolean uniforms, but there is no boolean counterpart to the uniform[if][1234] functions.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Note: patch is meant to be applied on top of the patch from bug 567565.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> Note: patch is meant to be applied on top of the patch from bug 567565.

(You mean bug 565404, I believe :-)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/fd3e5ddb8dda

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Wait!

As mentioned in comment 3, this is still not fully fixed, there still are a few errors! And as I explained above, I need help with those :)

Here is the corresponding text in the output of the test:

FAIL gl.getTexParameter(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER) should be 9728. Threw exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICanvasRenderingContextWebGL.getTexParameter]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/resources/js-test-pre.js :: shouldBe :: line 137" data: no]
FAIL gl.getTexParameter(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER) should be 9728. Threw exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICanvasRenderingContextWebGL.getTexParameter]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/resources/js-test-pre.js :: shouldBe :: line 137" data: no]
FAIL gl.getTexParameter(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S) should be 33071. Threw exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICanvasRenderingContextWebGL.getTexParameter]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/resources/js-test-pre.js :: shouldBe :: line 137" data: no]
FAIL gl.getTexParameter(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T) should be 33071. Threw exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICanvasRenderingContextWebGL.getTexParameter]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/resources/js-test-pre.js :: shouldBe :: line 137" data: no]
FAIL gl.getUniform(boolProgram, bvalLoc) should be true (of type boolean). Was 1 (of type number).

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

boolean uniforms are set via Uniform1i, but I guess there's a problem there with webgl, since we got rid of gl.TRUE and gl.FALSE -- send mail to the webgl list about it?

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

The very strange GetTexParameter bug is still present today, and I am still clueless about it, Vlad :)

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Huh, no idea for gettexparameter -- check and see if it's even getting called in that case?  Is the NativeCallContext creation failing?

Also, that function call needs to check 'target' to make sure it's a valid texture target.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Have you looked at my comments above? ;-) yes GetTexParameter is called, the error happens inside of the NativeJSContext default constructor.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh!  Sorry.  So the problem is that this is getting quickstubbed when it shouldn't be -- anything that uses the native xpconnect call context can't be.  Add it to the list of functions not quickstubbed in dom_quickstubs.qsconf

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

from looking at this again -- can you change this to use nsIVariant for the return?  That way we can skip the NativeCallContext, it can get quickstubbed, and we can return booleans/floats/whatever is needed easily.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Let getXxxParameter functions return nsIVariant

Thanks Vlad for the advice. This patch fixes this issue in all getXxxParameter functions.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix getUniform in the bool case and return nsIVariant

This fixes the last failure in this test: we now pass it completely! The GLSL program used in this test is declaring boolean uniforms, so we must support that; at the same time, it tests the subtlety that boolean _array_ uniforms must be handled as uint8 arrays, according to the spec.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451688 [details] [diff] [review]
Let getXxxParameter functions return nsIVariant

Great!  Looks good, but one fix:


> NS_IMETHODIMP
>-WebGLContext::GetTexParameter(WebGLenum target, WebGLenum pname)
>+WebGLContext::GetTexParameter(WebGLenum target, WebGLenum pname, nsIVariant **retval)

^ This still needs to check that |target| is either 2D or CUBE_MAP :)

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451689 [details] [diff] [review]
Fix getUniform in the bool case and return nsIVariant

Just a few style quirks:


>         if (unitSize == 1)
>-            js.SetRetVal(fv[0]);
>+            wrval->SetAsFloat(fv[0]);
>         else
>-            js.SetRetVal(fv, unitSize);
>+            wrval->SetAsArray(nsIDataType::VTYPE_FLOAT, nsnull,
>+                              unitSize, static_cast<void*>(fv));

Non-trivial (multiline) else statement, can you put braces around this stuff?  Also if any part of an if clause has braces, all should (see next comment).

>     } else if (baseType == LOCAL_GL_INT) {
>         GLint iv[16];
>         gl->fGetUniformiv(progname, location->Location(), iv);
>         if (unitSize == 1)
>-            js.SetRetVal(iv[0]);
>+            wrval->SetAsInt32(iv[0]);
>         else
>-            js.SetRetVal((PRInt32*)iv, unitSize);
>-
>+            wrval->SetAsArray(nsIDataType::VTYPE_INT32, nsnull,
>+                              unitSize, static_cast<void*>(iv));

Same thing

>+    } else if (baseType == LOCAL_GL_BOOL) {
>+        GLint iv[16];
>+        gl->fGetUniformiv(progname, location->Location(), iv);
>+        if (unitSize == 1)
>+            wrval->SetAsBool(PRBool(iv[0]));
>+        else {

{ } around the if clause as well, since the else has one.

>+            PRUint8 uv[16];
>+            for (int k = 0; k < unitSize; k++)
>+                uv[k] = PRUint8(iv[k]);
>+            wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull,
>+                              unitSize, static_cast<void*>(uv));
>+        }

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/212b80c33b5b
http://hg.mozilla.org/mozilla-central/rev/3fa3d8483e20