Bug 236116

Created attachment 450856 [details] wip

Created attachment 450858 [details] wip

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"?

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

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?

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.

Created attachment 450927 [details] Patch

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?

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

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

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

Created attachment 451317 [details] Patch

Created attachment 451343 [details] Patch

Created attachment 451345 [details] Patch

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.

Created attachment 451497 [details] Patch

Created attachment 451508 [details] Patch

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

Created attachment 451592 [details] Patch

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

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.

Created attachment 451599 [details] Patch

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

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

<rdar://problem/88791335>

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

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