WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
wip
(46.30 KB, patch)
2022-02-03 20:16 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(50.05 KB, patch)
2022-02-04 11:56 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(52.01 KB, patch)
2022-02-08 15:54 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(65.81 KB, patch)
2022-02-09 00:22 PST
,
Nikos Mouchtaris
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(65.67 KB, patch)
2022-02-09 01:06 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(56.53 KB, patch)
2022-02-09 23:30 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(55.04 KB, patch)
2022-02-10 02:09 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(55.82 KB, patch)
2022-02-10 13:00 PST
,
Nikos Mouchtaris
simon.fraser
: review+
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(56.39 KB, patch)
2022-02-10 13:57 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Nikos Mouchtaris
Comment 1
2022-02-03 19:32:23 PST
Created
attachment 450856
[details]
wip
Nikos Mouchtaris
Comment 2
2022-02-03 20:16:11 PST
Created
attachment 450858
[details]
wip
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
Created
attachment 450927
[details]
Patch
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
Created
attachment 451317
[details]
Patch
Nikos Mouchtaris
Comment 13
2022-02-09 00:22:21 PST
Created
attachment 451343
[details]
Patch
Nikos Mouchtaris
Comment 14
2022-02-09 01:06:15 PST
Created
attachment 451345
[details]
Patch
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
Created
attachment 451497
[details]
Patch
Nikos Mouchtaris
Comment 17
2022-02-10 02:09:57 PST
Created
attachment 451508
[details]
Patch
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
Created
attachment 451592
[details]
Patch
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
Created
attachment 451599
[details]
Patch
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
<
rdar://problem/88791335
>
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.
Top of Page
Format For Printing
XML
Clone This Bug