Bug 107526 – Optimize some operations for float type in texture format conversions of WebGL

• Component: WebGL
• Status: RESOLVED FIXED

[Jun Jiang]

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.

[Jun Jiang]

Created attachment 183934 [details]
Patch

[Darin Adler]

Comment on attachment 183934 [details]
Patch

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.

[Jun Jiang]

Created attachment 184167 [details]
Patch

[Jun Jiang]

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.

[Kenneth Russell]

Comment on attachment 184167 [details]
Patch

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."

[Jun Jiang]

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.

[Jun Jiang]

Created attachment 186038 [details]
Patch

[Jun Jiang]

Re-base the patch and modify the ChangeLog.

[Kenneth Russell]

Comment on attachment 186038 [details]
Patch

Looks fine. You already have Darin's review and don't need another. Submitting to CQ.

[WebKit Review Bot]

Comment on attachment 186038 [details]
Patch

Clearing flags on attachment: 186038

Committed r141706: <http://trac.webkit.org/changeset/141706>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.