[chromium] Stop using WebTransformationMatrix on WebLayer
Created attachment 191854 [details] Patch
This won't compile on clang bots until https://chromiumcodereview.appspot.com/12481008/. Most of WebTransformationMatrix is already dead code, but it's still being used as a transit type in WebTransformAnimationCurve and WebTransformOperations. Will have to land more code chromium-side first to get rid of those.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 191854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191854&action=review I like it -- I just have a couple of dumb questions. > Source/Platform/chromium/public/WebLayer.h:36 > +#include "WebTransformationMatrix.h" What's this being added for? > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:723 > + return ret; Given how Matrix4 is laid out, could you take advantage of SkMatrix44's setColMajord or setRowMajord.
(In reply to comment #4) > (From update of attachment 191854 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191854&action=review > > I like it -- I just have a couple of dumb questions. > > > Source/Platform/chromium/public/WebLayer.h:36 > > +#include "WebTransformationMatrix.h" > > What's this being added for? > No need, will remove. > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:723 > > + return ret; > > Given how Matrix4 is laid out, could you take advantage of SkMatrix44's setColMajord or setRowMajord. Do you have an example of code that does this? This code is doing the same thing that WebTransformationMatrixUtil::ToTransform() does.
Created attachment 192047 [details] remove unnecessary include from WebLayer
> > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:723 > > + return ret; > > Given how Matrix4 is laid out, could you take advantage of SkMatrix44's setColMajord or setRowMajord. Unfortunately I think doing that would require an extra intermediate float[16] array, and it seems not worth it to cause that extra copy. So my vote is that we should keep the code the way James has it now - what do you think Ian?
(In reply to comment #7) > > > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:723 > > > + return ret; > > > > Given how Matrix4 is laid out, could you take advantage of SkMatrix44's setColMajord or setRowMajord. > > Unfortunately I think doing that would require an extra intermediate float[16] array, and it seems not worth it to cause that extra copy. So my vote is that we should keep the code the way James has it now - what do you think Ian? Please forgive me if this is dumb, but if we have: double m[4][4]; and we assign double *linear = &m[0][0]; then can't we can access linear[0] through linear[15]? This seemed to work in a test program of mine, but perhaps we're not guaranteed that the 2D array will be laid out in such a way to allow this? If this isn't crazy, then we should be able to pass the Matrix4 owned by the TransformationMatrix directly to SkMatrix44::setColMajord or SkMatrix44::setRowMajord (I'm not sure which, I always get this wrong and end up transposed).
(In reply to comment #8) > (In reply to comment #7) > > > > > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:723 > > > > + return ret; > > > > > > Given how Matrix4 is laid out, could you take advantage of SkMatrix44's setColMajord or setRowMajord. > > > > Unfortunately I think doing that would require an extra intermediate float[16] array, and it seems not worth it to cause that extra copy. So my vote is that we should keep the code the way James has it now - what do you think Ian? > > Please forgive me if this is dumb, but if we have: > double m[4][4]; > and we assign > double *linear = &m[0][0]; > then can't we can access linear[0] through linear[15]? This seemed to work in a test program of mine, but perhaps we're not guaranteed that the 2D array will be laid out in such a way to allow this? > > If this isn't crazy, then we should be able to pass the Matrix4 owned by the TransformationMatrix directly to SkMatrix44::setColMajord or SkMatrix44::setRowMajord (I'm not sure which, I always get this wrong and end up transposed). I think TransformationMatrix is laid out as column-major. So if that works, and it's not considered taboo, that's OK with me =) I was more specifically looking at the toColumnMajorFloatArray() function in TransformationMatrix.
This is all fine but this patch is just removing dependencies and preserves the existing behavior exactly. I'd rather not try to do too many things at once.
unofficially this patch LGTM, you might want to remove the "no new tests oops" line in the changelog. Ian and I can resolve the optimizations later, then.
Comment on attachment 192047 [details] remove unnecessary include from WebLayer R=me.
Committed r145131: <http://trac.webkit.org/changeset/145131>