Bug 46318 – Perform framebuffer attachment checking upon draw call rather than attachment

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Per recent changes to the WebGL specification (Framebuffer Object Attachments, section 6.3), checks about the validity of the framebuffer configuration need to occur upon draw call rather than at the point a renderbuffer is attached to a framebuffer. In particular:

 - Attachments to more than one attachment point (DEPTH, STENCIL, DEPTH_STENCIL) are illegal.
 - The attachment's format must match that of the attachment point.

The reason for this is to avoid implicitly supporting separate depth+stencil (or packed depth+stencil) on some platforms and not others.

[Zhenyao Mo]

Created attachment 72252 [details]
patch

I will commit the test change to khronos once reviewed.

[Kenneth Russell]

Comment on attachment 72252 [details]
patch

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

Looks good overall. Couple of minor comments.

> WebCore/html/canvas/WebGLFramebuffer.cpp:38
> +static unsigned long getInternalFormat(WebGLObject* buffer)

You could use an anonymous namespace instead of static methods here.

> WebCore/html/canvas/WebGLFramebuffer.cpp:141
> +        return true;

This test seems redundant.

> WebCore/html/canvas/WebGLFramebuffer.h:49
> +    // readPixels, copyTexImage2D, copyTexSubImage2D if this frame bufferis

bufferis -> buffer is

[Kenneth Russell]

Comment on attachment 72252 [details]
patch

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

Sorry, I noticed an issue after r+'ing the patch. Hopefully you get this email before landing.

> WebCore/html/canvas/WebGLRenderingContext.cpp:458
> +        return GraphicsContext3D::FRAMEBUFFER_INCOMPLETE_ATTACHMENT;

According to the spec this is supposed to be FRAMEBUFFER_UNSUPPORTED. Is there a reason to diverge? If not, please update the code and the test.

[Zhenyao Mo]

Created attachment 72754 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 72754 [details]
revised patch: responding to kbr's review

Looks good. Thanks for taking care of this. Don't forget to sync the test with the Khronos repository.

[Zhenyao Mo]

Committed r71190: <http://trac.webkit.org/changeset/71190>