Bug 43942 – Textures and renderbuffers should be detached first before deletion if they are attached to framebuffers

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

We implement the delayed delete for shaders and programs.  We should do the same for textures and renderbuffers.

[Zhenyao Mo]

According to GLES2 spec, textures and renderbuffers should be detached first before deletion if they are attached to the currently-bound framebuffer; for any other framebuffers, the detachment is the responsibility of the application.

I just sent out an email to the public_webgl email list, suggesting that we detach before delete no matter a framebuffer is bound or not.  Let's wait until we hear some feedback.

[Zhenyao Mo]

Created attachment 67585 [details]
patch

The test will be uploaded to khronos after review (I also sent it to gman@google to review the test).

This patch fixes some bugs in object deletion behavior.  As for textures and renderbuffers, we only remove the attachment from the currently-bound framebuffer.  This will cause data inconsistency when we delete a texture/rbo that's bound to a non-currently-bound fbo and later query the currently bound object name of that fbo.  I guess we'll have to live with this bug.  At least the buggy behavior will be the same for webgl and gles 2.

[Kenneth Russell]

After the discussion on the public_webgl mailing list and side discussions with TransGaming about the behavior of object deletion when the object is still attached to another one (for example, the case where a texture is deleted, but is still the color attachment of a framebuffer), I think that the behavior should be slightly different. Consider the case where texture T is attached to framebuffer F, deleteTexture is called on T, and then the name of the color attachment of F is queried. Because T was attached to F at the time it was deleted, the texture object isn't actually deleted, but its name is immediately marked unused. Calls which generate new texture names might return this name.

I think the behavior should be that as soon as the texture is deleted, the WebGL implementation will no longer return its associated WebGLTexture* from any query calls, instead returning NULL. The entry in the internal table mapping that name to the WebGLTexture* needs to be either removed or somehow marked as invalid. Any subsequent creation operation which causes that name to be reused should just overwrite the entry in the table, regardless of whether the orphaned object is still alive. We should not track multiple versions of the same name.

[Zhenyao Mo]

The current behavior is this:  if a texture T is attached to framebuffer F (F is not currently bound) and T is deleted, now if query the attached object name of F, if the original name of T is not reused yet, null is returned; if the original name of T is reused, the new texture using the name is returned.  The latter case is buggy, but GLES 2 will behave the same.

[Zhenyao Mo]

(In reply to comment #3)
> After the discussion on the public_webgl mailing list and side discussions with TransGaming about the behavior of object deletion when the object is still attached to another one (for example, the case where a texture is deleted, but is still the color attachment of a framebuffer), I think that the behavior should be slightly different. Consider the case where texture T is attached to framebuffer F, deleteTexture is called on T, and then the name of the color attachment of F is queried. Because T was attached to F at the time it was deleted, the texture object isn't actually deleted, but its name is immediately marked unused. Calls which generate new texture names might return this name.
> 
> I think the behavior should be that as soon as the texture is deleted, the WebGL implementation will no longer return its associated WebGLTexture* from any query calls, instead returning NULL. The entry in the internal table mapping that name to the WebGLTexture* needs to be either removed or somehow marked as invalid. Any subsequent creation operation which causes that name to be reused should just overwrite the entry in the table, regardless of whether the orphaned object is still alive. We should not track multiple versions of the same name.

Forget to mention, the texture name is always removed immediately upon deleteTexture call no matter what, so there will be no situation for multiple versions of the same name.

[Kenneth Russell]

Comment on attachment 67585 [details]
patch

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

I think the code looks generally good. r- for a documentation issue as well as one potential code cleanup I'd like you to think about. Also, please check in the new test to the Khronos conformance suite.

> WebCore/html/canvas/WebGLObject.h:66
>          if (!m_attachmentCount && m_deleted)

The test of m_deleted seems to be no longer necessary since it's being tested in deleteObjectImpl.

> WebCore/html/canvas/WebGLObject.h:73
>      virtual void deleteObjectImpl(Platform3DObject) = 0;

Please document that this may be called multiple times for the same object, and that isDeleted() needs to be tested in implementations when deciding whether to delete the OpenGL resource.

[Zhenyao Mo]

(In reply to comment #6)
> (From update of attachment 67585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67585&action=review
> 
> I think the code looks generally good. r- for a documentation issue as well as one potential code cleanup I'd like you to think about. Also, please check in the new test to the Khronos conformance suite.
> 
> > WebCore/html/canvas/WebGLObject.h:66
> >          if (!m_attachmentCount && m_deleted)
> 
> The test of m_deleted seems to be no longer necessary since it's being tested in deleteObjectImpl.

Per our off-line discussion, m_deleted will be left here for readability because detach and delete are two different events.

> 
> > WebCore/html/canvas/WebGLObject.h:73
> >      virtual void deleteObjectImpl(Platform3DObject) = 0;
> 
> Please document that this may be called multiple times for the same object, and that isDeleted() needs to be tested in implementations when deciding whether to delete the OpenGL resource.

Will add.

[Zhenyao Mo]

Created attachment 68944 [details]
revised patch

[Zhenyao Mo]

Created attachment 68946 [details]
revised patch: fix a bug in the comments.

The test has been committed into Khronos already.

[Kenneth Russell]

Comment on attachment 68946 [details]
revised patch: fix a bug in the comments.

Looks good. Thanks for persevering on this fix.

[Zhenyao Mo]

Committed r68424: <http://trac.webkit.org/changeset/68424>