Closed
Bug 1320030
Opened 8 years ago
Closed 8 years ago
Failure in conformance/extensions/oes-vertex-array-object.html
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: daoshengmu, Assigned: jgilbert)
References
Details
Attachments
(1 file, 4 obsolete files)
11 bytes,
text/plain
|
jgilbert
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
FAIL Position buffer should no longer exist after last ref removed FAIL Color buffer should no longer exist after last ref removed FAIL Position buffer should no longer exist after last ref removed FAIL Color buffer should no longer exist after last ref removed FAIL Position buffer should no longer exist after last ref removed FAIL Color buffer should no longer exist after last ref removed The root cause is because the WebGLBuffer for Position and Color call WebGLVertexAttribData::VertexAttribPointer several times, but just be deleted once. So after calling DeleteBuffer(), they still exist.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → dmu
Reporter | ||
Comment 1•8 years ago
|
||
That is because https://github.com/KhronosGroup/WebGL/blob/bb6fcfa23c0fcb51f4891d5998a9254dde6c327a/sdk/tests/conformance/extensions/oes-vertex-array-object.html#L605 set Position and Color multiple times but we only delete it once here, https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/dom/canvas/WebGLContextBuffers.cpp#507.
Reporter | ||
Comment 2•8 years ago
|
||
In the spec, https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5, - deleteBuffer() "Delete the buffer object contained in the passed WebGLBuffer as if by calling glDeleteBuffers." - isBuffer() "Return true if the passed WebGLBuffer is valid and false otherwise." In our implementation, we make RefCount in WebGLBuffer minus one, when someone calls deleteBuffer(). After RefCount is zero, then calling fDeleteBuffers() and WebGLBuffer will be invalid. I think it doesn't conflict with the spec. If we move gl.isBuffer(positionBuffer) and gl.isBuffer(colorBuffer) to be out of the for-loop, it will return false, and it will get pass. Should we ask for modifying the test?
Flags: needinfo?(ethlin)
Comment 3•8 years ago
|
||
I think we should clear all attrib's buffers when calling deleteBuffer(). Currently we only clear mBoundVertexArray's buffer[1]. We should do fnClearIfBuffer for all mVertexArrays[i]->mAttribs. [1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextBuffers.cpp#520
Flags: needinfo?(ethlin)
Reporter | ||
Comment 4•8 years ago
|
||
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=aed3226b9a84213bb62fcec26269beff23fad42f
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8814838 [details] Bug 1320030 - Clear away webgl buffer while vertex array object is bound; https://reviewboard.mozilla.org/r/95960/#review96268 ::: dom/canvas/WebGLContextBuffers.cpp:520 (Diff revision 2) > fnClearIfBuffer(binding.mBufferBinding); > } > } > > + bool bBounded = false; > for (int32_t i = 0; i < mGLMaxVertexAttribs; i++) { mVertexArrays should contain mBoundVertexArray. If we check all buffers in mVertexArrays, we probably don't need to check mBoundVertexArray first. I'm not sure why we need the variable 'bBounded'. Can we just clear all buffers with the same pointer in mVertexArrays? ::: dom/canvas/WebGLContextBuffers.cpp:522 (Diff revision 2) > } > > + bool bBounded = false; > for (int32_t i = 0; i < mGLMaxVertexAttribs; i++) { > - if (mBoundVertexArray->HasAttrib(i)) { > + if (mBoundVertexArray->HasAttrib(i) && > + mBoundVertexArray->mAttribs[i].mBuf == buffer) { The check of pointer is in fnClearIfBuffer already. ::: dom/canvas/WebGLContextBuffers.cpp:534 (Diff revision 2) > + for (auto iter = mVertexArrays.begin(); > + iter != mVertexArrays.end(); ++iter) { > + WebGLVertexArray* vertexArray = (*iter); > + for (int32_t i = 0; i < mGLMaxVertexAttribs; i++) { > + if (vertexArray->HasAttrib(i)) { > + if (vertexArray->mAttribs[i].mBuf == buffer) { The check of pointer is in fnClearIfBuffer already.
Comment 8•8 years ago
|
||
Sorry, I was wrong. Spec GLES 3.0.4 p33 says: "If a buffer object is deleted while it is bound, all bindings to that object in the current context (i.e. in the thread that called DeleteBuffers) are reset to zero."
Comment hidden (mozreview-request) |
Reporter | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8814838 [details] Bug 1320030 - Clear away webgl buffer while vertex array object is bound; https://reviewboard.mozilla.org/r/95960/#review96268 > mVertexArrays should contain mBoundVertexArray. If we check all buffers in mVertexArrays, we probably don't need to check mBoundVertexArray first. I'm not sure why we need the variable 'bBounded'. Can we just clear all buffers with the same pointer in mVertexArrays? We need to check the vertex array is bound at mBoundVertexArray, then we can delete the vertex buffer.
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8814838 [details] Bug 1320030 - Clear away webgl buffer while vertex array object is bound; https://reviewboard.mozilla.org/r/95960/#review96578 If this is the fix, the test is wrong, because our behavior is correct as far as I can tell. GLES3.0.5 p317: Attachments to unbound container objects, such as deletion of a buffer attached to a vertex array object which is not bound to the context, are not affected and continue to act as references on the deleted object, as described in the following section.
Attachment #8814838 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 12•8 years ago
|
||
The test is wrong. It expects a second call to deleteBuffers to again search for and unbind attachment points. This is not what is intended by the spec.
Assignee | ||
Updated•8 years ago
|
Assignee: dmu → jgilbert
Assignee | ||
Comment 13•8 years ago
|
||
Spec clarification is: https://github.com/KhronosGroup/WebGL/pull/2178
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8815908 [details] Bug 1320030 - Handle program and shader object deletion differently. - https://reviewboard.mozilla.org/r/96694/#review97042
Attachment #8815908 -
Flags: review?(ethlin) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8815582 [details] Bug 1320030 - Simplify marking and deletion checks. - https://reviewboard.mozilla.org/r/96456/#review97048
Attachment #8815582 -
Flags: review?(ethlin) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8814838 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Summary: [WebGL1 conformance test]Pass conformance/extensions/oes-vertex-array-object.html → Failure in conformance/extensions/oes-vertex-array-object.html
Comment 20•8 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a7e352abec Simplify marking and deletion checks. - r=ethlin https://hg.mozilla.org/integration/mozilla-inbound/rev/513dde2c6720 Cherry-pick tests. https://hg.mozilla.org/integration/mozilla-inbound/rev/6611438aed06 Handle program and shader object deletion differently. - r=ethlin https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cbc261fc0 Mark tests.
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7a7e352abec https://hg.mozilla.org/mozilla-central/rev/513dde2c6720 https://hg.mozilla.org/mozilla-central/rev/6611438aed06 https://hg.mozilla.org/mozilla-central/rev/2d9cbc261fc0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Updated•7 years ago
|
status-firefox51:
--- → affected
status-firefox52:
--- → affected
Assignee | ||
Comment 22•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: webgl2 [User impact if declined]: [Is this code covered by automated tests?]: [Has the fix been verified in Nightly?]: [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: [Is the change risky?]: [Why is the change risky/not risky?]: [String changes made/needed]:
Attachment #8815582 -
Attachment is obsolete: true
Attachment #8815907 -
Attachment is obsolete: true
Attachment #8815908 -
Attachment is obsolete: true
Attachment #8818747 -
Flags: review+
Attachment #8818747 -
Flags: approval-mozilla-beta?
Attachment #8818747 -
Flags: approval-mozilla-aurora?
Comment 23•7 years ago
|
||
Comment on attachment 8818747 [details]
placeholder for all patches
WebGL2 related patch. Beta51+ & Aurora52+. Should be in 51 beta 8.
Attachment #8818747 -
Flags: approval-mozilla-beta?
Attachment #8818747 -
Flags: approval-mozilla-beta+
Attachment #8818747 -
Flags: approval-mozilla-aurora?
Attachment #8818747 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/630ad73cc24b
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 26•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/b86bbd7cd5f5
status-firefox50:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•