932762 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing, clipPath clipping, bbox calculations and animation/text along a path

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Moz2D paths are in user space. This means that, after converting SVG to Moz2D, we should be able to usefully cache the Moz2D path data on SVG elements and use it to speed up all of rendering, hit-testing and bbox calculations. I have a WIP patch locally for this.

Note to self - we should have a pref for this to make it easy to measure speed/memory tradeoffs.

Comment 1

[Jonathan Watt [:jwatt]]

One issue to consider here is that the GetPathForLengthOrPositionMeasuring method that I added currently means that we can in principal use paths containing different data for path and position measuring vs. painting and bounds calculations. That presents an issue for caching. My concern when introducing GetPathForLengthOrPositionMeasuring was that the little lines that SVGPathData::BuildPath inserts into the path data (for painting of zero length subpaths) would impact negatively on measuring for things like animation along a path. Now that I think about it though, it seems that that is very unlikely. Besides the fact that it seems unlikely that authors would be putting zero length subpath data into paths that they want to animate along, there's the fact that measuring requires flattening the path, and that introduces all sorts of rounding error anyway. Hence the 'pathLength' attribute that the spec added.

My proposal is to keep the GetPathForLengthOrPositionMeasuring functions (with a bit of renaming) for the sake of semantics, but use the same underlying path for everything so that we can share the same cached path when we get bounds for reflow, paint, get bbox bounds for paint, hit-test and measure. If at some point we find this is causing issues for content then we can do something like tag the path with information that it contains zero length subpaths, and then have GetPathForLengthOrPositionMeasuring skip the cache in that case alone (so that only such cases pay the cost).

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations

A few notes:

The caching is put behind a bool pref called svg.path-caching.enabled

DOMSVGPathSeg no longer caches the measuring path it returns since nsSVGPathGeometryElement is now doing caching of the path for all uses. None of the consumers of the measuring path actually wanted caching other than nsSVGPathGeometryElement.

Invalidation of the cached path is done mainly via nsSVGPathGeometryElement::AfterSetAttr and the applicable nsSVGElement DidAnimate*() methods.

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 8499781 [details] [diff] [review]
part 1 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional

It's OK as is, but I think having the method take a reference rather than a pointer would be better as that would prevent anybody trying to pass nullptr.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 8499784 [details] [diff] [review]
part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations

> 
>+  virtual nsSVGPathGeometryElement* AsPathGeometryElement() { return nullptr; }

This seems a silly dance, just make InvalidateAnyCachedPath virtual and
call that directly every time. I'm not really sold on the word Invalidate either
as it makes me thing it's going to do something with the refresh driver. ClearCachedData perhaps?

Comment 6

[Jonathan Watt [:jwatt]]

ClearAnyCachedPath, perhaps?

Regarding making in virtual, you mean on nsSVGElement? I guess so. I'm not really all that keen on that idea though. We have far too many functions moved up to base classes where they do nothing and have no business being. I'd rather not add another one. That said it also sucks to be putting nsSVGPathGeometryElement knowledge in nsSVGElement.cpp. :/

Comment 7

[Robert Longson [:longsonr]]

Might we not want later to extend this to polylines and polygons? I think to aid that the pref should be an integer 0 = no caching 1 = cache paths later bugs could then have 2=cache polygons and polylines.

So maybe it's best not to have Path in the method name. 

AsPathGeometryElement is on the nsSVGSVGElement in your implementation so we're not adding anything over and above your way of doing it.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 8499784 [details] [diff] [review]
part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations

I think we're making it unnecessarily hard to extend caching to polygons/polylines. Not that we should in this patch, just that we should make it easy for a future patch to do so.

Comment 9

[Jonathan Watt [:jwatt]]

What do you mean? nsSVGPathGeometryFrame is a base class for all our geometry elements, including polygon and polyline. So this does implement path caching for them.

Comment 10

[Jonathan Watt [:jwatt]]

Note that the "Path" in the name is in reference to the Moz2D path that is created to draw the element. It's nothing to do with the element being a <path>.

Comment 11

[Robert Longson [:longsonr]]

OK. Forget comment 7 then.

Just fix the virtual function and call it ClearAnyCachedPath then.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> It's OK as is, but I think having the method take a reference rather than a
> pointer would be better as that would prevent anybody trying to pass nullptr.

I did this BTW.

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 8499880 [details] [diff] [review]
part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations

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

If the fill rule changes don't we need to clear the cache? I don't see where that happens.

::: layout/svg/nsSVGClipPathFrame.cpp
@@ +74,2 @@
>          }
>        }

builder is no longer used. That line should be removed.

Comment 15

[Robert Longson [:longsonr]]

please check we have a dynamic reftest that changes the fill rule of something and add one if we haven't

r=longsonr with the reftest addressed.

Comment 16

[Robert Longson [:longsonr]]

And the builder comment dead code removal is about this bit...

           RefPtr<PathBuilder> builder =
             gfx->GetDrawTarget()->CreatePathBuilder(
               nsSVGUtils::ToFillRule(pathFrame->StyleSVG()->mClipRule));
-          clipPath = pathElement->BuildPath(builder);
+          clipPath = pathElement->GetOrBuildPath(gfx->GetDrawTarget(),
+                                                 nsSVGUtils::ToFillRule(pathFrame->StyleSVG()->mClipRule));


didn't really come out of splinter very well.

Comment 17

[Jonathan Watt [:jwatt]]

Ah, good catch on the fill-rule thing in particular!

Comment 18

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41bad654774b
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c93e9a7c97

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/41bad654774b
https://hg.mozilla.org/mozilla-central/rev/75c93e9a7c97