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.
Created attachment 183934 [details] Patch
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.
Created attachment 184167 [details] Patch
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 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."
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.
Created attachment 186038 [details] Patch
Re-base the patch and modify the ChangeLog.
Comment on attachment 186038 [details] Patch Looks fine. You already have Darin's review and don't need another. Submitting to CQ.
Comment on attachment 186038 [details] Patch Clearing flags on attachment: 186038 Committed r141706: <http://trac.webkit.org/changeset/141706>
All reviewed patches have been landed. Closing bug.