Closed Bug 1320030 Opened 3 years ago Closed 3 years ago

Failure in conformance/extensions/oes-vertex-array-object.html

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: daoshengmu, Assigned: jgilbert)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → dmu
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)
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)
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.
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 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.
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-
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: dmu → jgilbert
See Also: → 1288643
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 on attachment 8815582 [details]
Bug 1320030 - Simplify marking and deletion checks. -

https://reviewboard.mozilla.org/r/96456/#review97048
Attachment #8815582 - Flags: review?(ethlin) → review+
Attachment #8814838 - Attachment is obsolete: true
Summary: [WebGL1 conformance test]Pass conformance/extensions/oes-vertex-array-object.html → Failure in conformance/extensions/oes-vertex-array-object.html
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 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+
needs rebasing for aurora
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.