Bug 111635 – [chromium] Stop using WebTransformationMatrix on WebLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Stop using WebTransformationMatrix on WebLayer

[James Robinson]

Created attachment 191854 [details]
Patch

[James Robinson]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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.

[James Robinson]

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

[James Robinson]

Created attachment 192047 [details]
remove unnecessary include from WebLayer

[Shawn Singh]

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

[Shawn Singh]

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

[James Robinson]

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.

[Shawn Singh]

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.

[Adrienne Walker]

Comment on attachment 192047 [details]
remove unnecessary include from WebLayer

R=me.

[James Robinson]

Committed r145131: <http://trac.webkit.org/changeset/145131>