Bug 96578 – Add support for OES_vertex_array_object in chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Brandon Jones]

Add support for OES_vertex_array_object in chromium

[Brandon Jones]

Created attachment 163740 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Simon Hausmann]

Comment on attachment 163740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163740&action=review

> LayoutTests/ChangeLog:12
> +        * fast/canvas/webgl/oes-vertex-array-object-expected.txt: Added.
> +        * fast/canvas/webgl/oes-vertex-array-object.html: Added.
> +        * platform/chromium/fast/canvas/webgl/oes-vertex-array-object-expected.txt: Added.

Isn't there a high probability that this is going to fail right away on all the other platforms/bots, i.e. should it be skipped there?

[WebKit Review Bot]

Comment on attachment 163740 [details]
Patch

Attachment 163740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13848094

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/oes-vertex-array-object.html
fast/canvas/webgl/oes-vertex-array-object.html

[Brandon Jones]

Created attachment 164238 [details]
Patch

[Kenneth Russell]

Comment on attachment 164238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164238&action=review

This is good work so far but I'm concerned that there is a resource leak introduced with this patch; r-'ing it for that reason rather than leaving the r? bit alone. If it can be proven (and hopefully tested) that I'm wrong about this resource leak and I'm not available to re-review it (traveling on business for two days next week), dino could be a substitute reviewer. gman, benvanik, or zmo could be unofficial reviewers.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4398
> +    m_boundArrayBuffer->onAttached();

It looks to me like these newly introduced onAttached/onDetached calls are unbalanced and will prevent buffer objects from being deleted. In particular, consider the case where a VAO is allocated and bound, a vertexAttribPointer call using it is made, the VAO is unbound, the buffer object is deleted, and the VAO is deleted. My reading of the code is that the buffer object's m_attachmentCount will still be 1 at this point so the deletion will be deferred and never executed. There are missing calls to onDetached() in WebGLVertexArrayObjectOES::deleteObjectImpl. Note that WebGLFramebuffer, which is another type of container object, calls onDetached on all of its attachments in WebGLFramebuffer::deleteObjectImpl.

I hope that it is possible to add a test for this scenario using isBuffer(). After the buffer object is deleted but the VAO is still alive, isBuffer() might still return true -- not sure -- please test this. runDeleteTests() in the new layout test almost tests this, but I would specifically add a new test for it.

One way or another, these onAttached and onDetached calls should be encapsulated within the WebGLVertexArrayObjectOES class or its nested VertexAttribState struct, so that there aren't two distant places in the code which are calling attach and detach.

We should really come up with a better way to manage these lazy deletions via some sort of reference counted pointer (maybe a simple wrapper to RefPtr) but that should wait for another patch.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.cpp:83
> +        buffer->onAttached();

