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

RESOLVED FIXED in mozilla35

Status

()

Core
SVG
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on: 1 bug, {perf})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 923193
(Assignee)

Comment 1

3 years ago
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).
(Assignee)

Comment 2

3 years ago
Created attachment 8499781 [details] [diff] [review]
part 1 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional
Attachment #8499781 - Flags: review?(longsonr)
(Assignee)

Updated

3 years ago
Attachment #8499781 - Attachment description: part 2 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional → part 1 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional
(Assignee)

Comment 3

3 years ago
Created attachment 8499784 [details] [diff] [review]
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.
Attachment #8499784 - Flags: review?(longsonr)

Comment 4

3 years ago
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.
Attachment #8499781 - Flags: review?(longsonr) → review+

Comment 5

3 years ago
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?
(Assignee)

Comment 6

3 years ago
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

3 years ago
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

3 years ago
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.
Attachment #8499784 - Flags: review?(longsonr) → review-
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
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

3 years ago
OK. Forget comment 7 then.

Just fix the virtual function and call it ClearAnyCachedPath then.
(Assignee)

Comment 12

3 years ago
Created attachment 8499880 [details] [diff] [review]
part 2 - Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations
Attachment #8499784 - Attachment is obsolete: true
Attachment #8499880 - Flags: review?(longsonr)
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Updated

3 years ago
Summary: Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing and bbox calculations → Make SVG elements cache their Moz2D path data to speed up rendering, hit-testing, clipPath clipping, bbox calculations and animation/text along a path

Comment 14

3 years ago
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.
Attachment #8499880 - Flags: review?(longsonr) → review+

Comment 15

3 years ago
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

3 years ago
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.
(Assignee)

Comment 17

3 years ago
Ah, good catch on the fill-rule thing in particular!
(Assignee)

Comment 18

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

Updated

3 years ago
Depends on: 1077993
https://hg.mozilla.org/mozilla-central/rev/41bad654774b
https://hg.mozilla.org/mozilla-central/rev/75c93e9a7c97
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.