Add support for OES_vertex_array_object in chromium
Created attachment 163740 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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?
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
Created attachment 164238 [details] Patch
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.
(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?
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.
(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?
Created attachment 164464 [details] Patch
Created attachment 164472 [details] Patch
(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.
(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.
(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?
(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?
(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.
(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?
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.
(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.
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?
(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.
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.
(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.
Comment on attachment 164472 [details] Patch Clearing the review bit on this patch so others don't get confused about its state.
(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!
Created attachment 165206 [details] Patch
Created attachment 165209 [details] Patch
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.
Comment on attachment 165209 [details] Patch Clearing flags on attachment: 165209 Committed r129275: <http://trac.webkit.org/changeset/129275>
All reviewed patches have been landed. Closing bug.