As far as I can tell, this introduces the same resource leak described above for element array buffers.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:124
> +    if (gl.getParameter(ext.VERTEX_ARRAY_BINDING_OES) == vao0) {

See comment below about equality comparisons on these kinds of objects.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:220
> +            if (buffer == state.buffer) {

I am not sure these kinds of object equality comparisons on JavaScript wrapper objects for C++ objects are guaranteed to work, at least not in all JavaScript engines. There was a long discussion about them at one point on the public_webgl mailing list. It would be safer to add an attribute you know about to the buffer objects you create, and do the comparisons on the value of that attribute.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:246
> +            if (elbuffer == state.elbuffer) {

Same comment about equality comparisons.

> LayoutTests/platform/chromium/TestExpectations:3610
> +BUGWK96578 : platform/chromium/virtual/gpu/fast/canvas/webgl/oes-vertex-array-object.html = PASS TEXT

Please file a new bug referencing this one indicating that these lines should be removed once the Chromium implementation of this extension lands, and use that bug's ID here.

> LayoutTests/platform/efl/Skipped:882
> +# http://webkit.org/b/96828

Please file one or more new bugs referencing this one indicating that this test should be unskipped when the extension is supported on this platform, and reference that bug ID here and in the other ports' Skipped lists.

[Brandon Jones]

(In reply to comment #6)
> It looks to me like these newly introduced onAttached/onDetached calls are unbalanced and will prevent buffer objects from being deleted...
> We should really come up with a better way to manage these lazy deletions via some sort of reference counted pointer (maybe a simple wrapper to RefPtr) but that should wait for another patch.

I agree. In this case the code pattern was lifted from the shader and program objects which also use onAttached and onDetached manually to manage ref counts. It feels like if we're going to introduce a more foolproof ref counting mechanism it should be applied to those objects as well. Would you consider that out-of-scope for this CL, or should we try to work it in?

[Kenneth Russell]

That refactoring is out of scope for this change. I would suggest doing it afterward, since otherwise this work will be blocked until it's complete.

[Brandon Jones]

(In reply to comment #6)
> I am not sure these kinds of object equality comparisons on JavaScript wrapper objects for C++ objects are guaranteed to work, at least not in all JavaScript engines... It would be safer to add an attribute you know about to the buffer objects you create, and do the comparisons on the value of that attribute.

I'm not sure I understand what you're getting at here. Presumably the concern is that get___ may return a different wrapper object than create___ did around the same underlying data, and those wrapper objects may fail a == test if implemented naively, correct? If so, I fail to see how you could possibly test this in Javascript at all. I think what you are suggesting is that we modify the test like so:

var buffer1 = gl.createBuffer();
buffer1.uid = getNextUniqueID();

//Some time later

var buffer2 = gl.getVertexAttrib(n, gl.VERTEX_ATTRIB_ARRAY_BUFFER_BINDING);
if(buffer1.uid == buffe2.uid) {
  // Success!
}

But attaching any javascript variables to the returned wrapper would, I'm fairly certain, fall victim to the same issue: The newly returned wrappers will not have my metadata attached.

Have I misunderstood the request?

[Brandon Jones]

Created attachment 164464 [details]
Patch

[Brandon Jones]

Created attachment 164472 [details]
Patch

[Kenneth Russell]

(In reply to comment #9)
> (In reply to comment #6)
> > I am not sure these kinds of object equality comparisons on JavaScript wrapper objects for C++ objects are guaranteed to work, at least not in all JavaScript engines... It would be safer to add an attribute you know about to the buffer objects you create, and do the comparisons on the value of that attribute.
> 
> I'm not sure I understand what you're getting at here. Presumably the concern is that get___ may return a different wrapper object than create___ did around the same underlying data, and those wrapper objects may fail a == test if implemented naively, correct? If so, I fail to see how you could possibly test this in Javascript at all. I think what you are suggesting is that we modify the test like so:
> 
> var buffer1 = gl.createBuffer();
> buffer1.uid = getNextUniqueID();
> 
> //Some time later
> 
> var buffer2 = gl.getVertexAttrib(n, gl.VERTEX_ATTRIB_ARRAY_BUFFER_BINDING);
> if(buffer1.uid == buffe2.uid) {
>   // Success!
> }
> 
> But attaching any javascript variables to the returned wrapper would, I'm fairly certain, fall victim to the same issue: The newly returned wrappers will not have my metadata attached.
> 
> Have I misunderstood the request?

You've understood my request.

Looking back, the issue I was remembering was related to uniform location objects, which are specifically defined to return a new object each time, so equality comparisons are not well defined:

https://www.khronos.org/webgl/public-mailing-list/archives/1204/msg00295.html

Never mind my request here, since buffer objects are persistent and guaranteed to return the same object and JavaScript wrapper object if it's still reachable.

[James Robinson]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > I am not sure these kinds of object equality comparisons on JavaScript wrapper objects for C++ objects are guaranteed to work, at least not in all JavaScript engines... It would be safer to add an attribute you know about to the buffer objects you create, and do the comparisons on the value of that attribute.
> > 
> > I'm not sure I understand what you're getting at here. Presumably the concern is that get___ may return a different wrapper object than create___ did around the same underlying data, and those wrapper objects may fail a == test if implemented naively, correct? If so, I fail to see how you could possibly test this in Javascript at all. I think what you are suggesting is that we modify the test like so:
> > 
> > var buffer1 = gl.createBuffer();
> > buffer1.uid = getNextUniqueID();
> > 
> > //Some time later
> > 
> > var buffer2 = gl.getVertexAttrib(n, gl.VERTEX_ATTRIB_ARRAY_BUFFER_BINDING);
> > if(buffer1.uid == buffe2.uid) {
> >   // Success!
> > }
> > 
> > But attaching any javascript variables to the returned wrapper would, I'm fairly certain, fall victim to the same issue: The newly returned wrappers will not have my metadata attached.
> > 
> > Have I misunderstood the request?
> 
> You've understood my request.
> 
> Looking back, the issue I was remembering was related to uniform location objects, which are specifically defined to return a new object each time, so equality comparisons are not well defined:
> 
> https://www.khronos.org/webgl/public-mailing-list/archives/1204/msg00295.html
> 
> Never mind my request here, since buffer objects are persistent and guaranteed to return the same object and JavaScript wrapper object if it's still reachable.

Hmm, so you want all calls to gl.getVertexAttrib(...) to return the same wrapper?  That would mean you'd have to keep the JS wrapper alive via C++ in some way, right?  If so, then how would you handle this case:

function makeACycle(gl) {
  var buffer1 = gl.createBuffer();
  buffer1.nastyExpando = gl;
}

makeACycle(gl);


Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.

[Brandon Jones]

(In reply to comment #13) 
> Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.

That's sensible, but the real question in this context is if two distinct wrapper objects for the same underlying GL resource will be comparable:

var buffer1 = gl.createBuffer();
gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
if(buffer1 == buffer2) {
  // Success?
}

Most developers will expect the above to work (including myself up until I read Ken's comments). Does it?

Bringing the focus back to the patch, however, this question only affects wether the conformance test is valid on all platforms. If not, then there are other tests that need to be adjusted to compensate (https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/conformance/state/gl-object-get-calls.html) Beyond that, this particular unit test is skipped on most platforms and expected to be flaky on those that it's enabled on due to limits of the testing framework. If it's the only thing that's holding this CL back perhaps it's more prudent to remove the test until a better solution can be found?

[James Robinson]

(In reply to comment #14)
> (In reply to comment #13) 
> > Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.
> 
> That's sensible, but the real question in this context is if two distinct wrapper objects for the same underlying GL resource will be comparable:
> 
> var buffer1 = gl.createBuffer();
> gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
> if(buffer1 == buffer2) {
>   // Success?
> }
> 
> Most developers will expect the above to work (including myself up until I read Ken's comments). Does it?

they should === false, for sure.  I also don't think they should have the expandos, so I'd guess == would be false.  I'm less confident about when == and === should differ in JS.

Logically buffer1 and buffer 2 are different things.

> 
> Bringing the focus back to the patch, however, this question only affects wether the conformance test is valid on all platforms. If not, then there are other tests that need to be adjusted to compensate (https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/conformance/state/gl-object-get-calls.html) Beyond that, this particular unit test is skipped on most platforms and expected to be flaky on those that it's enabled on due to limits of the testing framework. If it's the only thing that's holding this CL back perhaps it's more prudent to remove the test until a better solution can be found?

[Kenneth Russell]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13) 
> > > Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.
> > 
> > That's sensible, but the real question in this context is if two distinct wrapper objects for the same underlying GL resource will be comparable:
> > 
> > var buffer1 = gl.createBuffer();
> > gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> > var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
> > if(buffer1 == buffer2) {
> >   // Success?
> > }
> > 
> > Most developers will expect the above to work (including myself up until I read Ken's comments). Does it?
> 
> they should === false, for sure.  I also don't think they should have the expandos, so I'd guess == would be false.  I'm less confident about when == and === should differ in JS.
> 
> Logically buffer1 and buffer 2 are different things.

No, they should be == and ===. They are referring to the same underlying C++ object. So long as the JS wrapper for the underlying C++ WebGLBuffer object hasn't been GC'd (which it won't be, because the buffer1 reference keeps it alive), the fact that the gl.getParameter call returns the same WebGLBuffer object will cause the bindings to return the same wrapper. Note the following, which does the same sorts of queries against the DOM:

> var j = document.createElement("div");
undefined
> j
<div>​</div>​
> var p1 = document.createElement("p");
undefined
> p1.innerHTML = "Hello";
"Hello"
> p1
<p>​Hello​</p>​
> j.appendChild(p1);
<p>​Hello​</p>
> j
<div>​
<p>Hello</p>​
</div>​
> j.childNodes[0]
<p>Hello</p>​
> j.childNodes[0] == p1
true
> j.childNodes[0] === p1
true


> > 
> > Bringing the focus back to the patch, however, this question only affects wether the conformance test is valid on all platforms. If not, then there are other tests that need to be adjusted to compensate (https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/conformance/state/gl-object-get-calls.html) Beyond that, this particular unit test is skipped on most platforms and expected to be flaky on those that it's enabled on due to limits of the testing framework. If it's the only thing that's holding this CL back perhaps it's more prudent to remove the test until a better solution can be found?

The test is fine. My original comment was wrong. Let's proceed with the patch.

[James Robinson]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13) 
> > > > Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.
> > > 
> > > That's sensible, but the real question in this context is if two distinct wrapper objects for the same underlying GL resource will be comparable:
> > > 
> > > var buffer1 = gl.createBuffer();
> > > gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> > > var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
> > > if(buffer1 == buffer2) {
> > >   // Success?
> > > }
> > > 
> > > Most developers will expect the above to work (including myself up until I read Ken's comments). Does it?
> > 
> > they should === false, for sure.  I also don't think they should have the expandos, so I'd guess == would be false.  I'm less confident about when == and === should differ in JS.
> > 
> > Logically buffer1 and buffer 2 are different things.
> 
> No, they should be == and ===. They are referring to the same underlying C++ object. So long as the JS wrapper for the underlying C++ WebGLBuffer object hasn't been GC'd (which it won't be, because the buffer1 reference keeps it alive), the fact that the gl.getParameter call returns the same WebGLBuffer object will cause the bindings to return the same wrapper. Note the following, which does the same sorts of queries against the DOM:
> 

I see, that makes sense.  What about expandos on buffer1?  i.e. what if I do:

function one() {
  var buffer1 = gl.createBuffer();
  buffer1.expando = "happy";
  gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
}

function two() {
  var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
  window.alert(buffer2.expando);
}

one();
two();

what do I get?

[Kenneth Russell]

Comment on attachment 164472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164472&action=review

Thanks, this is much easier to understand.

What about attempting to explicitly test the lifetime of buffer objects referred to by VAOs? In particular, if a buffer object was created, a VAO pointed to it, the buffer deleted, and the VAO deleted? isBuffer should return false, I believe, and I think the earlier version of the patch would have failed that test.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.cpp:92
> +        buffer->onAttached();

Should the "if (buffer) buffer->onAttached();" go at the top of the function? Otherwise, if the buffer's been deleted, then setting the element array buffer to the same buffer object will cause it to be deleted.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.cpp:105
> +        buffer->onAttached();

Same comment here.

[Kenneth Russell]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > (In reply to comment #13) 
> > > > > Since that doesn't work, you need to make the wrapper collectable from JS - but you don't want to expose when GC runs.  This is why I think it makes more sense to always return a new wrapper object from get...() calls.
> > > > 
> > > > That's sensible, but the real question in this context is if two distinct wrapper objects for the same underlying GL resource will be comparable:
> > > > 
> > > > var buffer1 = gl.createBuffer();
> > > > gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> > > > var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
> > > > if(buffer1 == buffer2) {
> > > >   // Success?
> > > > }
> > > > 
> > > > Most developers will expect the above to work (including myself up until I read Ken's comments). Does it?
> > > 
> > > they should === false, for sure.  I also don't think they should have the expandos, so I'd guess == would be false.  I'm less confident about when == and === should differ in JS.
> > > 
> > > Logically buffer1 and buffer 2 are different things.
> > 
> > No, they should be == and ===. They are referring to the same underlying C++ object. So long as the JS wrapper for the underlying C++ WebGLBuffer object hasn't been GC'd (which it won't be, because the buffer1 reference keeps it alive), the fact that the gl.getParameter call returns the same WebGLBuffer object will cause the bindings to return the same wrapper. Note the following, which does the same sorts of queries against the DOM:
> > 
> 
> I see, that makes sense.  What about expandos on buffer1?  i.e. what if I do:
> 
> function one() {
>   var buffer1 = gl.createBuffer();
>   buffer1.expando = "happy";
>   gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> }
> 
> function two() {
>   var buffer2 = gl.getParameter(gl.ARRAY_BUFFER_BINDING);
>   window.alert(buffer2.expando);
> }
> 
> one();
> two();
> 
> what do I get?

The expandos should be preserved, because the same JS wrapper should be being returned -- but I am not 100% sure that the WebGL implementation is guaranteeing that right now. (In particular, if a GC is forced between the calls to one() and two().) It's definitely guaranteed for extension objects -- you'll recall that a while back you reviewed the implementation of WebGLRenderingContext.getExtension() and caught bugs in this area.

If it turns out there is an issue like this, then the behavior needs to be clarified in the WebGL spec.

[James Robinson]

Wait, now I'm back to confused.  In this case:

function foo() {
  var buffer = gl.createBuffer();
  buffer.nastyExpando = gl;
  gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
}


are you saying that you expect the JS wrapper around buffer to be held alive?  In this case the wrapper is holding a ref to the context itself, so I don't know how anything will ever be torn down without the javascript garbage collector having knowledge of the relationship between the buffer's wrapper and the context's wrapper (like we do for DOM).  My understanding of how to fix this problem is that we agreed that all functions that query getters off of the context object itself have to return a fresh wrapper.  Maybe there's another solution?

[Kenneth Russell]

(In reply to comment #20)
> Wait, now I'm back to confused.  In this case:
> 
> function foo() {
>   var buffer = gl.createBuffer();
>   buffer.nastyExpando = gl;
>   gl.bindBuffer(gl.ARRAY_BUFFER, buffer1);
> }
> 
> 
> are you saying that you expect the JS wrapper around buffer to be held alive?  In this case the wrapper is holding a ref to the context itself, so I don't know how anything will ever be torn down without the javascript garbage collector having knowledge of the relationship between the buffer's wrapper and the context's wrapper (like we do for DOM).  My understanding of how to fix this problem is that we agreed that all functions that query getters off of the context object itself have to return a fresh wrapper.  Maybe there's another solution?

Many of the queries in the WebGL spec are defined to return new objects; getUniformLocation, getShaderPrecisionFormat, all getParameter calls returning sequences or typed arrays, etc. In these situations new wrappers are returned. However, for get calls which return a pointer to a preexisting OpenGL handle like a buffer or texture, they are going to return the same C++ object that was previously set, similar to how the DOM works.

Creating a JS reference from the buffer to the context won't (or shouldn't, at least) prevent the buffer or context from being cleaned up. The bindings hold only weak references from the C++ objects to their JS wrappers, so there is no reference cycle between JS and C++.

Right now the WebGL spec doesn't guarantee that the associated JS wrapper object will not be collected, so the expando might disappear during the next GC, or it might not. If we strongly care about making this behavior consistent among WebGL implementations then we should change the spec and implementation so that the C++ objects have a strong reference to their JS wrappers, and I believe that that would in fact require the C++ objects to tell the JS GC about that relationship. I'm not in favor of making this change since as far as I know it hasn't been raised as a problem by any developer.

[James Robinson]

Observable JS GC is a really difficult problem to attempt to patch later and will definitely trip someone up sooner or later. You really have to design to avoid issues or you're in for a world of hurt sometime down the road.

[Kenneth Russell]

(In reply to comment #22)
> Observable JS GC is a really difficult problem to attempt to patch later and will definitely trip someone up sooner or later. You really have to design to avoid issues or you're in for a world of hurt sometime down the road.

I agree with you that we should look more deeply into the issue with WebGL extensions requiring that the same object be returned each time, and whether there is any resource leak there. However, Brandon's patch definitely doesn't introduce any new problems, so let's proceed with landing this first.

[Kenneth Russell]

Comment on attachment 164472 [details]
Patch

Clearing the review bit on this patch so others don't get confused about its state.

[James Robinson]

(In reply to comment #23)
> (In reply to comment #22)
> > Observable JS GC is a really difficult problem to attempt to patch later and will definitely trip someone up sooner or later. You really have to design to avoid issues or you're in for a world of hurt sometime down the road.
> 
> I agree with you that we should look more deeply into the issue with WebGL extensions requiring that the same object be returned each time, and whether there is any resource leak there. However, Brandon's patch definitely doesn't introduce any new problems, so let's proceed with landing this first.

SGTM!

[Brandon Jones]

Created attachment 165206 [details]
Patch

[Brandon Jones]

Created attachment 165209 [details]
Patch

[Kenneth Russell]

Comment on attachment 165209 [details]
Patch

Looks good. Thanks for enhancing the test. r=me. Marking this cq+ since we've discussed it offline, but for future reference set the cq? bit if you want your patches submitted to the commit queue by the reviewer.

[WebKit Review Bot]

Comment on attachment 165209 [details]
Patch

Clearing flags on attachment: 165209

Committed r129275: <http://trac.webkit.org/changeset/129275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.