Bug 236116 – Web animations- Composite operation accumulation support for transform properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Nikos Mouchtaris]

Improve support for accumulation composite operation on transform properties.

[Nikos Mouchtaris]

Created attachment 450856 [details]
wip

[Nikos Mouchtaris]

Created attachment 450858 [details]
wip

[Simon Fraser (smfr)]

Comment on attachment 450858 [details]
wip

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

> Source/WebCore/ChangeLog:3
> +        Accumulation support for transform properties

Needs the word "Animation" here somewhere.

> Source/WebCore/ChangeLog:13
> +        A couple of bugs are also addressed. For a property with a different transform operation
> +        than the keyframes given, transformFunctionListsMatch() would return true when it shouldn't. 

Can these bugs be fixed independently of the composition change? If so, you should do that.

> Source/WebCore/platform/graphics/transforms/MatrixTransformOperation.cpp:42
> +        to.blend(from, context.progress, context.compositeOperation);        

trailing whitespace

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:26
> +#include "CSSPropertyBlendingClient.h"

This is a layering violation. We're in platform code here, so we can't include files outside of "platform"

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:29
> +#include "RenderBox.h"

Ditto

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1643
>      if (compositeOperation == CompositeOperation::Replace)
>          from = from + (to - from) * progress;
> +    else if (compositeOperation == CompositeOperation::Accumulate)
> +        from += from + (to - from - 1) * progress;

Should this be a switch()?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1678
> +    blendFloat(fromDecomp.m11, toDecomp.m11, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Accumulate : compositeOperation);

Not sure if I'm being dense, but how is 'compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Accumulate : compositeOperation" different from "compositeOperation"?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 450858 [details]
> wip
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450858&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:26
> > +#include "CSSPropertyBlendingClient.h"
> 
> This is a layering violation. We're in platform code here, so we can't
> include files outside of "platform"

Yes, you should pass down the size directly like the existing line of code below.

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1643
> >      if (compositeOperation == CompositeOperation::Replace)
> >          from = from + (to - from) * progress;
> > +    else if (compositeOperation == CompositeOperation::Accumulate)
> > +        from += from + (to - from - 1) * progress;
> 
> Should this be a switch()?

Might as well now that all three CompositeOperation enum values are handled.

[Antoine Quint]

I'm also interested in hearing what the next step would be to fix the remaining accumulation test failures. Do you know what will need to be done Nikos?

[Martin Robinson]

Comment on attachment 450858 [details]
wip

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

This looks like a nice progression. I've left a comment in addition to the others above.

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:89
> +        if (fromOperation && toOperation && !fromOperation->isSameType(*toOperation))

Instead of isSameType(), this should check whether fromOperation->sharedPrimitiveType(toOperation) return a value, because even when the types are different the two operations might share a blending primitive.

[Nikos Mouchtaris]

Created attachment 450927 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 450927 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:77
> +TransformOperations TransformOperations::blendByMatchingOperations(const TransformOperations& from, const BlendingContext& context, const LayoutSize& size) const

Call the argument boxSize to avoid ambiguity with vector sizes.

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:87
> +        if (fromOperation && toOperation && !fromOperation->isSameType(*toOperation))

