Bug 35611 - Implement and test new framebuffer object attachment behavior
Summary: Implement and test new framebuffer object attachment behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 14:45 PST by Kenneth Russell
Modified: 2010-04-02 13:03 PDT (History)
6 users (show)

See Also:


Attachments
patch (25.14 KB, patch)
2010-03-12 18:04 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
fixed style issue (25.13 KB, patch)
2010-03-12 18:12 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: responding to Ken Russell's review (26.49 KB, patch)
2010-03-16 17:24 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : responding to Ken Russell's review (26.91 KB, patch)
2010-03-16 18:19 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: made changes after Ken Russell's refac (27.62 KB, patch)
2010-03-24 19:12 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: skip the framebuffer-object-attachment.html test on snow leopard (28.40 KB, patch)
2010-03-31 19:04 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: fixed the bogus bug ID. (28.37 KB, application/octet-stream)
2010-03-31 19:10 PDT, Zhenyao Mo
no flags Details
revised patch: fixed the bogus bug ID. (28.37 KB, patch)
2010-03-31 19:12 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch (28.37 KB, patch)
2010-03-31 20:13 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: resolving a conflict (28.34 KB, patch)
2010-03-31 20:23 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.