Bug 74741

Summary: Postpone deleteRenderbuffer/deleteTexture until all framebuffer attachment points are removed.
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, kbr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch kbr: review+

Description Zhenyao Mo 2011-12-16 12:33:09 PST 
According to GLES2 spec, when deleteRenderbuffer/deleteTexture is called, and if the renderbuffer/texture is attached to a un-bound framebuffer, than the image of the renderbuffer/texture is not removed.
Comment 1 Zhenyao Mo 2011-12-16 12:52:35 PST 
Created attachment 119655 [details]
Patch
Comment 2 Zhenyao Mo 2011-12-16 13:56:16 PST 
Bots are green, please review.
Comment 3 Kenneth Russell 2011-12-16 15:56:48 PST 
Comment on attachment 119655 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:245
> +            gc3d->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::RENDERBUFFER, 0);

These calls assume that this framebuffer object is bound to the FRAMEBUFFER binding point at the time they're called. This may be guaranteed when the WebGLRenderingContext calls the setAttachment method, but not when deleteRenderbuffer, deleteTexture, etc. is called -- and these methods seem to be the ones that call removeAttachment(WebGLObject*).

I don't understand the logic here regardless. I thought the desired behavior was to defer the deletion of the WebGLTexture, WebGLRenderbuffer, etc. until it was detached from all live framebuffers?
Comment 4 Zhenyao Mo 2011-12-16 17:22:52 PST 
(In reply to comment #3)
> (From update of attachment 119655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119655&action=review
> 
> > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:245
> > +            gc3d->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::RENDERBUFFER, 0);
> 
> These calls assume that this framebuffer object is bound to the FRAMEBUFFER binding point at the time they're called. This may be guaranteed when the WebGLRenderingContext calls the setAttachment method, but not when deleteRenderbuffer, deleteTexture, etc. is called -- and these methods seem to be the ones that call removeAttachment(WebGLObject*).

In deleteRenderbuffer/deleteTexture, removeAttachment() is always called for the current bound framebuffer (if it's not null).  So I think the logic here is correct.

> 
> I don't understand the logic here regardless. I thought the desired behavior was to defer the deletion of the WebGLTexture, WebGLRenderbuffer, etc. until it was detached from all live framebuffers?

Yes, the deletion is deferred by onAttached() on the object (attachment count ++), and the true deletion is only happening when onDetached() is called on the object().  The deferred deletion logic is already implemented for shader/program/framebuffer objects in WebGLObject, now we extend the same mechanism to renderbuffer/texture object.

Please have another look.
Comment 5 Kenneth Russell 2011-12-19 12:38:17 PST 
Comment on attachment 119655 [details]
Patch

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

Thanks for the explanation. Looks good overall but please resolve the naming issue and various minor test issues before committing. r=me

>>> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:245
>>> +            gc3d->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::RENDERBUFFER, 0);
>> 
>> These calls assume that this framebuffer object is bound to the FRAMEBUFFER binding point at the time they're called. This may be guaranteed when the WebGLRenderingContext calls the setAttachment method, but not when deleteRenderbuffer, deleteTexture, etc. is called -- and these methods seem to be the ones that call removeAttachment(WebGLObject*).
>> 
>> I don't understand the logic here regardless. I thought the desired behavior was to defer the deletion of the WebGLTexture, WebGLRenderbuffer, etc. until it was detached from all live framebuffers?
> 
> In deleteRenderbuffer/deleteTexture, removeAttachment() is always called for the current bound framebuffer (if it's not null).  So I think the logic here is correct.

OK, thanks, I see now, but this is pretty subtle. Given the new semantics I strongly think you should rename this method to removeAttachmentFromCurrentlyBoundFramebuffer() or similar and update the comment in WebGLFramebuffer.h.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:29
> +shouldBeTrue("program != null");

You can use shouldBeNonNull(program), here and throughout.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:181
> +  shouldBeFalse("gl.checkFramebufferStatus(gl.FRAMEBUFFER) == gl.FRAMEBUFFER_COMPLETE");

Can use shouldNotBe here.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:188
> +  shouldBeTrue("gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) == rbo");

Can use shouldBe here.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:246
> +  shouldBeTrue("gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE");

Use shouldNotBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:253
> +  shouldBeTrue("gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) == rbo");

Use shouldBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:258
> +  shouldBeTrue("gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE");

shouldNotBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:282
> +  shouldBeTrue("gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE");

shouldNotBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:285
> +  shouldBeTrue("gl.checkFramebufferStatus(gl.FRAMEBUFFER) == gl.FRAMEBUFFER_COMPLETE");

shouldBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:290
> +  shouldBeTrue("gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) == tex");

shouldBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:294
> +  shouldBeTrue("gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE");

shouldNotBe.

> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:410
> +  // check again because many buggy implementations will have bound to the true backbuffer on deleteFramebuffer.

I don't understand this comment nor what the test below is trying to achieve. Upon deleteFramebuffer of the bound FBO the implementation should revert back to the default framebuffer.
Comment 6 Zhenyao Mo 2011-12-19 14:14:12 PST 
Comment on attachment 119655 [details]
Patch

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

>>>> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:245
>>>> +            gc3d->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::RENDERBUFFER, 0);
>>> 
>>> These calls assume that this framebuffer object is bound to the FRAMEBUFFER binding point at the time they're called. This may be guaranteed when the WebGLRenderingContext calls the setAttachment method, but not when deleteRenderbuffer, deleteTexture, etc. is called -- and these methods seem to be the ones that call removeAttachment(WebGLObject*).
>>> 
>>> I don't understand the logic here regardless. I thought the desired behavior was to defer the deletion of the WebGLTexture, WebGLRenderbuffer, etc. until it was detached from all live framebuffers?
>> 
>> In deleteRenderbuffer/deleteTexture, removeAttachment() is always called for the current bound framebuffer (if it's not null).  So I think the logic here is correct.
> 
> OK, thanks, I see now, but this is pretty subtle. Given the new semantics I strongly think you should rename this method to removeAttachmentFromCurrentlyBoundFramebuffer() or similar and update the comment in WebGLFramebuffer.h.

Done and I add a ASSERT to make sure the function is called upon the bound framebuffer.

>> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:181
>> +  shouldBeFalse("gl.checkFramebufferStatus(gl.FRAMEBUFFER) == gl.FRAMEBUFFER_COMPLETE");
> 
> Can use shouldNotBe here.

Done and all the belowing.  We should be more formal for khronos side tests code review.  Otherwise everytime we  try to sync with it, we end up cleaning issues in some tests.

>> LayoutTests/fast/canvas/webgl/object-deletion-behaviour.html:410
>> +  // check again because many buggy implementations will have bound to the true backbuffer on deleteFramebuffer.
> 
> I don't understand this comment nor what the test below is trying to achieve. Upon deleteFramebuffer of the bound FBO the implementation should revert back to the default framebuffer.

My guess is to check bindFramebuffer(null) bind to the WebGL's backbuffer, instead of trully calling glBindFramebuffer(null).  I think this test is checking a potential bug and we should keep it.
Comment 7 Zhenyao Mo 2011-12-19 15:03:16 PST 
Committed r103272: <http://trac.webkit.org/changeset/103272>