Rework the SVGPathConsumer interface

Source/core/svg/SVGPathBlender.cpp

Index: Source/core/svg/SVGPathBlender.cpp
diff --git a/Source/core/svg/SVGPathBlender.cpp b/Source/core/svg/SVGPathBlender.cpp
index 64a9374cfdfc6178487592df5b36b170600068f1..3e0fb27661e770a10ee6c9dd825b94d8b537384a 100644
--- a/Source/core/svg/SVGPathBlender.cpp
+++ b/Source/core/svg/SVGPathBlender.cpp
@@ -113,101 +113,168 @@ FloatPoint SVGPathBlender::blendAnimatedFloatPoint(const FloatPoint& fromPoint,
     return animatedPoint;
 }
 
-void SVGPathBlender::blendMoveToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendMoveToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->moveTo(blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint), m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendLineToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendLineToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->lineTo(blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint), m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendLineToHorizontalSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendLineToHorizontalSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    float fromX = fromSeg.targetPoint.x();
-    float toX = toSeg.targetPoint.x();
-    m_consumer->lineToHorizontal(blendAnimatedDimensonalFloat(fromX, toX, BlendHorizontal), m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
-    m_fromCurrentPoint.setX(m_fromMode == AbsoluteCoordinates ? fromX : m_fromCurrentPoint.x() + fromX);
-    m_toCurrentPoint.setX(m_toMode == AbsoluteCoordinates ? toX : m_toCurrentPoint.x() + toX);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint.setX(blendAnimatedDimensonalFloat(fromSeg.targetPoint.x(), toSeg.targetPoint.x(), BlendHorizontal));
+
+    m_fromCurrentPoint.setX(m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint.x() : m_fromCurrentPoint.x() + fromSeg.targetPoint.x());
+    m_toCurrentPoint.setX(m_toMode == AbsoluteCoordinates ? toSeg.targetPoint.x() : m_toCurrentPoint.x() + toSeg.targetPoint.x());
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendLineToVerticalSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendLineToVerticalSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    float fromY = fromSeg.targetPoint.y();
-    float toY = toSeg.targetPoint.y();
-    m_consumer->lineToVertical(blendAnimatedDimensonalFloat(fromY, toY, BlendVertical), m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
-    m_fromCurrentPoint.setY(m_fromMode == AbsoluteCoordinates ? fromY : m_fromCurrentPoint.y() + fromY);
-    m_toCurrentPoint.setY(m_toMode == AbsoluteCoordinates ? toY : m_toCurrentPoint.y() + toY);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint.setY(blendAnimatedDimensonalFloat(fromSeg.targetPoint.y(), toSeg.targetPoint.y(), BlendVertical));
+
+    m_fromCurrentPoint.setY(m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint.y() : m_fromCurrentPoint.y() + fromSeg.targetPoint.y());
+    m_toCurrentPoint.setY(m_toMode == AbsoluteCoordinates ? toSeg.targetPoint.y() : m_toCurrentPoint.y() + toSeg.targetPoint.y());
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendCurveToCubicSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendCurveToCubicSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->curveToCubic(
-        blendAnimatedFloatPoint(fromSeg.point1, toSeg.point1),
-        blendAnimatedFloatPoint(fromSeg.point2, toSeg.point2),
-        blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint),
-        m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+    blendedSegment.point1 = blendAnimatedFloatPoint(fromSeg.point1, toSeg.point1);
+    blendedSegment.point2 = blendAnimatedFloatPoint(fromSeg.point2, toSeg.point2);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendCurveToCubicSmoothSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendCurveToCubicSmoothSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->curveToCubicSmooth(
-        blendAnimatedFloatPoint(fromSeg.point2, toSeg.point2),
-        blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint),
-        m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+    blendedSegment.point2 = blendAnimatedFloatPoint(fromSeg.point2, toSeg.point2);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendCurveToQuadraticSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendCurveToQuadraticSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->curveToQuadratic(
-        blendAnimatedFloatPoint(fromSeg.point1, toSeg.point1),
-        blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint),
-        m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+    blendedSegment.point1 = blendAnimatedFloatPoint(fromSeg.point1, toSeg.point1);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendCurveToQuadraticSmoothSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendCurveToQuadraticSmoothSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
-    m_consumer->curveToQuadraticSmooth(blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint), m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
 }
 
-void SVGPathBlender::blendArcToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+PathSegmentData SVGPathBlender::blendArcToSegment(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
 {
     ASSERT(!m_addTypesCount || fromSeg.command == toSeg.command);
 
-    bool blendedLargeArc;
-    bool blendedSweep;
-
+    PathSegmentData blendedSegment;
+    blendedSegment.command = m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;

[f(malita), 2015/03/25 15:08:15]

This is common logic within all these methods - would it look nicer if we
instead handle it in blendSegments() (and pass the result blendedSegment as an
out-param)?

(maybe trading functional encapsulation for DRYness is not worth it - up to you)

[fs, 2015/03/25 17:08:13]

Yes, I think I have a follow-up that "commons up" this code a bit. There's more
common ground here. Expect something soonish.

+    blendedSegment.targetPoint = blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint);
+    blendedSegment.point1 = blendAnimatedFloatPointSameCoordinates(fromSeg.arcRadii(), toSeg.arcRadii());
+    blendedSegment.point2 = blendAnimatedFloatPointSameCoordinates(fromSeg.point2, toSeg.point2);
     if (m_addTypesCount) {
-        blendedLargeArc = fromSeg.arcLarge || toSeg.arcLarge;
-        blendedSweep = fromSeg.arcSweep || toSeg.arcSweep;
+        blendedSegment.arcLarge = fromSeg.arcLarge || toSeg.arcLarge;
+        blendedSegment.arcSweep = fromSeg.arcSweep || toSeg.arcSweep;
     } else {
-        blendedLargeArc = m_isInFirstHalfOfAnimation ? fromSeg.arcLarge : toSeg.arcLarge;
-        blendedSweep = m_isInFirstHalfOfAnimation ? fromSeg.arcSweep : toSeg.arcSweep;
+        blendedSegment.arcLarge = m_isInFirstHalfOfAnimation ? fromSeg.arcLarge : toSeg.arcLarge;
+        blendedSegment.arcSweep = m_isInFirstHalfOfAnimation ? fromSeg.arcSweep : toSeg.arcSweep;
     }
 
-    FloatPoint blendedRadii = blendAnimatedFloatPointSameCoordinates(fromSeg.arcRadii(), toSeg.arcRadii());
-    float blendedAngle = blendAnimatedFloatPointSameCoordinates(fromSeg.point2, toSeg.point2).x();
-
-    m_consumer->arcTo(
-        blendedRadii.x(), blendedRadii.y(), blendedAngle, blendedLargeArc, blendedSweep,
-        blendAnimatedFloatPoint(fromSeg.targetPoint, toSeg.targetPoint),
-        m_isInFirstHalfOfAnimation ? m_fromMode : m_toMode);
-
     m_fromCurrentPoint = m_fromMode == AbsoluteCoordinates ? fromSeg.targetPoint : m_fromCurrentPoint + fromSeg.targetPoint;
     m_toCurrentPoint = m_toMode == AbsoluteCoordinates ? toSeg.targetPoint : m_toCurrentPoint + toSeg.targetPoint;
+    return blendedSegment;
+}
+
+void SVGPathBlender::blendSegments(const PathSegmentData& fromSeg, const PathSegmentData& toSeg)
+{
+    PathSegmentData blendedSegment;
+    switch (toSeg.command) {
+    case PathSegMoveToRel:
+    case PathSegMoveToAbs:
+        blendedSegment = blendMoveToSegment(fromSeg, toSeg);
+        break;
+    case PathSegLineToRel:
+    case PathSegLineToAbs:
+        blendedSegment = blendLineToSegment(fromSeg, toSeg);
+        break;
+    case PathSegLineToHorizontalRel:
+    case PathSegLineToHorizontalAbs:
+        blendedSegment = blendLineToHorizontalSegment(fromSeg, toSeg);
+        break;
+    case PathSegLineToVerticalRel:
+    case PathSegLineToVerticalAbs:
+        blendedSegment = blendLineToVerticalSegment(fromSeg, toSeg);
+        break;
+    case PathSegClosePath:
+        blendedSegment = toSeg;
+        break;
+    case PathSegCurveToCubicRel:
+    case PathSegCurveToCubicAbs:
+        blendedSegment = blendCurveToCubicSegment(fromSeg, toSeg);
+        break;
+    case PathSegCurveToCubicSmoothRel:
+    case PathSegCurveToCubicSmoothAbs:
+        blendedSegment = blendCurveToCubicSmoothSegment(fromSeg, toSeg);
+        break;
+    case PathSegCurveToQuadraticRel:
+    case PathSegCurveToQuadraticAbs:
+        blendedSegment = blendCurveToQuadraticSegment(fromSeg, toSeg);
+        break;
+    case PathSegCurveToQuadraticSmoothRel:
+    case PathSegCurveToQuadraticSmoothAbs:
+        blendedSegment = blendCurveToQuadraticSmoothSegment(fromSeg, toSeg);
+        break;
+    case PathSegArcRel:
+    case PathSegArcAbs:
+        blendedSegment = blendArcToSegment(fromSeg, toSeg);
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+    }
+
+    m_consumer->emitSegment(blendedSegment);

[f(malita), 2015/03/25 15:08:15]

Do you know if we have any perf coverage for this code? Any chance to run some
tests and make sure we're not regressing?

I'm just a tad worried because now we're going through this intermediate segment
representation instead of the previously more direct consumer calls.

[fs, 2015/03/25 17:08:13]

I'm not aware of any tests like that, so I suppose I'd have to ad-hoc something
together. Since I measured a general improvement for the parser case I'm not
terribly worried about the  "dispatch cost" - PathSegmentData is small (< 32
bytes, so should fit in a single cacheline on anything we care about), and
otherwise the cost should be the same - so it's mainly the cost of the switch
above I think. I'll see if I can dig up some animated path that is heavy enough
(hopefully I have one on file already somewhere...)

[fs, 2015/03/25 17:56:11]

I extracted some path animations from some SVGs I had lying around - producing
[1], and then multiplied those by a few hundred (lost count, but ~600 I think).
SVGPathBlender barely registered on the profile. The emitSegment() consumer
showed up with ~CoalescingBuffer (inlined) as the hotspot (unsurprisingly, and
nothing to do with this CL really). Otherwise it seems the (SMIL) animation code
is too inefficient when it comes to path animations (I suspect it reparses the
values too often.) So any further investigation would need to be of the
microbench kind to not get shadowed by the other bottlenecks...

FWIW I'll try to look into this some more if possible (I've been wanting to
squash the ~CoalescingBuffer inefficiency mentioned above...)

[1] http://jsfiddle.net/e33ybbdh/

 }
 
 static inline PathCoordinateMode coordinateModeOfCommand(const SVGPathSegType& type)
