Bug 94860 – [chromium] Should accelerate rotations of >= 180 degrees

• Component: WebKit Misc.
• Status: RESOLVED FIXED

Currently we don't accelerate these animations because our matrix-based keyframe interpolation would fail. If WebTransformOperation was given an x y z and angle, we could do better rotation interpolation (actually, for all operation types) and drop this restriction.

Created attachment 160485 [details]
Patch

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

[Dana Jansens]

Comment on attachment 160485 [details]
Patch

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

> Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:83
> +bool isIdentity(const WebTransformOperation* operation)

static for all these methods?

[Shawn Singh]

Comment on attachment 160485 [details]
Patch


Seems like a nice cleanup to me!

I'm a bit worried about the additional identity stuff.  It seems to be a bit precarious in places where if we want to modify the code later, we might need to remember to check isIdentity() which could be easy to overlook.  Otherwise LGTM =)


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

> Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:65
>      WebTransformationMatrix matrix;
> +    double x;
> +    double y;
> +    double z;
> +    double angle;

do we use the matrix anymore, for any operation other than WebTransformOperationMatrix?

maybe we could use unions here.  Not only could that save space, but would allow us to have more intuitive names for the parameters of each transform.

> Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:154
> +        double fromX = isIdentity(from) ? 0 : from->x;
> +        double fromY = isIdentity(from) ? 0 : from->y;
> +        double fromZ = isIdentity(from) ? 0 : from->z;
> +        double toX = isIdentity(to) ? 0 : to->x;
> +        double toY = isIdentity(to) ? 0 : to->y;
> +        double toZ = isIdentity(to) ? 0 : to->z;

this pattern shows up a lot in this patch - is it really necessary?  or could we just initialize to and from x, y, z appropriately in all cases?   Seems like this isIdentity() ? 0 : value  pattern may be error-prone down the road...

> Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:177
> +        else {
> +            WebTransformationMatrix toMatrix;
> +            if (!isIdentity(to))
> +                toMatrix = to->matrix;
> +            WebTransformationMatrix fromMatrix;
> +            if (!isIdentity(from))
> +                fromMatrix = from->matrix;
> +            toReturn = toMatrix;
> +            toReturn.blend(fromMatrix, progress);
> +        }

Will this else-statement also work for angles greater than 180?  I thought we would have to interpolate the axis and angle separately to get this part correct?

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:117
> +            webTransformOperations.appendIdentity();

I'm willing to believe that this is correct, but I'm just wondering why this change is necessary?   Especially for the NONE case?

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:-148
> -bool causesRotationOfAtLeast180Degrees(const TransformAnimationValue* value, const TransformAnimationValue* lastValue)

Refreshing!

Created attachment 160761 [details]
Patch

(In reply to comment #4)
> (From update of attachment 160485 [details])
>
> Seems like a nice cleanup to me!
>
> I'm a bit worried about the additional identity stuff.  It seems to be a bit precarious in places where if we want to modify the code later, we might need to remember to check isIdentity() which could be easy to overlook.  Otherwise LGTM =)
>
>
> View in context: https://bugs.webkit.org/attachment.cgi?id=160485&action=review
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:65
> >      WebTransformationMatrix matrix;
> > +    double x;
> > +    double y;
> > +    double z;
> > +    double angle;
>
> do we use the matrix anymore, for any operation other than WebTransformOperationMatrix?
>
> maybe we could use unions here.  Not only could that save space, but would allow us to have more intuitive names for the parameters of each transform.
I have used a union, but the matrix couldn't be included as it has a non-trivial copy constructor.
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:154
> > +        double fromX = isIdentity(from) ? 0 : from->x;
> > +        double fromY = isIdentity(from) ? 0 : from->y;
> > +        double fromZ = isIdentity(from) ? 0 : from->z;
> > +        double toX = isIdentity(to) ? 0 : to->x;
> > +        double toY = isIdentity(to) ? 0 : to->y;
> > +        double toZ = isIdentity(to) ? 0 : to->z;
>
> this pattern shows up a lot in this patch - is it really necessary?  or could we just initialize to and from x, y, z appropriately in all cases?   Seems like this isIdentity() ? 0 : value  pattern may be error-prone down the road...
I do think it's necessary. It can be the case that to or from is NULL. It's also possible that 'to' is a translation and 'from' is a rotation, but it just so happens that the 'from' rotation is 0 degrees (and is, therefore, an identity transform).
So if I tried to access from.translate.x, it would not be initialized correctly (since we set the rotate part of the union, not the translate).
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:177
> > +        else {
> > +            WebTransformationMatrix toMatrix;
> > +            if (!isIdentity(to))
> > +                toMatrix = to->matrix;
> > +            WebTransformationMatrix fromMatrix;
> > +            if (!isIdentity(from))
> > +                fromMatrix = from->matrix;
> > +            toReturn = toMatrix;
> > +            toReturn.blend(fromMatrix, progress);
> > +        }
>
> Will this else-statement also work for angles greater than 180?  I thought we would have to interpolate the axis and angle separately to get this part correct?
I experimented with interpolating the axes, but the results were unattractive and do not match the unaccelerated path. It appears that the correct behavior is to
rotate3d(x, y, z, blend(fromAngle, toAngle, progress)) if the axes line up, but revert to blending the rotation matrices otherwise (which decomposes to quaternions
and does a quaternion slerp under the covers).
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:117
> > +            webTransformOperations.appendIdentity();
>
> I'm willing to believe that this is correct, but I'm just wondering why this change is necessary?   Especially for the NONE case?
This was a bug -- we should do nothing in the NONE case. Fixed.

[James Robinson]

Comment on attachment 160761 [details]
Patch

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

OK

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:119
> +            // do nothing.

Not sure why this changed - I think the caps was better

Created attachment 160875 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 160875 [details]
Patch for landing

Rejecting attachment 160875 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
re/ChangeLog
Auto-merging Source/WebCore/WebCore.xcodeproj/project.pbxproj
Failed to merge in the changes.
Patch failed at 0001 Even more unreviewed rebaselining after r126683.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13644161

Created attachment 160884 [details]
Patch

Rebasing.

[WebKit Review Bot]

Comment on attachment 160884 [details]
Patch

Clearing flags on attachment: 160884

Committed r126838: <http://trac.webkit.org/changeset/126838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.