Bug 52068 - Need to consider UNPACK_ALIGNMENT in GraphicsContext3D::texImage2DResourceSafe
Summary: Need to consider UNPACK_ALIGNMENT in GraphicsContext3D::texImage2DResourceSafe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks: 52237
  Show dependency treegraph
 
Reported: 2011-01-07 10:05 PST by Zhenyao Mo
Modified: 2011-01-11 12:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2011-01-07 11:06 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (32.54 KB, patch)
2011-01-10 13:30 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (33.85 KB, patch)
2011-01-11 10:54 PST, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2011-01-07 10:05:57 PST
Need to UNPACK_ALIGNMENT to 1 before calling texImage2D, then restore the original value.
Comment 1 Zhenyao Mo 2011-01-07 11:06:41 PST
Created attachment 78250 [details]
Patch
Comment 2 Kenneth Russell 2011-01-07 11:57:41 PST
Comment on attachment 78250 [details]
Patch

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

> WebCore/platform/graphics/GraphicsContext3D.cpp:138
> +        getIntegerv(GraphicsContext3D::UNPACK_ALIGNMENT, &unpackAlignment);

This query will kill performance of texture uploads. Instead, change the function signature and all callers to pass in the current unpack alignment, or track the unpack alignment in the GraphicsContext3D implementation.
Comment 3 Zhenyao Mo 2011-01-10 13:30:56 PST
Created attachment 78443 [details]
Patch

refactor the code, adding a new function computeImageSizeInBytes and use it whenever we can
Comment 4 Kenneth Russell 2011-01-10 15:35:57 PST
Comment on attachment 78443 [details]
Patch

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

The code changes look good; however, there's a bug in the test case pulled down from Khronos that needs to be fixed.

> LayoutTests/fast/canvas/webgl/read-pixels-test-expected.txt:1
> +CONSOLE MESSAGE: line 114: ReferenceError: Can't find variable: INVALID_OPERATION

There's a bug in the test case; the initialization of the various bad tests isn't picking up the enum values through the WebGLRenderingContext object. An exception is being thrown, and the test isn't completing successfully.
Comment 5 Zhenyao Mo 2011-01-11 10:54:16 PST
Created attachment 78551 [details]
Patch
Comment 6 Zhenyao Mo 2011-01-11 10:57:52 PST
(In reply to comment #5)
> Created an attachment (id=78551) [details]
> Patch

I fixed the test on the khronos side.  This patch is in sync with khronos.

Sorry Adrienne, I copied finishTest() to webgl-test.html for this patch.  It might add to the mess you try to clean up.
Comment 7 Adrienne Walker 2011-01-11 11:01:25 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=78551) [details] [details]
> > Patch
> 
> I fixed the test on the khronos side.  This patch is in sync with khronos.
> 
> Sorry Adrienne, I copied finishTest() to webgl-test.html for this patch.  It might add to the mess you try to clean up.

Thanks for the heads up.  I needed that function for the video test too, which is why I originally started looking into the script cleanup.  It's a good short term solution; we can clean it up with the other changes later.
Comment 8 Kenneth Russell 2011-01-11 11:28:18 PST
Comment on attachment 78551 [details]
Patch

Looks good. Nice work.
Comment 9 Zhenyao Mo 2011-01-11 11:49:52 PST
Committed r75524: <http://trac.webkit.org/changeset/75524>
Comment 10 WebKit Review Bot 2011-01-11 12:33:15 PST
http://trac.webkit.org/changeset/75524 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/canvas/webgl/read-pixels-test.html