Rework the SVGPathConsumer interface

Source/core/svg/SVGPathBlender.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010, 2011. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 95 matching lines @@
 
     // Transform the animated point to the coordinate mode, needed for the current progress.
     FloatPoint currentPoint = blendFloatPoint(m_fromCurrentPoint, m_toCurrentPoint, m_progress);
     if (m_toMode == AbsoluteCoordinates)
         return animatedPoint + currentPoint;
 
     animatedPoint.move(-currentPoint.x(), -currentPoint.y());
     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)
 {
     if (type < PathSegMoveToAbs)
         return AbsoluteCoordinates;
 
     // Odd number = relative command
     if (type % 2)
         return RelativeCoordinates;
@@ skipping 29 matching lines @@
 
         if (toAbsolutePathSegType(fromSeg.command) != toAbsolutePathSegType(toSeg.command))
             return false;
 
         m_fromMode = coordinateModeOfCommand(fromSeg.command);
         m_toMode = coordinateModeOfCommand(toSeg.command);
 
         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;
         if (m_fromSource->hasMoreData() != m_toSource->hasMoreData())
             return false;
     }
     return true;
 }
 
 }