Fix handling of multiple keyframes at same offset

third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 189 matching lines @@
 }
 
 void KeyframeEffectModelBase::ensureInterpolationEffectPopulated() const
 {
     if (m_interpolationEffect.isPopulated())
         return;
 
     for (const auto& entry : *m_keyframeGroups) {
         const PropertySpecificKeyframeVector& keyframes = entry.value->keyframes();
         for (size_t i = 0; i < keyframes.size() - 1; i++) {
-            double applyFrom = i ? keyframes[i]->offset() : (-std::numeric_limits<double>::infinity());
-            double applyTo = i == keyframes.size() - 2 ? std::numeric_limits<double>::infinity() : keyframes[i + 1]->offset();
-            if (applyTo == 1)
+            size_t startKeyframe = i;

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

I'd call these startIndex and endIndex to avoid confusing them with Keyframes.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Done.

+            size_t endKeyframe = i+1;
+            double startOffset = keyframes[startKeyframe]->offset();
+            double endOffset = keyframes[endKeyframe]->offset();
+            double applyFrom = startOffset;
+            double applyTo = endOffset;
+
+            if (i == 0) {
+                applyFrom = -std::numeric_limits<double>::infinity();
+                ASSERT(startOffset == 0.0);
+                if (endOffset == 0.0) {
+                    ASSERT(endKeyframe + 1 < keyframes.size()

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

No need to check this, WTF::Vector indexing comes with bounds checking.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Done.

+                        && keyframes[endKeyframe + 1]->offset() != 0.0);
+                    endKeyframe = startKeyframe;
+                }
+            }
+            if (i == keyframes.size() - 2) {
                 applyTo = std::numeric_limits<double>::infinity();
+                ASSERT(endOffset == 1.0);
+                if (startOffset == 1.0) {
+                    ASSERT(startKeyframe > 1

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

Won't this fail with [{}, {offset: 1}, {}]?
I don't think this check is necessary with bounds checking.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Ah, typo, that should have been startKeyframe >= 1 (or > 0) but at any rate it
is now obsolete.

+                        && keyframes[startKeyframe - 1]->offset() != 1.0);
+                    startKeyframe = endKeyframe;
+                }
+            }
 
-            m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i + 1], applyFrom, applyTo);
+            if (applyFrom != applyTo) {
+                m_interpolationEffect.addInterpolationsFromKeyframes(
+                    entry.key, *keyframes[startKeyframe], *keyframes[endKeyframe], applyFrom, applyTo);
+            }
+            // else the interpolation will never be used in sampling
         }
     }
 
     m_interpolationEffect.setPopulated();
 }
 
 bool KeyframeEffectModelBase::isReplaceOnly()
 {
     ensureKeyframeGroups();
     for (const auto& entry : *m_keyframeGroups) {
@@ skipping 14 matching lines @@
 }
 
 void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::appendKeyframe(PassRefPtr<Keyframe::PropertySpecificKeyframe> keyframe)
 {
     ASSERT(m_keyframes.isEmpty() || m_keyframes.last()->offset() <= keyframe->offset());
     m_keyframes.append(keyframe);
 }
 
 void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::removeRedundantKeyframes()
 {
-    // As an optimization, removes keyframes in the following categories, as
-    // they will never be used by sample().
-    // - End keyframes with the same offset as their neighbor
-    // - Interior keyframes with the same offset as both their neighbors
+    // As an optimization, removes interior keyframes that have the same offset
+    // as both their neighbours, as they will never be used by sample().
     // Note that synthetic keyframes must be added before this method is
     // called.
     ASSERT(m_keyframes.size() >= 2);
-    for (int i = m_keyframes.size() - 1; i >= 0; --i) {
+    for (int i = m_keyframes.size() - 2; i > 0; --i) {
         double offset = m_keyframes[i]->offset();
-        bool hasSameOffsetAsPreviousNeighbor = !i || m_keyframes[i - 1]->offset() == offset;
-        bool hasSameOffsetAsNextNeighbor = i == static_cast<int>(m_keyframes.size() - 1) || m_keyframes[i + 1]->offset() == offset;
+        bool hasSameOffsetAsPreviousNeighbor = m_keyframes[i - 1]->offset() == offset;
+        bool hasSameOffsetAsNextNeighbor = m_keyframes[i + 1]->offset() == offset;
         if (hasSameOffsetAsPreviousNeighbor && hasSameOffsetAsNextNeighbor)
             m_keyframes.remove(i);
     }
     ASSERT(m_keyframes.size() >= 2);
 }
 
 bool KeyframeEffectModelBase::PropertySpecificKeyframeGroup::addSyntheticKeyframeIfRequired(PassRefPtr<TimingFunction> zeroOffsetEasing)
 {
     ASSERT(!m_keyframes.isEmpty());
 
     bool addedSyntheticKeyframe = false;
 
     if (m_keyframes.first()->offset() != 0.0) {
         m_keyframes.insert(0, m_keyframes.first()->neutralKeyframe(0, zeroOffsetEasing));
         addedSyntheticKeyframe = true;
     }
     if (m_keyframes.last()->offset() != 1.0) {
         appendKeyframe(m_keyframes.last()->neutralKeyframe(1, nullptr));
         addedSyntheticKeyframe = true;
     }
 
     return addedSyntheticKeyframe;
 }
 
 } // namespace blink