Handle arc segments correctly for marker placement

When producing Path objects, arcs are converted to conic segments, and
when iterating those with Path::Apply(...) they are converted to
quadratic beziers. So the arc semantics are lost when
SVGMarkerDataBuilder gets the resulting segment. Thus it produces
markers for each quadratic bezier, which is incorrect.

Teach SVGMarkerDataBuilder to operate on the original path data too by
accepting SVGPathByteStreams, and contort the arc segments into single
(faux) beziers with matching properties (same start/end tangents). This
allows reuse of the rest of the marker data builder machinery.
Use the new code-path for the cases which can contain arc segments
(<path> elements with 'd').

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

third_party/blink/renderer/core/layout/svg/layout_svg_path.cc

@@ -56,6 +56,12 @@ void LayoutSVGPath::UpdateShapeFromElement() {
   UpdateMarkers();
 }
 
+const StylePath* LayoutSVGPath::GetStylePath() const {
+  if (!IsA<SVGPathElement>(*GetElement()))
+    return nullptr;
+  return StyleRef().SvgStyle().D();
+}
+
 void LayoutSVGPath::UpdateMarkers() {
   marker_positions_.clear();
 
@@ -74,7 +80,11 @@ void LayoutSVGPath::UpdateMarkers() {
   if (!(marker_start || marker_mid || marker_end))
     return;
 
-  SVGMarkerDataBuilder(marker_positions_).Build(GetPath());
+  SVGMarkerDataBuilder builder(marker_positions_);
+  if (const StylePath* style_path = GetStylePath())
+    builder.Build(style_path->ByteStream());
+  else
+    builder.Build(GetPath());
 
   if (marker_positions_.IsEmpty())
     return;

third_party/blink/renderer/core/layout/svg/layout_svg_path.h

@@ -47,6 +47,7 @@ class LayoutSVGPath final : public LayoutSVGShape {
 
   void UpdateShapeFromElement() override;
 
+  const StylePath* GetStylePath() const;
   void UpdateMarkers();
 
   Vector<MarkerPosition> marker_positions_;

third_party/blink/renderer/core/layout/svg/svg_marker_data.cc

@@ -19,6 +19,9 @@
 
 #include "third_party/blink/renderer/core/layout/svg/svg_marker_data.h"
 
+#include "base/auto_reset.h"
+#include "third_party/blink/renderer/core/svg/svg_path_byte_stream_source.h"
+#include "third_party/blink/renderer/core/svg/svg_path_parser.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
 
 namespace blink {
@@ -32,12 +35,7 @@ static double BisectingAngle(double in_angle, double out_angle) {
 
 void SVGMarkerDataBuilder::Build(const Path& path) {
   path.Apply(this, SVGMarkerDataBuilder::UpdateFromPathElement);
-  if (positions_.IsEmpty())
-    return;
-  const bool kEndsSubpath = true;
-  UpdateAngle(kEndsSubpath);
-  // Mark the last marker as 'end'.
-  positions_.back().type = kEndMarker;
+  Flush();
 }
 
 void SVGMarkerDataBuilder::UpdateFromPathElement(void* info,
@@ -45,6 +43,109 @@ void SVGMarkerDataBuilder::UpdateFromPathElement(void* info,
   static_cast<SVGMarkerDataBuilder*>(info)->UpdateFromPathElement(*element);
 }
 
+namespace {
+
+// Path processor that converts an arc segment to a cubic segment with
+// equivalent start/end tangents.
+class MarkerPathSegmentProcessor : public SVGPathNormalizer {
+  STACK_ALLOCATED();
+
+ public:
+  MarkerPathSegmentProcessor(SVGPathConsumer* consumer)
+      : SVGPathNormalizer(consumer) {}
+
+  void EmitSegment(const PathSegmentData&);
+
+ private:
+  Vector<PathSegmentData> DecomposeArc(const PathSegmentData&);
+};
+
+Vector<PathSegmentData> MarkerPathSegmentProcessor::DecomposeArc(
+    const PathSegmentData& segment) {
+  class SegmentCollector : public SVGPathConsumer {
+    STACK_ALLOCATED();
+
+   public:
+    void EmitSegment(const PathSegmentData& segment) override {
+      DCHECK_EQ(segment.command, kPathSegCurveToCubicAbs);
+      segments_.push_back(segment);
+    }
+    Vector<PathSegmentData> ReturnSegments() { return std::move(segments_); }
+
+   private:
+    Vector<PathSegmentData> segments_;
+  } collector;
+  // Temporarily switch to our "collector" to collect the curve segments
+  // emitted by DecomposeArcToCubic(), and then switch back to the actual
+  // consumer.
+  base::AutoReset<SVGPathConsumer*> consumer_scope(&consumer_, &collector);
+  DecomposeArcToCubic(current_point_, segment);
+  return collector.ReturnSegments();
+}
+
+void MarkerPathSegmentProcessor::EmitSegment(
+    const PathSegmentData& original_segment) {
+  PathSegmentData segment = original_segment;
+  // Convert a relative arc to absolute.
+  if (segment.command == kPathSegArcRel) {
+    segment.command = kPathSegArcAbs;
+    segment.target_point += current_point_;
+  }
+  if (segment.command == kPathSegArcAbs) {
+    // Decompose and then pass/emit a synthesized cubic with matching tangents.
+    Vector<PathSegmentData> decomposed_arc_curves = DecomposeArc(segment);
+    if (decomposed_arc_curves.IsEmpty()) {
+      segment.command = kPathSegLineToAbs;
+    } else {
+      // Use the first control point from the first curve and the second and
+      // last control points from the last curve. (If the decomposition only
+      // has one curve then the second line just copies the same point again.)
+      segment = decomposed_arc_curves.back();
+      segment.point1 = decomposed_arc_curves[0].point1;
+    }
+  }
+  // Invoke the base class to normalize and emit to the consumer
+  // (SVGMarkerDataBuilder).
+  SVGPathNormalizer::EmitSegment(segment);
+}
+
+}  // namespace
+
+void SVGMarkerDataBuilder::Build(const SVGPathByteStream& stream) {
+  SVGPathByteStreamSource source(stream);
+  MarkerPathSegmentProcessor processor(this);
+  svg_path_parser::ParsePath(source, processor);
+  Flush();
+}
+
+void SVGMarkerDataBuilder::EmitSegment(const PathSegmentData& segment) {
+  PathElement element;
+  FloatPoint points[3];
+  element.points = points;
+  switch (segment.command) {
+    case kPathSegClosePath:
+      element.type = kPathElementCloseSubpath;
+      break;
+    case kPathSegMoveToAbs:
+      element.type = kPathElementMoveToPoint;
+      element.points[0] = segment.target_point;
+      break;
+    case kPathSegLineToAbs:
+      element.type = kPathElementAddLineToPoint;
+      element.points[0] = segment.target_point;
+      break;
+    case kPathSegCurveToCubicAbs:
+      element.type = kPathElementAddCurveToPoint;
+      element.points[0] = segment.point1;
+      element.points[1] = segment.point2;
+      element.points[2] = segment.target_point;
+      break;
+    default:
+      NOTREACHED();
+  }
+  UpdateFromPathElement(element);
+}
+
 double SVGMarkerDataBuilder::CurrentAngle(AngleType type) const {
   // For details of this calculation, see:
   // http://www.w3.org/TR/SVG/single-page.html#painting-MarkerElement
@@ -176,4 +277,13 @@ void SVGMarkerDataBuilder::UpdateFromPathElement(const PathElement& element) {
   positions_.push_back(MarkerPosition(marker_type, origin_, 0));
 }
 
+void SVGMarkerDataBuilder::Flush() {
+  if (positions_.IsEmpty())
+    return;
+  const bool kEndsSubpath = true;
+  UpdateAngle(kEndsSubpath);
+  // Mark the last marker as 'end'.
+  positions_.back().type = kEndMarker;
+}
+
 }  // namespace blink

third_party/blink/renderer/core/layout/svg/svg_marker_data.h

@@ -20,6 +20,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_SVG_SVG_MARKER_DATA_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_SVG_SVG_MARKER_DATA_H_
 
+#include "third_party/blink/renderer/core/svg/svg_path_consumer.h"
 #include "third_party/blink/renderer/platform/graphics/path.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/math_extras.h"
@@ -29,6 +30,7 @@ namespace blink {
 enum SVGMarkerType { kStartMarker, kMidMarker, kEndMarker };
 
 class LayoutSVGResourceMarker;
+class SVGPathByteStream;
 
 struct MarkerPosition {
   DISALLOW_NEW();
@@ -58,7 +60,7 @@ struct MarkerPosition {
   float angle;
 };
 
-class SVGMarkerDataBuilder {
+class SVGMarkerDataBuilder : private SVGPathConsumer {
   STACK_ALLOCATED();
 
  public:
@@ -67,9 +69,21 @@ class SVGMarkerDataBuilder {
         last_moveto_index_(0),
         last_element_type_(kPathElementMoveToPoint) {}
 
+  // Build marker data for a Path.
   void Build(const Path&);
 
+  // Build marker data for a SVGPathByteStream.
+  //
+  // A SVGPathByteStream is semantically higher-level than a Path, and thus
+  // this allows those higher-level constructs (for example arcs) to be handled
+  // correctly. This should be used in cases where the original path data can
+  // contain such higher-level constructs.
+  void Build(const SVGPathByteStream&);
+
  private:
+  // SVGPathConsumer
+  void EmitSegment(const PathSegmentData&) override;
+
   static void UpdateFromPathElement(void* info, const PathElement*);
 
   enum AngleType {
@@ -95,6 +109,7 @@ class SVGMarkerDataBuilder {
                                   const FloatPoint& end);
   SegmentData ExtractPathElementFeatures(const PathElement&) const;
   void UpdateFromPathElement(const PathElement&);
+  void Flush();
 
   Vector<MarkerPosition>& positions_;
   unsigned last_moveto_index_;

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

@@ -62,7 +62,7 @@ static inline bool IsAbsolutePathSegType(const SVGPathSegType type) {
 }
 
 struct PathSegmentData {
-  STACK_ALLOCATED();
+  DISALLOW_NEW();
 
  public:
   PathSegmentData()

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

@@ -60,7 +60,7 @@ class SVGPathNormalizer {
 
   void EmitSegment(const PathSegmentData&);
 
- private:
+ protected:
   bool DecomposeArcToCubic(const FloatPoint& current_point,
                            const PathSegmentData&);
 

third_party/blink/web_tests/TestExpectations

@@ -356,8 +356,6 @@ crbug.com/1018273 [ Mac ] paint/overflow/composited-scroll-vertical-rl.html [ Fa
 crbug.com/1025019 compositing/overflow/scroller-with-border-radius.html [ Crash ]
 crbug.com/1025019 virtual/prefer_compositing_to_lcd_text/compositing/overflow/scroller-with-border-radius.html [ Crash ]
 
-crbug.com/583097 external/wpt/svg/painting/marker-orient-001.svg [ Failure ]
-
 # ====== Paint team owned tests to here ======
 
 crbug.com/922249 virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html [ Failure Pass ]