1074161 - Avoid creating a Moz2D Path object to draw SVG <rect>, <image> and <line> elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Now that nsSVGPathGeometryFrame is using Moz2D Path objects (returned by calling BuildPath() on the SVGElement subclasses) we are no longer benefiting from the gfxContext::mPathIsRect optimization where we take the DrawTarget::FillRect/StrokeRect fast path. Creating Path objects is relatively expensive, so we should figure out how to get back to a place where we are calling those faster DrawTarget methods when possible.

Bas, could Moz2D's Path object hold some sort of "this is a rect" state?

I know IE has some benchmarks where they test painting tons of rects where we got hammered without the mPathIsRect optimization, so we do want to do this. I believe we had a Talos regression when we fell off that fast path before too.

Comment 1

[Jonathan Watt [:jwatt]]

For example, we could create a PathRect subclass of Path, adding a new BackendType (RAW_RECT? NONE_RAW_RECT?) and having CopyToBuilder() return nullptr.

Implementing StrokeContainsPoint for stroke with dashing would be annoying if we feel we should implement that.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Bas didn't like the idea of changing Moz2D for this. We have various perf regressions that we need to fix, so let's do something. Not loving this, but it does the trick.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8508929 [details] [diff] [review]
patch

Review of attachment 8508929 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of optional suggestions:

Include the RefPtr<Path> in the SimplePath class, and combine BuildPath/GetAsSimplePath. That would require changing all the callers of BuildPath though, maybe not helpful.

Add Fill/Stroke methods to SimplePath and have that choose which DT method to call so the users of SimplePath don't need to have if statements every time.

::: content/svg/content/src/nsSVGPathGeometryElement.h
@@ +98,5 @@
> +   * a Moz2D Path object. For Rects and lines it is better to get the path data
> +   * using this method and then use the optimized DrawTarget methods for
> +   * filling/stroking rects and lines.
> +   */
> +  virtual void GetAsSimplePath(SimplePath* aSimplePath) {

Why not just return the SimplePath object by-value? It's small, and the compiler should optimize it just fine.

return SimplePath(NONE); seems nicer.

::: layout/svg/nsSVGPathGeometryFrame.cpp
@@ +729,5 @@
>      if (fillPattern.GetPattern()) {
> +      DrawOptions drawOptions(1.0f, CompositionOp::OP_OVER, aaMode);
> +      if (simplePath.mType == nsSVGPathGeometryElement::SimplePath::RECT) {
> +        drawTarget->FillRect(simplePath.mRect, fillPattern, drawOptions);
> +      } else if (path) {

Assert that we never get SimplePath::LINE here

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8508929 [details] [diff] [review]
patch

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Assert that we never get SimplePath::LINE here

Actually that's perfectly legitimate. We simply have nothing to fill, so we don't want to do anything for that case.

As for the other suggestions I wasn't really happy with anything that we've come up with. I'll think on it some more, but for now I've landed mostly unchanged.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71cefcb137d7

Comment 5

[Jonathan Watt [:jwatt]]

It seems this very slightly changes parts of the edges of the stroked rects in non-scaling-stroke-01.svg's _reference_ file on Windows 7:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/non-scaling-stroke-01-ref.svg

This is because the reference is now drawn using DrawTarget::StrokeRect, while the test is still drawn using DrawTarget::Stroke(path,...) (because the test has non-scaling-stroke, preventing DrawTarget::StrokeRect from being used). Seems these two DrawTarget functions don't quite produce the same rendering even when the Path contains a rect with the same float values.

I added some fuzz as a follow-up patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dedc769ea8a8

Comment 6

[Jonathan Watt [:jwatt]]

OS X 10.8 also needed fuzzing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0891ada9cf

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71cefcb137d7
https://hg.mozilla.org/mozilla-central/rev/dedc769ea8a8
https://hg.mozilla.org/mozilla-central/rev/7b0891ada9cf