Create SVG PathSegList lazily

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

13 years ago
Created attachment 219356 [details] [diff] [review]
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.
(Assignee)

Comment 2

13 years ago
Created attachment 219670 [details] [diff] [review]
varient with compact path storage

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

Comment 3

13 years ago
Created attachment 219895 [details] [diff] [review]
updated compact version
Attachment #219356 - Attachment is obsolete: true
Attachment #219670 - Attachment is obsolete: true
Attachment #219895 - Flags: review?(roc)
(Assignee)

Comment 4

13 years ago
Created attachment 219896 [details] [diff] [review]
diff -w version
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?
(Assignee)

Comment 6

13 years ago
> 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.
okay...

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

Comment 8

13 years ago
Created attachment 220182 [details] [diff] [review]
pathbuild removal only
Attachment #220182 - Flags: review?(roc)
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.
Attachment #220182 - Flags: superreview+
Attachment #220182 - Flags: review?(roc)
Attachment #220182 - Flags: review+
currently we have ellipse, line, path, circle, rect, polygon, and polyline frames...
(Assignee)

Comment 11

13 years ago
nsISVGPathBuilder removal portion checked in.
Comment on attachment 219895 [details] [diff] [review]
updated compact version

waiting for a new patch
Attachment #219895 - Flags: review?(roc) → review-
(Assignee)

Comment 13

13 years ago
Created attachment 220516 [details] [diff] [review]
path storage chunk - update to tip, parser subclasses
Attachment #219895 - Attachment is obsolete: true
Attachment #220516 - Flags: review?(roc)
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)
Attachment #220516 - Flags: superreview+
Attachment #220516 - Flags: review?(roc)
Attachment #220516 - Flags: review+
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.
(Assignee)

Comment 16

13 years ago
Checked in with the requested modifications from comment 14.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Depends on: 338281
Can someone confirm whether or not this patch caused bug 361643 to be fixed on trunk?
You need to log in before you can comment on or make changes to this bug.