object-deletion-behaviour.html test failures

RESOLVED FIXED in mozilla9

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

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 :-)
(Assignee)

Comment 1

6 years ago
Created attachment 555867 [details] [diff] [review]
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.
Attachment #555867 - Flags: review?(bjacob)
(Assignee)

Comment 2

6 years ago
Created attachment 555875 [details] [diff] [review]
Patch v1.1, WebGL object deletion behavior conformance fixes.

Patch v1.1, WebGL object deletion behavior conformance fixes. Fixed failing_tests_* files.
Attachment #555867 - Attachment is obsolete: true
Attachment #555867 - Flags: review?(bjacob)
Attachment #555875 - Flags: review?(bjacob)

Comment 3

6 years ago
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/dsherk@mozilla.com-b6532561077a
(Assignee)

Comment 4

6 years ago
(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/dsherk@mozilla.com-
> 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

6 years ago
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/dsherk@mozilla.com-0320e5612a50
(Reporter)

Comment 6

6 years ago
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.
Attachment #555875 - Flags: review?(bjacob) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 556129 [details] [diff] [review]
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.
Attachment #556129 - Flags: review?(bjacob)
(Reporter)

Comment 8

6 years ago
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/
Attachment #556129 - Flags: review?(bjacob) → review+
(Reporter)

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b9e81dedac48
http://hg.mozilla.org/mozilla-central/rev/b9e81dedac48
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.