Bug 48877 – [chromium] WebGL isFoo() tests fail in command buffer

• Component: WebGL
• Status: RESOLVED FIXED

[Adrienne Walker]

Created attachment 72748 [details]
Add this test to LayoutTests/fast/canvas/webgl

There are unexpected errors when running the attached file on WebKit r70788 (run with run-safari).  This works fine in Chromium at the same WebKit revision.  

I would expect gl.isBuffer(gl.createBuffer()) to return true.  Oddly, the object-deletion-behaviour.html should cover this test, but it doesn't.  Once fixed, perhaps this more simple test should be pushed to the conformance suite.

Here's the actual output from the test:

PASS buffer = gl.createBuffer() was expected value: NO_ERROR.
PASS framebuffer = gl.createFramebuffer() was expected value: NO_ERROR.
PASS program = wtu.setupSimpleTextureProgram(gl) was expected value: NO_ERROR.
PASS renderbuffer = gl.createRenderbuffer() was expected value: NO_ERROR.
PASS shader = gl.createShader(gl.VERTEX_SHADER) was expected value: NO_ERROR.
PASS texture = gl.createTexture() was expected value: NO_ERROR.
FAIL gl.isBuffer(buffer) should be true. Was false.
FAIL gl.isFramebuffer(framebuffer) should be true. Was false.
PASS gl.isProgram(program) is true
FAIL gl.isRenderbuffer(renderbuffer) should be true. Was false.
PASS gl.isShader(shader) is true
FAIL gl.isTexture(texture) should be true. Was false.
PASS successfullyParsed is true

TEST COMPLETE

[Kenneth Russell]

It looks like the reason is this sentence in the documentation for glIsBuffer:

"A name returned by glGenBuffers, but not yet associated with a buffer object by calling glBindBuffer, is not the name of a buffer object."

Chromium's command buffer OpenGL implementation is actually probably incorrect here. Chromium fails this test when run with --in-process-webgl. Either the WebGL implementation needs to be changed to always temporarily bind a newly created buffer so that it returns true from isBuffer, or the test needs to be updated to do so. Either way the command buffer OpenGL code probably (unfortunately) needs to change behavior to be correct.

[Adrienne Walker]

(In reply to comment #1)
> It looks like the reason is this sentence in the documentation for glIsBuffer:
> 
> "A name returned by glGenBuffers, but not yet associated with a buffer object by calling glBindBuffer, is not the name of a buffer object."

Thanks for the clarification.  It looks like the command buffer will need to be fixed.

[Kenneth Russell]

Because WebGL tries to track OpenGL behavior, I think that the conformance test should bind the objects it creates appropriately before making the is* queries. Adrienne, can you take care of this in the Khronos repo and pull the test down to the WebKit layout tests?

[Adrienne Walker]

(In reply to comment #3)
> Because WebGL tries to track OpenGL behavior, I think that the conformance test should bind the objects it creates appropriately before making the is* queries. Adrienne, can you take care of this in the Khronos repo and pull the test down to the WebKit layout tests?

I'm not sure I follow.  The above posted test was an example, that's not part of any existing test in the Khronos repository.

If you're asking that we fix the command buffer to follow this behavior and then add a test verifying that calling isBuffer on a non-bound buffer returns false, then I agree.

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #3)
> > Because WebGL tries to track OpenGL behavior, I think that the conformance test should bind the objects it creates appropriately before making the is* queries. Adrienne, can you take care of this in the Khronos repo and pull the test down to the WebKit layout tests?
> 
> I'm not sure I follow.  The above posted test was an example, that's not part of any existing test in the Khronos repository.

Ah. I thought this was one of the conformance tests in the Khronos repo.

> If you're asking that we fix the command buffer to follow this behavior and then add a test verifying that calling isBuffer on a non-bound buffer returns false, then I agree.

To be precise, a buffer that has never been bound. Once the name is bound, it becomes a buffer object. My thought is that we could add a test that allocates each type of object, binds / unbinds it once, and verifies that is* returns true for the object. We can add the test verifying that is* returns false before the object is bound once the command buffer code is fixed.

[Adrienne Walker]

Created attachment 73442 [details]
Patch

[Kenneth Russell]

Comment on attachment 73442 [details]
Patch

Good catch and looks good overall. Per Brian Paul's comments on the Mesa bug you filed, if real drivers have incorrect behavior (and it seems that some do) then we may have a problem. In this case we would need to make the test more conservative, and only call isBuffer, isFramebuffer, etc. after the first bind of the object.

Please add the test to the Khronos repo if you haven't already.

[WebKit Commit Bot]

Comment on attachment 73442 [details]
Patch

Clearing flags on attachment: 73442

Committed r71753: <http://trac.webkit.org/changeset/71753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Thanks.

(In reply to comment #7)
> (From update of attachment 73442 [details])
> Good catch and looks good overall. Per Brian Paul's comments on the Mesa bug you filed, if real drivers have incorrect behavior (and it seems that some do) then we may have a problem. In this case we would need to make the test more conservative, and only call isBuffer, isFramebuffer, etc. after the first bind of the object.

At the very least, the driver for the ATI card in my OS X machine had correct behavior.  We can certainly adjust this test later if this is a commonly mishandled edge case.

> Please add the test to the Khronos repo if you haven't already.

Done.

[Zhenyao Mo]

(In reply to comment #10)
> Thanks.
> 
> (In reply to comment #7)
> > (From update of attachment 73442 [details] [details])
> > Good catch and looks good overall. Per Brian Paul's comments on the Mesa bug you filed, if real drivers have incorrect behavior (and it seems that some do) then we may have a problem. In this case we would need to make the test more conservative, and only call isBuffer, isFramebuffer, etc. after the first bind of the object.
> 
> At the very least, the driver for the ATI card in my OS X machine had correct behavior.  We can certainly adjust this test later if this is a commonly mishandled edge case.

My Mac (Nvidia) also has the correct behavior, but my Windows (NVidia Quadro FX380) doesn't.  So my guess is Mac drivers get this correct, but not other platforms.

> 
> > Please add the test to the Khronos repo if you haven't already.
> 
> Done.