[webkit-reviews] review granted: [Bug 74741] Postpone deleteRenderbuffer/deleteTexture until all framebuffer attachment points are removed. : [Attachment 119655] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 74741: Postpone deleteRenderbuffer/deleteTexture until all framebuffer
attachment points are removed.
https://bugs.webkit.org/show_bug.cgi?id=74741

Attachment 119655: Patch
https://bugs.webkit.org/attachment.cgi?id=119655&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.