Decouple animation value from SVGAnimateElements

SVGAnimateElement currently holds on to the computed animation value,
and one of the animation elements in a sandwich is appointed the
"result element". This value management is unnecessarily complex, since
the lifecycle of the value is roughly create->compute->apply, and after
having applied the value we no longer need it because a new one will be
created on the next animation update. Clearing the value does not
require actually having the value itself.
Instead introduce a SMILAnimationValue structure which essentially wraps
all possible animation value types. This structure is setup by
CreateAnimationValue() (previously ResetAnimatedType()) and then
subsequently passed to ApplyAnimation() (CalculateAnimatedValue()) and
ApplyResultsToTarget().

ClearAnimatedType() is renamed to ClearAnimationValue() and
CalculateAnimatedValue() is renamed to CalculateAnimationValue().

For SVGAnimateMotionElement this means we can compute the resulting
transform and then apply it as the final step - i.e we don't need to
compute it in-place. This should allow simplifying the application logic
for motion paths in the future.

Bug: 1017723
Change-Id: I205e83a8c471bbe8becb0bc4a686086d811eb482
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523183
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825299}

third_party/blink/renderer/core/svg/animation/smil_animation_sandwich.cc

@@ -27,6 +27,7 @@
 
 #include <algorithm>
 
+#include "third_party/blink/renderer/core/svg/animation/smil_animation_value.h"
 #include "third_party/blink/renderer/core/svg/svg_animation_element.h"
 
 namespace blink {
@@ -55,16 +56,14 @@ void SMILAnimationSandwich::Remove(SVGAnimationElement* animation) {
   auto* position = std::find(sandwich_.begin(), sandwich_.end(), animation);
   DCHECK(sandwich_.end() != position);
   sandwich_.erase(position);
-  if (animation == ResultElement()) {
-    animation->ClearAnimatedType();
+  // If the sandwich is now empty, clear any animated value if there are active
+  // animation elements.
+  if (sandwich_.IsEmpty() && !active_.IsEmpty()) {
+    animation->ClearAnimationValue();
     active_.Shrink(0);
   }
 }
 
-SVGAnimationElement* SMILAnimationSandwich::ResultElement() const {
-  return !active_.IsEmpty() ? active_.front() : nullptr;
-}
-
 void SMILAnimationSandwich::UpdateActiveAnimationStack(
     SMILTime presentation_time) {
   if (!std::is_sorted(sandwich_.begin(), sandwich_.end(),
@@ -73,7 +72,6 @@ void SMILAnimationSandwich::UpdateActiveAnimationStack(
               PriorityCompare(presentation_time));
   }
 
-  SVGAnimationElement* old_result_element = ResultElement();
   active_.Shrink(0);
   active_.ReserveCapacity(sandwich_.size());
   // Build the contributing/active sandwich.
@@ -83,15 +81,19 @@ void SMILAnimationSandwich::UpdateActiveAnimationStack(
     animation->UpdateProgressState(presentation_time);
     active_.push_back(animation);
   }
-  // If we switched result element, clear the old one.
-  if (old_result_element && old_result_element != ResultElement())
-    old_result_element->ClearAnimatedType();
 }
 
 bool SMILAnimationSandwich::ApplyAnimationValues() {
-  SVGAnimationElement* result_element = ResultElement();
-  if (!result_element)
+  // For now we need an element to setup and apply an animation. Any animation
+  // element in the sandwich will do.
+  SVGAnimationElement* animation = sandwich_.front();
+
+  // If the sandwich does not have any active elements, clear any animated
+  // value.
+  if (active_.IsEmpty()) {
+    animation->ClearAnimationValue();
     return false;
+  }
 
   // Animations have to be applied lowest to highest prio.
   //
@@ -111,14 +113,15 @@ bool SMILAnimationSandwich::ApplyAnimationValues() {
   // Only reset the animated type to the base value once for
   // the lowest priority animation that animates and
   // contributes to a particular element/attribute pair.
-  result_element->ResetAnimatedType(needs_underlying_value);
+  SMILAnimationValue animation_value =
+      animation->CreateAnimationValue(needs_underlying_value);
 
   for (auto* sandwich_it = sandwich_start; sandwich_it != active_.end();
        sandwich_it++) {
-    (*sandwich_it)->ApplyAnimation(result_element);
+    (*sandwich_it)->ApplyAnimation(animation_value);
   }
 
-  result_element->ApplyResultsToTarget();
+  animation->ApplyResultsToTarget(animation_value);
   return true;
 }
 

third_party/blink/renderer/core/svg/animation/smil_animation_sandwich.h

@@ -109,11 +109,6 @@ class SMILAnimationSandwich : public GarbageCollected<SMILAnimationSandwich> {
   void Trace(Visitor*) const;
 
  private:
-  // Results are accumulated to the first animation element that animates and
-  // contributes to a particular element/attribute pair. We refer to this as
-  // the "result element".
-  SVGAnimationElement* ResultElement() const;
-
   using AnimationsVector = HeapVector<Member<SVGAnimationElement>>;
 
   // All the animation (really: timed) elements that make up the sandwich,

third_party/blink/renderer/core/svg/animation/smil_animation_value.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_SVG_ANIMATION_SMIL_ANIMATION_VALUE_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_SVG_ANIMATION_SMIL_ANIMATION_VALUE_H_
+
+#include "third_party/blink/renderer/platform/transforms/affine_transform.h"
+#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
+
+namespace blink {
+
+class SVGPropertyBase;
+
+// Representation of value being computed and applied. Different fields will be
+// used depending on which target is being animated (attribute/property
+// vs. motion path) but always the same field within the same sandwich.
+struct SMILAnimationValue {
+  STACK_ALLOCATED();
+
+ public:
+  SVGPropertyBase* property_value = nullptr;
+  AffineTransform motion_transform;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_SVG_ANIMATION_SMIL_ANIMATION_VALUE_H_

third_party/blink/renderer/core/svg/build.gni

@@ -9,6 +9,7 @@ blink_core_sources_svg = [
   "animation/smil_animation_effect_parameters.h",
   "animation/smil_animation_sandwich.cc",
   "animation/smil_animation_sandwich.h",
+  "animation/smil_animation_value.h",
   "animation/smil_repeat_count.h",
   "animation/smil_time.cc",
   "animation/smil_time.h",

third_party/blink/renderer/core/svg/svg_animate_element.cc

@@ -28,6 +28,7 @@
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/qualified_name.h"
 #include "third_party/blink/renderer/core/svg/animation/smil_animation_effect_parameters.h"
+#include "third_party/blink/renderer/core/svg/animation/smil_animation_value.h"
 #include "third_party/blink/renderer/core/svg/properties/svg_animated_property.h"
 #include "third_party/blink/renderer/core/svg/properties/svg_property.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_color.h"
@@ -332,25 +333,23 @@ static SVGPropertyBase* DiscreteSelectValue(AnimationMode animation_mode,
   return from;
 }
 
-void SVGAnimateElement::CalculateAnimatedValue(
+void SVGAnimateElement::CalculateAnimationValue(
+    SMILAnimationValue& animation_value,
     float percentage,
-    unsigned repeat_count,
-    SVGSMILElement* result_element) const {
-  DCHECK(result_element);
+    unsigned repeat_count) const {
   DCHECK(targetElement());
-  if (!IsSVGAnimateElement(*result_element))
-    return;
-
   DCHECK(percentage >= 0 && percentage <= 1);
   DCHECK_NE(type_, kAnimatedUnknown);
   DCHECK(from_property_);
   DCHECK_EQ(from_property_->GetType(), type_);
   DCHECK(to_property_);
 
-  auto* result_animation_element = To<SVGAnimateElement>(result_element);
-  DCHECK(result_animation_element->animated_value_);
-  DCHECK_EQ(result_animation_element->type_, type_);
+  DCHECK(animation_value.property_value);
+  DCHECK_EQ(animation_value.property_value->GetType(), type_);
 
+  // The semantics of the 'set' element is that it always (and only) sets the
+  // 'to' value. (It is also always set as a 'to' animation and will thus never
+  // be additive or cumulative.)
   if (IsA<SVGSetElement>(*this))
     percentage = 1;
 
@@ -359,7 +358,7 @@ void SVGAnimateElement::CalculateAnimatedValue(
 
   // Values-animation accumulates using the last values entry corresponding to
   // the end of duration time.
-  SVGPropertyBase* animated_value = result_animation_element->animated_value_;
+  SVGPropertyBase* animated_value = animation_value.property_value;
   SVGPropertyBase* to_at_end_of_duration_value =
       to_at_end_of_duration_property_ ? to_at_end_of_duration_property_
                                       : to_property_;
@@ -375,7 +374,7 @@ void SVGAnimateElement::CalculateAnimatedValue(
   // If the animated type can only be animated discretely, then do that here,
   // replacing |result_element|s animated value.
   if (!AnimatedPropertyTypeSupportsAddition()) {
-    result_animation_element->animated_value_ = DiscreteSelectValue(
+    animation_value.property_value = DiscreteSelectValue(
         GetAnimationMode(), percentage, from_value, to_value);
     return;
   }
@@ -426,28 +425,31 @@ bool SVGAnimateElement::CalculateFromAndByValues(const String& from_string,
   return true;
 }
 
-void SVGAnimateElement::ResetAnimatedType(bool needs_underlying_value) {
+SMILAnimationValue SVGAnimateElement::CreateAnimationValue(
+    bool needs_underlying_value) const {
   DCHECK(targetElement());
+  SMILAnimationValue animation_value;
   if (IsAnimatingSVGDom()) {
     // SVG DOM animVal animation code-path.
-    animated_value_ = target_property_->CreateAnimatedValue();
-    DCHECK_EQ(animated_value_->GetType(), type_);
-    return;
+    animation_value.property_value = target_property_->CreateAnimatedValue();
+    DCHECK_EQ(animation_value.property_value->GetType(), type_);
+  } else {
+    DCHECK(IsAnimatingCSSProperty());
+    // Presentation attributes that have an SVG DOM representation should use
+    // the "SVG DOM" code-path (above.)
+    DCHECK(SVGElement::IsAnimatableCSSProperty(AttributeName()));
+
+    // CSS properties animation code-path.
+    String base_value =
+        needs_underlying_value
+            ? ComputeCSSPropertyValue(targetElement(), css_property_id_)
+            : g_empty_string;
+    animation_value.property_value = CreatePropertyForAnimation(base_value);
   }
-  DCHECK(IsAnimatingCSSProperty());
-  // Presentation attributes which has an SVG DOM representation should use the
-  // "SVG DOM" code-path (above.)
-  DCHECK(SVGElement::IsAnimatableCSSProperty(AttributeName()));
-
-  // CSS properties animation code-path.
-  String base_value =
-      needs_underlying_value
-          ? ComputeCSSPropertyValue(targetElement(), css_property_id_)
-          : g_empty_string;
-  animated_value_ = CreatePropertyForAnimation(base_value);
+  return animation_value;
 }
 
-void SVGAnimateElement::ClearAnimatedType() {
+void SVGAnimateElement::ClearAnimationValue() {
   SVGElement* target_element = targetElement();
   DCHECK(target_element);
 
@@ -464,18 +466,18 @@ void SVGAnimateElement::ClearAnimatedType() {
   // SVG DOM animVal animation code-path.
   if (IsAnimatingSVGDom())
     target_element->ClearAnimatedAttribute(AttributeName());
-
-  animated_value_.Clear();
 }
 
-void SVGAnimateElement::ApplyResultsToTarget() {
-  DCHECK(animated_value_);
+void SVGAnimateElement::ApplyResultsToTarget(
+    const SMILAnimationValue& animation_value) {
+  DCHECK(animation_value.property_value);
   DCHECK(targetElement());
   DCHECK_NE(type_, kAnimatedUnknown);
 
   // We do update the style and the animation property independent of each
   // other.
   SVGElement* target_element = targetElement();
+  SVGPropertyBase* animated_value = animation_value.property_value;
 
   // CSS properties animation code-path.
   if (IsAnimatingCSSProperty()) {
@@ -483,7 +485,7 @@ void SVGAnimateElement::ApplyResultsToTarget() {
     // property on the target_element.
     MutableCSSPropertyValueSet* properties =
         target_element->EnsureAnimatedSMILStyleProperties();
-    auto animated_value_string = animated_value_->ValueAsString();
+    auto animated_value_string = animated_value->ValueAsString();
     auto& document = target_element->GetDocument();
     auto set_result = properties->SetProperty(
         css_property_id_, animated_value_string, false,
@@ -497,7 +499,7 @@ void SVGAnimateElement::ApplyResultsToTarget() {
   }
   // SVG DOM animVal animation code-path.
   if (IsAnimatingSVGDom())
-    target_element->SetAnimatedAttribute(AttributeName(), animated_value_);
+    target_element->SetAnimatedAttribute(AttributeName(), animated_value);
 }
 
 bool SVGAnimateElement::AnimatedPropertyTypeSupportsAddition() const {
@@ -527,8 +529,6 @@ float SVGAnimateElement::CalculateDistance(const String& from_string,
 
 void SVGAnimateElement::WillChangeAnimatedType() {
   UnregisterAnimation(attribute_name_);
-  // Should've been cleared by the above if needed.
-  DCHECK(!animated_value_);
   from_property_.Clear();
   to_property_.Clear();
   to_at_end_of_duration_property_.Clear();
@@ -577,7 +577,6 @@ void SVGAnimateElement::Trace(Visitor* visitor) const {
   visitor->Trace(from_property_);
   visitor->Trace(to_property_);
   visitor->Trace(to_at_end_of_duration_property_);
-  visitor->Trace(animated_value_);
   visitor->Trace(target_property_);
   SVGAnimationElement::Trace(visitor);
 }

third_party/blink/renderer/core/svg/svg_animate_element.h

@@ -61,8 +61,9 @@ class CORE_EXPORT SVGAnimateElement : public SVGAnimationElement {
 
   bool HasValidAnimation() const override;
 
-  void ResetAnimatedType(bool needs_underlying_value) final;
-  void ClearAnimatedType() final;
+  SMILAnimationValue CreateAnimationValue(
+      bool needs_underlying_value) const final;
+  void ClearAnimationValue() final;
 
   bool CalculateToAtEndOfDurationValue(
       const String& to_at_end_of_duration_string) final;
@@ -70,10 +71,10 @@ class CORE_EXPORT SVGAnimateElement : public SVGAnimationElement {
                                 const String& to_string) final;
   bool CalculateFromAndByValues(const String& from_string,
                                 const String& by_string) final;
-  void CalculateAnimatedValue(float percentage,
-                              unsigned repeat_count,
-                              SVGSMILElement* result_element) const final;
-  void ApplyResultsToTarget() final;
+  void CalculateAnimationValue(SMILAnimationValue&,
+                               float percentage,
+                               unsigned repeat_count) const final;
+  void ApplyResultsToTarget(const SMILAnimationValue&) final;
   float CalculateDistance(const String& from_string,
                           const String& to_string) final;
 
@@ -114,7 +115,6 @@ class CORE_EXPORT SVGAnimateElement : public SVGAnimationElement {
   Member<SVGPropertyBase> from_property_;
   Member<SVGPropertyBase> to_property_;
   Member<SVGPropertyBase> to_at_end_of_duration_property_;
-  Member<SVGPropertyBase> animated_value_;
 
  protected:
   Member<SVGAnimatedPropertyBase> target_property_;

third_party/blink/renderer/core/svg/svg_animate_motion_element.cc

@@ -24,6 +24,7 @@
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/svg/animation/smil_animation_effect_parameters.h"
+#include "third_party/blink/renderer/core/svg/animation/smil_animation_value.h"
 #include "third_party/blink/renderer/core/svg/svg_mpath_element.h"
 #include "third_party/blink/renderer/core/svg/svg_parser_utilities.h"
 #include "third_party/blink/renderer/core/svg/svg_path_element.h"
@@ -153,16 +154,14 @@ static bool ParsePoint(const String& string, FloatPoint& point) {
   });
 }
 
-void SVGAnimateMotionElement::ResetAnimatedType(bool needs_underlying_value) {
-  SVGElement* target_element = targetElement();
-  DCHECK(target_element);
-  DCHECK(TargetCanHaveMotionTransform(*target_element));
-  AffineTransform* transform = target_element->AnimateMotionTransform();
-  DCHECK(transform);
-  transform->MakeIdentity();
+SMILAnimationValue SVGAnimateMotionElement::CreateAnimationValue(
+    bool needs_underlying_value) const {
+  DCHECK(targetElement());
+  DCHECK(TargetCanHaveMotionTransform(*targetElement()));
+  return SMILAnimationValue();
 }
 
-void SVGAnimateMotionElement::ClearAnimatedType() {
+void SVGAnimateMotionElement::ClearAnimationValue() {
   SVGElement* target_element = targetElement();
   DCHECK(target_element);
   AffineTransform* transform = target_element->AnimateMotionTransform();
@@ -202,15 +201,12 @@ bool SVGAnimateMotionElement::CalculateFromAndByValues(
   return true;
 }
 
-void SVGAnimateMotionElement::CalculateAnimatedValue(float percentage,
-                                                     unsigned repeat_count,
-                                                     SVGSMILElement*) const {
+void SVGAnimateMotionElement::CalculateAnimationValue(
+    SMILAnimationValue& animation_value,
+    float percentage,
+    unsigned repeat_count) const {
   SMILAnimationEffectParameters parameters = ComputeEffectParameters();
-
-  SVGElement* target_element = targetElement();
-  DCHECK(target_element);
-  AffineTransform* transform = target_element->AnimateMotionTransform();
-  DCHECK(transform);
+  AffineTransform* transform = &animation_value.motion_transform;
 
   // If additive, we accumulate into the underlying (transform) value.
   if (!parameters.is_additive)
@@ -251,13 +247,15 @@ void SVGAnimateMotionElement::CalculateAnimatedValue(float percentage,
   transform->Rotate(angle);
 }
 
-void SVGAnimateMotionElement::ApplyResultsToTarget() {
+void SVGAnimateMotionElement::ApplyResultsToTarget(
+    const SMILAnimationValue& animation_value) {
   // We accumulate to the target element transform list so there is not much to
   // do here.
   SVGElement* target_element = targetElement();
   DCHECK(target_element);
   AffineTransform* target_transform = target_element->AnimateMotionTransform();
   DCHECK(target_transform);
+  *target_transform = animation_value.motion_transform;
 
   if (LayoutObject* target_layout_object = target_element->GetLayoutObject())
     InvalidateForAnimateMotionTransformChange(*target_layout_object);

third_party/blink/renderer/core/svg/svg_animate_motion_element.h

@@ -42,18 +42,19 @@ class SVGAnimateMotionElement final : public SVGAnimationElement {
 
   void ParseAttribute(const AttributeModificationParams&) override;
 
-  void ResetAnimatedType(bool needs_underlying_value) override;
-  void ClearAnimatedType() override;
+  SMILAnimationValue CreateAnimationValue(
+      bool needs_underlying_value) const override;
+  void ClearAnimationValue() override;
   bool CalculateToAtEndOfDurationValue(
       const String& to_at_end_of_duration_string) override;
   bool CalculateFromAndToValues(const String& from_string,
                                 const String& to_string) override;
   bool CalculateFromAndByValues(const String& from_string,
                                 const String& by_string) override;
-  void CalculateAnimatedValue(float percentage,
-                              unsigned repeat_count,
-                              SVGSMILElement* result_element) const override;
-  void ApplyResultsToTarget() override;
+  void CalculateAnimationValue(SMILAnimationValue&,
+                               float percentage,
+                               unsigned repeat_count) const override;
+  void ApplyResultsToTarget(const SMILAnimationValue&) override;
   float CalculateDistance(const String& from_string,
                           const String& to_string) override;
 

third_party/blink/renderer/core/svg/svg_animation_element.cc

@@ -659,7 +659,7 @@ SMILAnimationEffectParameters SVGAnimationElement::ComputeEffectParameters()
   return parameters;
 }
 
-void SVGAnimationElement::ApplyAnimation(SVGAnimationElement* result_element) {
+void SVGAnimationElement::ApplyAnimation(SMILAnimationValue& animation_value) {
   if (animation_valid_ == AnimationValidity::kUnknown) {
     if (CheckAnimationParameters()) {
       animation_valid_ = AnimationValidity::kValid;
@@ -709,8 +709,8 @@ void SVGAnimationElement::ApplyAnimation(SVGAnimationElement* result_element) {
   } else {
     effective_percent = percent;
   }
-  CalculateAnimatedValue(effective_percent, progress_state.repeat,
-                         result_element);
+  CalculateAnimationValue(animation_value, effective_percent,
+                          progress_state.repeat);
 }
 
 bool SVGAnimationElement::OverwritesUnderlyingAnimationValue() const {

third_party/blink/renderer/core/svg/svg_animation_element.h

@@ -52,6 +52,7 @@ enum CalcMode {
 };
 
 struct SMILAnimationEffectParameters;
+struct SMILAnimationValue;
 
 class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
   DEFINE_WRAPPERTYPEINFO();
@@ -71,13 +72,15 @@ class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
   DEFINE_ATTRIBUTE_EVENT_LISTENER(end, kEndEvent)
   DEFINE_ATTRIBUTE_EVENT_LISTENER(repeat, kRepeatEvent)
 
-  virtual void ResetAnimatedType(bool needs_underlying_value) = 0;
-  virtual void ClearAnimatedType() = 0;
-  virtual void ApplyResultsToTarget() = 0;
+  virtual SMILAnimationValue CreateAnimationValue(
+      bool needs_underlying_value) const = 0;
+  void ApplyAnimation(SMILAnimationValue&);
+  virtual void ApplyResultsToTarget(const SMILAnimationValue&) = 0;
+
+  virtual void ClearAnimationValue() = 0;
   // Returns true if this animation "sets" the value of the animation. Thus all
   // previous animations are rendered useless.
   bool OverwritesUnderlyingAnimationValue() const;
-  void ApplyAnimation(SVGAnimationElement* result_element);
 
  protected:
   SVGAnimationElement(const QualifiedName&, Document&);
@@ -128,9 +131,9 @@ class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
                                         const String& to_string) = 0;
   virtual bool CalculateFromAndByValues(const String& from_string,
                                         const String& by_string) = 0;
-  virtual void CalculateAnimatedValue(float percent,
-                                      unsigned repeat_count,
-                                      SVGSMILElement* result_element) const = 0;
+  virtual void CalculateAnimationValue(SMILAnimationValue&,
+                                       float percent,
+                                       unsigned repeat_count) const = 0;
   virtual float CalculateDistance(const String& /*fromString*/,
                                   const String& /*toString*/) {
     return -1.f;