Closed Bug 1074161 Opened 8 years ago Closed 8 years ago

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


(Core :: SVG, defect)

Not set





(Reporter: jwatt, Assigned: jwatt)


(Blocks 1 open bug)


(Keywords: perf)


(1 file)

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.
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.
Summary: Avoid creating a Moz2D Path object for SVG <rect> and <image> → Avoid creating a Moz2D Path object to draw SVG <rect>, <image> and <line> elements
Attached patch patchSplinter Review
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.
Attachment #8508929 - Flags: review?(matt.woodrow)
Comment on attachment 8508929 [details] [diff] [review]

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
Attachment #8508929 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8508929 [details] [diff] [review]

(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.
Attachment #8508929 - Flags: checkin+
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:

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:
Blocks: 1034958
Depends on: 1122578
You need to log in before you can comment on or make changes to this bug.