Martin had a comment about this.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> +    blendFloat(fromDecomp.m12, toDecomp.m12, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> +    blendFloat(fromDecomp.m21, toDecomp.m21, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);

Why is CompositeOperation::Add used here and not, say, for m11 or m22? Very mysterious. Would help to have a named local variable that explains why.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1709
> +    blendFloat(fromDecomp.skewXY, toDecomp.skewXY, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);

Ditto; why is CompositeOperation::Add not used for scaleX etc?

[Nikos Mouchtaris]

(In reply to Antoine Quint from comment #5)
> I'm also interested in hearing what the next step would be to fix the
> remaining accumulation test failures. Do you know what will need to be done
> Nikos?

Most of the remaining failures are related to matrix3d. Will most likely require changes to how we accumulate for non affine matrices.

[Nikos Mouchtaris]

(In reply to Martin Robinson from comment #6)
> Comment on attachment 450858 [details]
> wip
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450858&action=review
> 
> This looks like a nice progression. I've left a comment in addition to the
> others above.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:89
> > +        if (fromOperation && toOperation && !fromOperation->isSameType(*toOperation))
> 
> Instead of isSameType(), this should check whether
> fromOperation->sharedPrimitiveType(toOperation) return a value, because even
> when the types are different the two operations might share a blending
> primitive.

Yeah I need to rebase line against your changes in https://bugs.webkit.org/show_bug.cgi?id=235311.

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 450927 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450927&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> > +    blendFloat(fromDecomp.m12, toDecomp.m12, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> > +    blendFloat(fromDecomp.m21, toDecomp.m21, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> 
> Why is CompositeOperation::Add used here and not, say, for m11 or m22? Very
> mysterious. Would help to have a named local variable that explains why.
I tried to address this in the changelog but accumulation for certain properties needs  to use one based accumulation (for example such that two scales [1 1] and [1 1] don't produce [2 2] but produce [1 1]). I can add a local variable that references one based accumulation.

[Nikos Mouchtaris]

Created attachment 451317 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451343 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451345 [details]
Patch

[Martin Robinson]

Comment on attachment 451345 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:91
> +        if (fromOperation && toOperation && !fromOperation->sharedPrimitiveType(toOperation.get()))

I'm working on related code today and I think I'm fairly certain this means that you can ASSERT(blendedOperation) below and remove the else statement. Might want to double-check that by making it a RELEASE_ASSERT first and running the test suite.

[Nikos Mouchtaris]

Created attachment 451497 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451508 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 451508 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        First thing being addressed is handling accumulation of TransformationMatrices.

Try to avoid using abbreviated language; it gets in the way of communicating the information.

If you need to, break the text into paragraphs.

Currently, this changelog reads like a laundry list of intermingled bug fixes and feature work, and it's unclear to me if they should be, or can be split out.

> Source/WebCore/ChangeLog:12
> +        A couple of bugs are also addressed. For a property with a different transform operation

Can these bugs be fixed independently of the composition change? If so, you should do that.

> Source/WebCore/ChangeLog:16
> +        Another bug is that we need to pass the composite operation to TransformationMatrix blend().
> +        Another bug is that we should to use Replace behavior if either of the transform matrices are

Ditto.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:173
> +    auto size = is<RenderBox>(context.client->renderer()) ? downcast<RenderBox>(*context.client->renderer()).borderBoxRect().size() : LayoutSize();

size -> boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:77
> +TransformOperations TransformOperations::blendByMatchingOperations(const TransformOperations& from, const BlendingContext& context, const LayoutSize& size) const

size -> boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:83
> +    unsigned maxSize = std::max(fromSize, toSize);

This illustrates why renaming "size" to "boxSize" is necessary.

This code would be clearer if "fromSize" etc referred to "operationCount".

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:92
> +        if (fromOperation && toOperation && !fromOperation->sharedPrimitiveType(toOperation.get()))
> +            return blendByUsingMatrixInterpolation(from, context, size);

Doesn't this need to do something different based on Martin's changes?

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:130
> +        return blendByMatchingOperations(from, context, size);

rename size to boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:35
> +class CSSPropertyBlendingClient;

Unused

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> +    blendFloat(fromDecomp.m12, toDecomp.m12, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> +    blendFloat(fromDecomp.m21, toDecomp.m21, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);

I'm still looking for a local variable with a name that tells me why m12, m21 etc get special treatment.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1718
> +    blendFloat(fromDecomp.skewXY, toDecomp.skewXY, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);

Ditto

[Nikos Mouchtaris]

Created attachment 451592 [details]
Patch

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 451508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451508&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        First thing being addressed is handling accumulation of TransformationMatrices.
> 
> Try to avoid using abbreviated language; it gets in the way of communicating
> the information.
> 
> If you need to, break the text into paragraphs.
> 
> Currently, this changelog reads like a laundry list of intermingled bug
> fixes and feature work, and it's unclear to me if they should be, or can be
> split out.
Updated language/split into two paragraphs.
> 
> > Source/WebCore/ChangeLog:12
> > +        A couple of bugs are also addressed. For a property with a different transform operation
> 
> Can these bugs be fixed independently of the composition change? If so, you
> should do that.
Added a little extra in the changelog but these bugs are only exposed once we start doing something different for accumulate, so I think its reasonable to keep these fixes in this bug.
> 
> > Source/WebCore/ChangeLog:16
> > +        Another bug is that we need to pass the composite operation to TransformationMatrix blend().
> > +        Another bug is that we should to use Replace behavior if either of the transform matrices are
> 
> Ditto.
> 
> > Source/WebCore/animation/CSSPropertyAnimation.cpp:173
> > +    auto size = is<RenderBox>(context.client->renderer()) ? downcast<RenderBox>(*context.client->renderer()).borderBoxRect().size() : LayoutSize();
> 
> size -> boxSize
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:77
> > +TransformOperations TransformOperations::blendByMatchingOperations(const TransformOperations& from, const BlendingContext& context, const LayoutSize& size) const
> 
> size -> boxSize
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:83
> > +    unsigned maxSize = std::max(fromSize, toSize);
> 
> This illustrates why renaming "size" to "boxSize" is necessary.
> 
> This code would be clearer if "fromSize" etc referred to "operationCount".
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:92
> > +        if (fromOperation && toOperation && !fromOperation->sharedPrimitiveType(toOperation.get()))
> > +            return blendByUsingMatrixInterpolation(from, context, size);
> 
> Doesn't this need to do something different based on Martin's changes?
This was changed to used sharedPrimitiveType.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:130
> > +        return blendByMatchingOperations(from, context, size);
> 
> rename size to boxSize
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.h:35
> > +class CSSPropertyBlendingClient;
> 
> Unused
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> > +    blendFloat(fromDecomp.m12, toDecomp.m12, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> > +    blendFloat(fromDecomp.m21, toDecomp.m21, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> 
> I'm still looking for a local variable with a name that tells me why m12,
> m21 etc get special treatment.
Added local variable with comment.
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1718
> > +    blendFloat(fromDecomp.skewXY, toDecomp.skewXY, progress, compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> 
> Ditto
Fixed.

[Simon Fraser (smfr)]

Comment on attachment 451592 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        This patch brings further support for accumulation on transform properties. The main aspect being addressed 
> +        is handling accumulation of TransformationMatrices. This involves accumulating on the decomposed functions, 
> +        which occurs in blendFloat() and accumulateQuaternion(). Since we need to perform accumulation for one-based 
> +        values on certain decomposed functions, (scale, diagonals of matrices, etc), we set the operation to accumulate 
> +        only for these cases.
> +        
> +        A couple of bugs introduced by this additional support are also addressed. For a property with a 
> +        different transform operation than the keyframes given, transformFunctionListsMatch() would return 
> +        true when it shouldn't. Addressed by checking in blendByMatchingOperations and falling back on matrix 
> +        interpolation. Another bug is that we need to pass the composite operation to TransformationMatrix blend().
> +        Another bug is that we should to use Replace behavior if either of the transform matrices are
> +        non-invertible.

Much better!

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:88
> +    unsigned fromOperationCount = from.operations().size();
> +    unsigned toOperationCount = operations().size();
> +    unsigned maxOperationCount = std::max(fromOperationCount, toOperationCount);
> +    
> +    if (context.compositeOperation == CompositeOperation::Accumulate && (!from.isInvertible(boxSize) || !isInvertible(boxSize)))
> +        return blendByUsingMatrixInterpolation(from, context, boxSize);
> +    
> +    for (unsigned i = 0; i < maxOperationCount; i++) {

More readable now.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> +    // When compositeOperation is accumulate, if the decomposed function is a one based value (for affine matrix these properties are
> +    // m11, m22, scaleX and scaleY), use one based accumulation. Otherwise, use behavior for add.
> +    auto operationForNonOneBasedValues = compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation;

Nice. Maybe "one based" -> "1-based".

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1737
> +        accumulateQuaternion(&fromDecomp.quaternionX, &toDecomp.quaternionX);
> +    else
> +        slerp(&fromDecomp.quaternionX, &toDecomp.quaternionX, progress);

Not new, but it's weird that slerp() and now accumulateQuaternion() assume that's it's OK to cast from `double quaternionX, quaternionY, quaternionZ, quaternionW;` in Decomposed4Type to double[4]. There's nothing to prevent the compiler from adding padding between those doubles.

[Nikos Mouchtaris]

Created attachment 451599 [details]
Patch

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #21)
> Comment on attachment 451592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451592&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1737
> > +        accumulateQuaternion(&fromDecomp.quaternionX, &toDecomp.quaternionX);
> > +    else
> > +        slerp(&fromDecomp.quaternionX, &toDecomp.quaternionX, progress);
> 
> Not new, but it's weird that slerp() and now accumulateQuaternion() assume
> that's it's OK to cast from `double quaternionX, quaternionY, quaternionZ,
> quaternionW;` in Decomposed4Type to double[4]. There's nothing to prevent
> the compiler from adding padding between those doubles.

I agree. Added a small helper function to guarantee these arrays are correct.

[EWS]

Committed r289599 (247112@main): <https://commits.webkit.org/247112@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451599 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88791335>

[Antoine Quint]

*** Bug 235782 has been marked as a duplicate of this bug. ***

[Antoine Quint]

*** Bug 235784 has been marked as a duplicate of this bug. ***