334999 - Create SVG PathSegList lazily

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Currently paths are stored internally as a list of various PathSeg classes, which are walked every time we want to draw or otherwise handle a path.  A lighterweight storage system should speed things up a bit and ideally would reduce memory usage.

Comment 1

[tor]

Attachment: work in progress

First cut - stores paths as cairo_path_t.  Unfortunately this will actually increase our memory footprint as cairo stores coordinates as doubles instead of the floats we previously used.

We might want to do our own lightweight playback list for paths.

Comment 2

[tor]

Attachment: varient with compact path storage

Alternative version with our own playback list (2 bits/command + float args).

Comment 3

[tor]

Attachment: updated compact version

Comment 4

[tor]

Attachment: diff -w version

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could you separate out the patch that removes the pathbuilder and has pathgeometry frames poke cairo directly? nsSVGPathFrame::ConstructPath would need a little work but that's the only extra work, I think, and it would make things a lot simpler to review (and better CVS history too).

It's unfortunate that we now have two forms of path data storage. Couldn't we just have one compact form, and make the SVG DOM pathseg objects be wrappers around it?

Comment 6

[tor]

> It's unfortunate that we now have two forms of path data storage. Couldn't we
> just have one compact form, and make the SVG DOM pathseg objects be wrappers
> around it?

Not really, if we want to optimize for rendering rather than DOM manipulation (which is likely to be much rarer).  The SVG path description includes items that do not directly map to cairo, such as "smooth" curve segments that reference previous segments, elliptical arg segments, and quadratic curve segments.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

okay...

Then I think I'd like to have nsSVGPathDataParser have two distinct subclasses, one for cairo-rendering generation and one for DOM generation.

Comment 8

[tor]

Attachment: pathbuild removal only

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 220182 [details] [diff] [review]
pathbuild removal only

+  if (rx == 0.0f || ry == 0.0f) {
+    cairo_line_to(aCtx, x2, y2);
+  }

Shouldn't there be a "return" here?

+    const float magic = 4*(sqrt(2.)-1)/3;
+

Would you mind explaining this in a comment? I assume it's just a way to express a circular arc with cubic beziers.

    *
    * @param pathBuilder Object to write path description to.
    */
-  void constructPath(in nsISVGRendererPathBuilder pathBuilder);
+  void constructPath(in cairo_t aCtx);

Fix the comment.

I'd love to see a patch removing the pathgeometryframe subclasses with ConstructPath moved to content.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

currently we have ellipse, line, path, circle, rect, polygon, and polyline frames...

Comment 11

[tor]

nsISVGPathBuilder removal portion checked in.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 219895 [details] [diff] [review]
updated compact version

waiting for a new patch

Comment 13

[tor]

Attachment: path storage chunk - update to tip, parser subclasses

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 220516 [details] [diff] [review]
path storage chunk - update to tip, parser subclasses

+  nsresult rv = StoreClosePath();
+  NS_ENSURE_SUCCESS(rv, rv);
+
   return NS_OK;

return StoreClosePath();

+  rv = AppendSegment(seg);
+  return rv;

return AppendSegment(seg);
(lots of these)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsSVGPathDataParserToDOM could be shrunk a good deal. Make NS_NewSVGPathSeg* (except for Arc) all take the same signature: (nsIDOMSVGPathSeg**, float, float, float, float, float, float) (or better still just return the new segment directly, or null). Then you can have an AppendSegment(functionptr, float, float, float, float, float, float) method that creates the object and appends it, and most of your DOM Store* methods become just one call to that method. This can be done later.

Comment 16

[tor]

Checked in with the requested modifications from comment 14.

Comment 17

[Stephanie Daugherty [:sdaugherty]]

Can someone confirm whether or not this patch caused bug 361643 to be fixed on trunk?