Bug 35611

Summary: Implement and test new framebuffer object attachment behavior
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dglazkov, fishd, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
fixed style issue
none
revised patch: responding to Ken Russell's review
none
revised patch : responding to Ken Russell's review
none
revised patch: made changes after Ken Russell's refac
none
revised patch: skip the framebuffer-object-attachment.html test on snow leopard
none
revised patch: fixed the bogus bug ID.
none
revised patch: fixed the bogus bug ID.
none
revised patch
none
revised patch: resolving a conflict none

Description Kenneth Russell 2010-03-02 14:45:25 PST
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.
Comment 1 Zhenyao Mo 2010-03-12 18:04:45 PST
Created attachment 50648 [details]
patch
Comment 2 WebKit Review Bot 2010-03-12 18:06:39 PST
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.
Comment 3 Zhenyao Mo 2010-03-12 18:12:01 PST
Created attachment 50649 [details]
fixed style issue
Comment 4 Kenneth Russell 2010-03-15 17:47:00 PDT
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.
Comment 5 Zhenyao Mo 2010-03-16 17:24:39 PDT
Created attachment 50855 [details]
revised patch: responding to Ken Russell's review
Comment 6 Kenneth Russell 2010-03-16 17:34:49 PDT
We need to address http://bugs.webkit.org/show_bug.cgi?id=36194 before considering committing this or any additional WebGL changes.
Comment 7 Zhenyao Mo 2010-03-16 18:19:13 PDT
Created attachment 50864 [details]
revised patch : responding to Ken Russell's review
Comment 8 Zhenyao Mo 2010-03-24 19:12:20 PDT
Created attachment 51580 [details]
revised patch: made changes after Ken Russell's refac
Comment 9 Kenneth Russell 2010-03-30 16:52:26 PDT
Comment on attachment 51580 [details]
revised patch: made changes after Ken Russell's refac

Looks great.
Comment 10 Zhenyao Mo 2010-03-31 19:04:20 PDT
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.
Comment 11 Kenneth Russell 2010-03-31 19:06:14 PDT
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
Comment 12 Zhenyao Mo 2010-03-31 19:10:59 PDT
Created attachment 52238 [details]
revised patch: fixed the bogus bug ID.
Comment 13 Zhenyao Mo 2010-03-31 19:12:18 PDT
Created attachment 52239 [details]
revised patch: fixed the bogus bug ID.
Comment 14 Kenneth Russell 2010-03-31 19:24:42 PDT
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.
Comment 15 Zhenyao Mo 2010-03-31 20:13:37 PDT
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.
Comment 16 Zhenyao Mo 2010-03-31 20:23:59 PDT
Created attachment 52249 [details]
revised patch: resolving a conflict
Comment 17 Kenneth Russell 2010-04-01 14:36:43 PDT
Comment on attachment 52249 [details]
revised patch: resolving a conflict

Looks good to me.
Comment 18 Eric Seidel (no email) 2010-04-02 11:54:36 PDT
Comment on attachment 52249 [details]
revised patch: resolving a conflict

Looks good as far as I can tell.
Comment 19 WebKit Commit Bot 2010-04-02 13:03:13 PDT
Comment on attachment 52249 [details]
revised patch: resolving a conflict

Clearing flags on attachment: 52249

Committed r57018: <http://trac.webkit.org/changeset/57018>
Comment 20 WebKit Commit Bot 2010-04-02 13:03:19 PDT
All reviewed patches have been landed.  Closing bug.