Bug 107526

Summary: Optimize some operations for float type in texture format conversions of WebGL
Product: WebKit Reporter: Jun Jiang <jun.a.jiang>
Component: WebGLAssignee: Jun Jiang <jun.a.jiang>
Severity: Normal CC: dino, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Patch none

Description Jun Jiang 2013-01-22 01:56:11 PST
In the implementation of texture format conversion for WebGL, some operations for float type could be optimized. One is that, divide operations could be reduced to once from twice. The other is that, use memcpy() to copy a pack of float type instead of one by one.
Comment 1 Jun Jiang 2013-01-22 02:04:32 PST
Created attachment 183934 [details]
Comment 2 Darin Adler 2013-01-22 10:16:03 PST
Comment on attachment 183934 [details]

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

Optimizations look fine. I’m a little concerned that we truncate rather than rounding after multiplying by scale factors. I’m not sure that’s the correct calculation. I also wonder if there’s an even faster branch-free way to compute those scale factors?

Someone should fix the style mistake all over this file, calling the type "unsigned int" when the WebKit coding style suggests calling it just "unsigned".

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1177
> +    ASSERT(source && destination);

Two thoughts:

1) We never do ASSERT with && in it. We always prefer two separate assertions in such cases so we can more easily figure out which one has failed.
2) It seems unnecessary to assert these pointers are non-zero here. Such pointers could be scattered all over the code; there’s no particular reason to expect null pointers here or to assume it would be hard to diagnose a null pointer related crash inside memcpy.

Based on that, I suggest removing this line of code.
Comment 3 Jun Jiang 2013-01-23 00:38:52 PST
Created attachment 184167 [details]
Comment 4 Jun Jiang 2013-01-23 01:07:04 PST
Hi, Darin. Thanks for your comments. I had changed and rebased the patch. As you had suggested, it was really unnecessary to add the ASSERT() there. And for the issue that we truncate rather than rounding after multiplying by scale factors, I agree that rounding may have a smaller deviation than truncate. But I am not quite sure if this is the only criteria in design to choose the implementation. Therefore, the truncate behavior is not changed and left for future change if needed.
Comment 5 Kenneth Russell 2013-01-23 13:45:13 PST
Comment on attachment 184167 [details]

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

Looks good. I'd prefer not to override darin's r+; would it be too much trouble to upload a new patch? Then it can be cq+'d easily.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

At this point you can and should change this line to say "Reviewed by Darin Adler."
Comment 6 Jun Jiang 2013-01-23 17:33:51 PST
Hi, Darin. Could you help to review and merge it ? As Kenneth mentioned, I should add "Reviewed by Darin Adler." in the Changelog but I miss it. Thanks.
Comment 7 Jun Jiang 2013-02-01 07:12:56 PST
Created attachment 186038 [details]
Comment 8 Jun Jiang 2013-02-01 07:15:39 PST
Re-base the patch and modify the ChangeLog.
Comment 9 Kenneth Russell 2013-02-02 14:29:42 PST
Comment on attachment 186038 [details]

Looks fine. You already have Darin's review and don't need another. Submitting to CQ.
Comment 10 WebKit Review Bot 2013-02-02 14:34:08 PST
Comment on attachment 186038 [details]

Clearing flags on attachment: 186038

Committed r141706: <http://trac.webkit.org/changeset/141706>
Comment 11 WebKit Review Bot 2013-02-02 14:34:12 PST
All reviewed patches have been landed.  Closing bug.