Bug 74741 - Postpone deleteRenderbuffer/deleteTexture until all framebuffer attachment points are removed.
: Postpone deleteRenderbuffer/deleteTexture until all framebuffer attachment po...
Status: RESOLVED FIXED
: WebKit
WebGL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-12-16 12:33 PST by
Modified: 2011-12-19 15:03 PST (History)


Attachments
Patch (51.44 KB, patch)
2011-12-16 12:52 PST, Zhenyao Mo
kbr: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-12-16 12:52:35 PST -------
Created an attachment (id=119655) [details]
Patch
------- Comment #2 From 2011-12-16 13:56:16 PST -------
Bots are green, please review.
------- Comment #3 From 2011-12-16 15:56:48 PST -------
(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*).

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 From 2011-12-16 17:22:52 PST -------
(In reply to comment #3)
> (From update of attachment 119655 [details] [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 From 2011-12-19 12:38:17 PST -------
(From update of attachment 119655 [details])
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 From 2011-12-19 14:14:12 PST -------
(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*).
>>> 
>>> 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 From 2011-12-19 15:03:16 PST -------
Committed r103272: <http://trac.webkit.org/changeset/103272>