@@ -257,49 +324,7 @@ bool SVGPathBlender::blendAnimatedPath(float progress)
         if (m_addTypesCount && m_fromMode != m_toMode)
             return false;
 
-        switch (toSeg.command) {
-        case PathSegMoveToRel:
-        case PathSegMoveToAbs:
-            blendMoveToSegment(fromSeg, toSeg);
-            break;
-        case PathSegLineToRel:
-        case PathSegLineToAbs:
-            blendLineToSegment(fromSeg, toSeg);
-            break;
-        case PathSegLineToHorizontalRel:
-        case PathSegLineToHorizontalAbs:
-            blendLineToHorizontalSegment(fromSeg, toSeg);
-            break;
-        case PathSegLineToVerticalRel:
-        case PathSegLineToVerticalAbs:
-            blendLineToVerticalSegment(fromSeg, toSeg);
-            break;
-        case PathSegClosePath:
-            m_consumer->closePath();
-            break;
-        case PathSegCurveToCubicRel:
-        case PathSegCurveToCubicAbs:
-            blendCurveToCubicSegment(fromSeg, toSeg);
-            break;
-        case PathSegCurveToCubicSmoothRel:
-        case PathSegCurveToCubicSmoothAbs:
-            blendCurveToCubicSmoothSegment(fromSeg, toSeg);
-            break;
-        case PathSegCurveToQuadraticRel:
-        case PathSegCurveToQuadraticAbs:
-            blendCurveToQuadraticSegment(fromSeg, toSeg);
-            break;
-        case PathSegCurveToQuadraticSmoothRel:
-        case PathSegCurveToQuadraticSmoothAbs:
-            blendCurveToQuadraticSmoothSegment(fromSeg, toSeg);
-            break;
-        case PathSegArcRel:
-        case PathSegArcAbs:
-            blendArcToSegment(fromSeg, toSeg);
-            break;
-        default:
-            ASSERT_NOT_REACHED();
-        }
+        blendSegments(fromSeg, toSeg);
 
         if (!fromSourceHadData)
             continue;