Closed Bug 567499 Opened 14 years ago Closed 14 years ago

webgl gl-object-get-call test fails

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bjacob, Assigned: bjacob)

Details

Attachments

(3 files, 1 obsolete file)

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?
Not yet asking for review, let's fix the rest first...
Summary: Getting Khrono's gl-object-get-call test to pass → webgl gl-object-get-call test fails
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?
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.
Attachment #446860 - Attachment is obsolete: true
Attachment #448414 - Flags: review?(vladimir)
Note: patch is meant to be applied on top of the patch from bug 567565.
Assignee: nobody → bjacob
Flags: in-testsuite?
(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 :-)
http://hg.mozilla.org/mozilla-central/rev/fd3e5ddb8dda
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
The very strange GetTexParameter bug is still present today, and I am still clueless about it, Vlad :)
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.
Have you looked at my comments above? ;-) yes GetTexParameter is called, the error happens inside of the NativeJSContext default constructor.
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
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.
Thanks Vlad for the advice. This patch fixes this issue in all getXxxParameter functions.
Attachment #451688 - Flags: review?(vladimir)
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.
Attachment #451689 - Flags: review?(vladimir)
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 :)
Attachment #451688 - Flags: review?(vladimir) → review+
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));
>+        }
Attachment #451689 - Flags: review?(vladimir) → review+
http://hg.mozilla.org/mozilla-central/rev/212b80c33b5b
http://hg.mozilla.org/mozilla-central/rev/3fa3d8483e20
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: