From 843a0e51f0355245264cea3dcb5b9a383ac28f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 24 Feb 2020 00:13:41 +0000 Subject: [PATCH] Bug 1610171 - Fix SVGAnimatedTransformList optimization to avoid pathological frame reconstruction. r=jwatt The optimization here was trying to avoid reconstructing in some cases when we started animating a transform. Unfortunately it wasn't handling the transform animations properly, which means that once it decided that _adding_ the transform value needed a reconstruction, it was deciding that _changing_ it also needed. Which means that we reconstructed the frames once per animation frame, which is pretty sad. While fixing the optimization, also use a more comprehensive change hint, the one that we use for transform property changes as well. Differential Revision: https://phabricator.services.mozilla.com/D63803 --HG-- extra : moz-landing-system : lando --- dom/svg/SVGAnimatedTransformList.cpp | 12 ++++++++---- dom/svg/SVGAnimatedTransformList.h | 16 ++++++++-------- dom/svg/SVGTransformableElement.cpp | 6 +++--- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/dom/svg/SVGAnimatedTransformList.cpp b/dom/svg/SVGAnimatedTransformList.cpp index 19c6119f8f36..f03c2e96125e 100644 --- a/dom/svg/SVGAnimatedTransformList.cpp +++ b/dom/svg/SVGAnimatedTransformList.cpp @@ -60,16 +60,16 @@ nsresult SVGAnimatedTransformList::SetBaseValue(const SVGTransformList& aValue, domWrapper->InternalBaseValListWillChangeLengthTo(mBaseVal.Length()); } else { mIsAttrSet = true; - // We only need to reconstruct the frame for aSVGElement if it already - // exists and the stacking context changes because a transform is created. - mRequiresFrameReconstruction = + // We only need to treat this as a creation or removal of a transform if the + // frame already exists and it didn't have an existing one. + mCreatedOrRemovedOnLastChange = aSVGElement->GetPrimaryFrame() && !hadTransform; } return rv; } void SVGAnimatedTransformList::ClearBaseValue() { - mRequiresFrameReconstruction = !HasTransform(); + mCreatedOrRemovedOnLastChange = !HasTransform(); DOMSVGAnimatedTransformList* domWrapper = DOMSVGAnimatedTransformList::GetDOMWrapperIfExists(this); @@ -122,6 +122,8 @@ nsresult SVGAnimatedTransformList::SetAnimValue(const SVGTransformList& aValue, } else { modType = MutationEvent_Binding::ADDITION; } + mCreatedOrRemovedOnLastChange = + modType == MutationEvent_Binding::ADDITION; aElement->DidAnimateTransformList(modType); return NS_OK; } @@ -144,6 +146,8 @@ void SVGAnimatedTransformList::ClearAnimValue(SVGElement* aElement) { } else { modType = MutationEvent_Binding::REMOVAL; } + mCreatedOrRemovedOnLastChange = + modType == MutationEvent_Binding::REMOVAL; aElement->DidAnimateTransformList(modType); } diff --git a/dom/svg/SVGAnimatedTransformList.h b/dom/svg/SVGAnimatedTransformList.h index 7d02206c8742..0eff5da5c1d4 100644 --- a/dom/svg/SVGAnimatedTransformList.h +++ b/dom/svg/SVGAnimatedTransformList.h @@ -46,7 +46,7 @@ class SVGAnimatedTransformList { public: SVGAnimatedTransformList() - : mIsAttrSet(false), mRequiresFrameReconstruction(true) {} + : mIsAttrSet(false), mCreatedOrRemovedOnLastChange(true) {} /** * Because it's so important that mBaseVal and its DOMSVGTransformList wrapper @@ -94,18 +94,18 @@ class SVGAnimatedTransformList { bool IsAnimating() const { return !!mAnimVal; } /** - * Returns true if we need to reconstruct the frame of the element associated - * with this transform list because the stacking context has changed. + * Returns true if the last change of this transform went from having to not + * having a transform or vice versa. * * (This is used as part of an optimization in * SVGTransformableElement::GetAttributeChangeHint. That function reports an * inexpensive nsChangeHint when a transform has just modified -- but this - * accessor lets it detect cases where the "modification" is actually adding + * accessor lets it detect cases where the "modification" is actually creating * a transform where we previously had none. These cases require a more * thorough nsChangeHint.) */ - bool RequiresFrameReconstruction() const { - return mRequiresFrameReconstruction; + bool CreatedOrRemovedOnLastChange() const { + return mCreatedOrRemovedOnLastChange; } mozilla::UniquePtr ToSMILAttr(dom::SVGElement* aSVGElement); @@ -119,8 +119,8 @@ class SVGAnimatedTransformList { SVGTransformList mBaseVal; nsAutoPtr mAnimVal; bool mIsAttrSet; - // (See documentation for accessor, RequiresFrameReconstruction.) - bool mRequiresFrameReconstruction; + // See documentation for accessor. + bool mCreatedOrRemovedOnLastChange; struct SMILAnimatedTransformList : public SMILAttr { public: diff --git a/dom/svg/SVGTransformableElement.cpp b/dom/svg/SVGTransformableElement.cpp index 56484b9b2115..500541f7be99 100644 --- a/dom/svg/SVGTransformableElement.cpp +++ b/dom/svg/SVGTransformableElement.cpp @@ -67,16 +67,16 @@ nsChangeHint SVGTransformableElement::GetAttributeChangeHint( "Unknown modification type."); if (!mTransforms || !mTransforms->HasTransform()) { // New value is empty, treat as removal. + // FIXME: Should we just rely on CreatedOrRemovedOnLastChange? isAdditionOrRemoval = true; - } else if (mTransforms->RequiresFrameReconstruction()) { + } else if (mTransforms->CreatedOrRemovedOnLastChange()) { // Old value was empty, treat as addition. isAdditionOrRemoval = true; } } if (isAdditionOrRemoval) { - // Reconstruct the frame tree to handle stacking context changes: - retval |= nsChangeHint_ReconstructFrame; + retval |= nsChangeHint_ComprehensiveAddOrRemoveTransform; } else { // We just assume the old and new transforms are different. retval |= nsChangeHint_UpdatePostTransformOverflow |