Bug 35611 – Implement and test new framebuffer object attachment behavior

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The new framebuffer object attachment behavior in the WebGL spec needs to be implemented and tested. In particular:

 - The new DEPTH_STENCIL and DEPTH_STENCIL_ATTACHMENT enums need to be hooked up
 - The new verification of the renderbuffer's internal format during framebufferRenderbuffer calls needs to be implemented
 - The new verification that illegal combinations of renderbuffers raise INVALID_OPERATION from framebufferRenderbuffer needs to be implemented

All of these need tests as well.

[Zhenyao Mo]

Created attachment 50648 [details]
patch

[WebKit Review Bot]

Attachment 50648 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLRenderingContext.cpp:1226:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

Created attachment 50649 [details]
fixed style issue

[Kenneth Russell]

Comment on attachment 50649 [details]
fixed style issue

This looks very good. A couple of minor comments, which from my standpoint aren't necessary to address in this initial patch. First, in calls like WebGLRenderingContext::framebufferRenderbuffer, consider passing DEPTH_STENCIL_ATTACHMENT into the GraphicsContext3D and letting it decide how it wants to dispatch it. This might help in future implementations on top of OpenGL 3 where the DEPTH_STENCIL attachment point actually exists. Second, in the test case, calls like shouldBe("gl.getError() == gl.NO_ERROR", "true") can be better written shouldBe("gl.getError()", "gl.NO_ERROR"). But again this looks very good and well tested.

[Zhenyao Mo]

Created attachment 50855 [details]
revised patch: responding to Ken Russell's review

[Kenneth Russell]

We need to address http://bugs.webkit.org/show_bug.cgi?id=36194 before considering committing this or any additional WebGL changes.

[Zhenyao Mo]

Created attachment 50864 [details]
revised patch : responding to Ken Russell's review

[Zhenyao Mo]

Created attachment 51580 [details]
revised patch: made changes after Ken Russell's refac

[Kenneth Russell]

Comment on attachment 51580 [details]
revised patch: made changes after Ken Russell's refac

Looks great.

[Zhenyao Mo]

Created attachment 52236 [details]
revised patch: skip the framebuffer-object-attachment.html test on snow leopard

Because the bot does not support EXT_packed_depth_stencil, which is required by WebGL.

[Kenneth Russell]

Comment on attachment 52236 [details]
revised patch: skip the framebuffer-object-attachment.html test on snow leopard

> Index: LayoutTests/platform/mac-snowleopard/Skipped
> ===================================================================
> --- LayoutTests/platform/mac-snowleopard/Skipped	(revision 56464)
> +++ LayoutTests/platform/mac-snowleopard/Skipped	(working copy)
> @@ -110,3 +110,7 @@ fast/forms/search-event-delay.html
>  # computed style on HW transforms are wrong in SL, but
>  # correct on other platforms.
>  animations/fill-mode-transform.html
> +
> +# Snow Leopard bot does not support EXT_packed_depth_stencil, which is required by WebGL.
> +# https://bugs.webkit.org/show_bug.cgi?id=51580

This is a bogus bug ID.

> +fast/canvas/webgl/framebuffer-object-attachment.html
> \ No newline at end of file

[Zhenyao Mo]

Created attachment 52238 [details]
revised patch: fixed the bogus bug ID.

[Zhenyao Mo]

Created attachment 52239 [details]
revised patch: fixed the bogus bug ID.

[Kenneth Russell]

Comment on attachment 52239 [details]
revised patch: fixed the bogus bug ID.

> Index: LayoutTests/platform/mac-snowleopard/Skipped
> ===================================================================
> --- LayoutTests/platform/mac-snowleopard/Skipped	(revision 56464)
> +++ LayoutTests/platform/mac-snowleopard/Skipped	(working copy)
> @@ -110,3 +110,7 @@ fast/forms/search-event-delay.html
>  # computed style on HW transforms are wrong in SL, but
>  # correct on other platforms.
>  animations/fill-mode-transform.html
> +
> +# Snow Leopard bot does not support EXT_packed_depth_stencil, which is required by WebGL.
> +# https://bugs.webkit.org/show_bug.cgi?id=35611
> +fast/canvas/webgl/framebuffer-object-attachment.html

I'm not sure what the preferred style is in these skipped lists, but the last time I modified them, the reviewer wanted a new bug filed which detailed the plan for getting the test out of the skipped list. In this case, it would be to upgrade either the OS or the graphics hardware on the Snow Leopard bot so that it is capable of running WebGL properly.

[Zhenyao Mo]

Created attachment 52248 [details]
revised patch

Created a new bug for snow leopard bot
https://bugs.webkit.org/show_bug.cgi?id=36925

The skipped test comment is pointing to the created bug.

[Zhenyao Mo]

Created attachment 52249 [details]
revised patch: resolving a conflict

[Kenneth Russell]

Comment on attachment 52249 [details]
revised patch: resolving a conflict

Looks good to me.

[Eric Seidel (no email)]

Comment on attachment 52249 [details]
revised patch: resolving a conflict

Looks good as far as I can tell.

[WebKit Commit Bot]

Comment on attachment 52249 [details]
revised patch: resolving a conflict

Clearing flags on attachment: 52249

Committed r57018: <http://trac.webkit.org/changeset/57018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.