[webkit-reviews] review granted: [Bug 236116] Web animations- Composite operation accumulation support for transform properties : [Attachment 451592] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 236116: Web animations- Composite operation accumulation support for
transform properties
https://bugs.webkit.org/show_bug.cgi?id=236116

Attachment 451592: Patch

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




--- Comment #21 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 451592
  --> https://bugs.webkit.org/attachment.cgi?id=451592
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.