681835 - object-deletion-behaviour.html test failures

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

This WebGL conformance test:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/misc/object-deletion-behaviour.html

fails in many places. The first is:

PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) is gl.NONE
FAIL gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) expected: INVALID_ENUM. Was NO_ERROR.

In other words, when there is framebuffer attachment, a query for FRAMEBUFFER_ATTACHMENT_OBJECT_NAME should produce a INVALID_ENUM error (according to GLES 2.0 spec section 6.1.3), and we fail to generate any error.

Indeed, our code for that is:

http://hg.mozilla.org/mozilla-central/file/e58e98a89827/content/canvas/src/WebGLContextGL.cpp#l2193

We have a "case:" here for FRAMEBUFFER_ATTACHMENT_OBJECT_NAME, and removing it should fix this failure as the "default:" does the right thing.

Then there are many other failures in this page, good luck to fix them too :-)

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, WebGL object deletion behavior conformance fixes.

Fix for conformance issues with WebGL object deletion behavior.

The bindX() commands were erroring with INVALID_VALUE when they're instead supposed to simply fail silently when they're given a deleted object. Additionally, the getParameter() function was failing after its associated variable was deleted, sometimes returning values when it should return null.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, WebGL object deletion behavior conformance fixes.

Patch v1.1, WebGL object deletion behavior conformance fixes. Fixed failing_tests_* files.

Comment 3

[Mozilla RelEng Bot]

Try run for b6532561077a is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b6532561077a
Results (out of 17 total builds):
    exception: 15
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b6532561077a

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Mozilla RelEng Bot from comment #3)
> Try run for b6532561077a is complete.
> Detailed breakdown of the results available here:
>     http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b6532561077a
> Results (out of 17 total builds):
>     exception: 15
>     failure: 2
> Builds available at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> b6532561077a

This was with the first patch, the second one is currently running here and seems to be working so far:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=0320e5612a50

Comment 5

[Mozilla RelEng Bot]

Try run for 0320e5612a50 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=0320e5612a50
Results (out of 30 total builds):
    success: 30
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0320e5612a50

Comment 6

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

Comment on attachment 555875 [details] [diff] [review]
Patch v1.1, WebGL object deletion behavior conformance fixes.

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

Great job, but there's a little problem:

::: content/canvas/src/WebGLContextGL.cpp
@@ +1081,5 @@
>      gl->fDeleteFramebuffers(1, &fbufname);
>      fbuf->Delete();
>      mMapFramebuffers.Remove(fbufname);
>  
> +    mBoundFramebuffer = NULL;

Good catch, but you can't do this unconditionally here. The framebuffer object being deleted here is not necessarily the currently bound one. You have to check that the framebuffer being deleted (fbuf) IS actually the currently bound one (mBoundFramebuffer). To compare them, I would compare their actual GL names which you can get by calling the GLName() method on them, though for fbuf you already have it in the fbufname variable in this function.

@@ +1117,4 @@
>      rbuf->Delete();
>      mMapRenderbuffers.Remove(rbufname);
>  
> +    mBoundRenderbuffer = NULL;

Same comment as for framebuffers.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0 (on top of previous): fixes issue with deleting buffers clearing context when it shouldn't

WebGL fix for previous patch which introduced a bug with deletion.

DeleteRenderbuffer and DeleteFramebuffer weren't checking if the deleted buffer was the currently bound buffer before deleting them. This patch implements this functionality. A separate test case patch was also submitted to Khronos:
http://www.khronos.org/bugzilla/show_bug.cgi?id=518

Note that this is applied on top of the previous patch; it doesn't supersede it. I did it this way because I thought I was going to be including the html test suite changes listed in the Khronos ticket above, but I ended up separating them.

Comment 8

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

Comment on attachment 556129 [details] [diff] [review]
Patch v1.0 (on top of previous): fixes issue with deleting buffers clearing context when it shouldn't

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

Thanks, this is good. But next time please merge the 2 patches together as now it's weird that I have to r+ a patch that depends on a r-'d patch ;-) A good tool to merge patches is 'hg qfold' if you're using MQ.

http://blog.mozilla.com/bjacob/2011/03/02/some-mercurial-queues-tips-hg-qcrecord-qfold-and-qpush-move/

Comment 9

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9e81dedac48

Comment 10

[(no longer active)]

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