RESOLVED FIXED Bug 236116
Web animations- Composite operation accumulation support for transform properties
https://bugs.webkit.org/show_bug.cgi?id=236116
Summary Web animations- Composite operation accumulation support for transform proper...
Nikos Mouchtaris
Reported 2022-02-03 19:01:00 PST
Improve support for accumulation composite operation on transform properties.
Attachments
wip (46.02 KB, patch)
2022-02-03 19:32 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
wip (46.30 KB, patch)
2022-02-03 20:16 PST, Nikos Mouchtaris
no flags
Patch (50.05 KB, patch)
2022-02-04 11:56 PST, Nikos Mouchtaris
no flags
Patch (52.01 KB, patch)
2022-02-08 15:54 PST, Nikos Mouchtaris
no flags
Patch (65.81 KB, patch)
2022-02-09 00:22 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
Patch (65.67 KB, patch)
2022-02-09 01:06 PST, Nikos Mouchtaris
no flags
Patch (56.53 KB, patch)
2022-02-09 23:30 PST, Nikos Mouchtaris
no flags
Patch (55.04 KB, patch)
2022-02-10 02:09 PST, Nikos Mouchtaris
no flags
Patch (55.82 KB, patch)
2022-02-10 13:00 PST, Nikos Mouchtaris
simon.fraser: review+
ews-feeder: commit-queue-
Patch (56.39 KB, patch)
2022-02-10 13:57 PST, Nikos Mouchtaris
no flags
Nikos Mouchtaris
Comment 1 2022-02-03 19:32:23 PST
Nikos Mouchtaris
Comment 2 2022-02-03 20:16:11 PST
Simon Fraser (smfr)
Comment 3 2022-02-03 21:44:29 PST
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
Comment 4 2022-02-04 01:38:56 PST
(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
Comment 5 2022-02-04 01:39:50 PST
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 6 2022-02-04 01:47:34 PST
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
Comment 7 2022-02-04 11:56:40 PST
Simon Fraser (smfr)
Comment 8 2022-02-04 12:04:44 PST
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
Comment 9 2022-02-04 12:17:02 PST
(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
Comment 10 2022-02-04 12:17:56 PST
(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
Comment 11 2022-02-04 12:24:23 PST
(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
Comment 12 2022-02-08 15:54:05 PST
Nikos Mouchtaris
Comment 13 2022-02-09 00:22:21 PST
Nikos Mouchtaris
Comment 14 2022-02-09 01:06:15 PST
Martin Robinson
Comment 15 2022-02-09 03:06:07 PST
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
Comment 16 2022-02-09 23:30:50 PST
Nikos Mouchtaris
Comment 17 2022-02-10 02:09:57 PST
Simon Fraser (smfr)
Comment 18 2022-02-10 10:23:50 PST
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
Comment 19 2022-02-10 13:00:55 PST
Nikos Mouchtaris
Comment 20 2022-02-10 13:06:21 PST
(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 21 2022-02-10 13:25:57 PST
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
Comment 22 2022-02-10 13:57:10 PST
Nikos Mouchtaris
Comment 23 2022-02-10 13:58:33 PST
(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
Comment 24 2022-02-10 18:28:17 PST
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
Comment 25 2022-02-10 18:29:17 PST
Antoine Quint
Comment 26 2022-02-11 00:02:37 PST
*** Bug 235782 has been marked as a duplicate of this bug. ***
Antoine Quint
Comment 27 2022-02-11 00:03:25 PST
*** Bug 235784 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.