Make PaintSVG painting work by passing transforms down, rather than walking up the tree using GetCanvasTM

RESOLVED FIXED in mozilla35

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 824620 [details] [diff] [review]
WIP

Right now the PaintSVG painting code makes extensive use of nsSVGUtils::GetCanvasTM() in order to find the transform that should be used at any given point. The diverse ways in which we can paint SVG (SVG-as-image, font, doc (doc being split into more than one way soon)) combined with hacks (in the SVG text code in particular) over the years have meant that it's very difficult to know what's going on at any given GetCanvasTM() call site for all the various call scenarios. If we can manage to convert the painting over to a model where we pass transforms down as we paint it would make the SVG painting code much, much simpler to understand and reason about, and it would make the conversion to Moz2D a whole lot easier.

This is basically the painting equivalent of the GetBBox overhaul that happened in bug 488314 (which was a major simplification of the code win).
(Assignee)

Updated

4 years ago
Blocks: 1050142
(Assignee)

Updated

4 years ago
Attachment #824620 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1052999
(Assignee)

Comment 1

4 years ago
Created attachment 8482983 [details] [diff] [review]
Remove the "Skipping eUserSpaceToParent transforms makes no sense" asserts
Attachment #8482983 - Flags: review?(longsonr)
(Assignee)

Comment 2

4 years ago
Created attachment 8482986 [details] [diff] [review]
Add a nsLayoutUtils::PointToGfxPoint method

Similar to the RectToGfxRect method just above it, but for points.
Attachment #8482986 - Flags: review?(longsonr)
(Assignee)

Updated

4 years ago
Depends on: 1061920
Attachment #8482983 - Flags: review?(longsonr) → review+
Comment on attachment 8482986 [details] [diff] [review]
Add a nsLayoutUtils::PointToGfxPoint method

>   /**
>    * Convert an nsRect to a gfxRect.
>    */
>   static gfxRect RectToGfxRect(const nsRect& aRect,
>                                int32_t aAppUnitsPerDevPixel);
> 
>+  static gfxPoint PointToGfxPoint(const nsPoint& aPoint,
>+                                  int32_t aAppUnitsPerPixel) {
>+    return gfxPoint(gfxFloat(aPoint.x) / aAppUnitsPerPixel,
>+                    gfxFloat(aPoint.y) / aAppUnitsPerPixel);
>+  }
>+

I'd rather the variable was called aAppUnitsPerDevPixel like the RectToGfxRect case.

r=longsonr with that nit fixed.
Attachment #8482986 - Flags: review?(longsonr) → review+
(In reply to Robert Longson from comment #3)
> Comment on attachment 8482986 [details] [diff] [review]
> Add a nsLayoutUtils::PointToGfxPoint method
> 
> >   /**
> >    * Convert an nsRect to a gfxRect.
> >    */
> >   static gfxRect RectToGfxRect(const nsRect& aRect,
> >                                int32_t aAppUnitsPerDevPixel);
> > 
> >+  static gfxPoint PointToGfxPoint(const nsPoint& aPoint,
> >+                                  int32_t aAppUnitsPerPixel) {
> >+    return gfxPoint(gfxFloat(aPoint.x) / aAppUnitsPerPixel,
> >+                    gfxFloat(aPoint.y) / aAppUnitsPerPixel);
> >+  }
> >+
> 
> I'd rather the variable was called aAppUnitsPerDevPixel like the
> RectToGfxRect case.

I think you missed that.
(Assignee)

Updated

4 years ago
Blocks: 1062249
(Assignee)

Comment 6

4 years ago
Created attachment 8483530 [details] [diff] [review]
Make PaintSVG painting work by passing transforms down, rather than walking up the tree using GetCanvasTM
Attachment #8483530 - Flags: review?(longsonr)
(Assignee)

Comment 7

4 years ago
(In reply to :Ms2ger from comment #5)
> (In reply to Robert Longson from comment #3)
> > I'd rather the variable was called aAppUnitsPerDevPixel like the
> > RectToGfxRect case.
> 
> I think you missed that.

Oh, I did. That said, I've found it useful to use these functions to do conversions to CSS px (instead of device pixels) is the past though, so I think not specifying the type of px conversion factor in the name is better.
I think it would be simpler if nsSVGFilterInstance took a pointer, you could then pass nullptr in the non-painting cases (or better yet have a different constructor for non-painting cases rather than passing lots of nullptr fields). You could still have a gfxMatrix mPaintingTransform in nsSVGFilterInstance of course.
(Assignee)

Comment 10

4 years ago
I like the idea of separate ctors. Can we do that in a follow-up though? This patch is big enough as it is.
I have a patch in my queue that rectifies the lots-of-nullptrs-in-constructor situation by removing the arguments from the constructor and instead passing them to the method that's called afterwards and that actually needs it. The patch is on top of some canvas filter patches, so it doesn't apply at the moment, otherwise I'd try to land it asap.
Comment on attachment 8483530 [details] [diff] [review]
Make PaintSVG painting work by passing transforms down, rather than walking up the tree using GetCanvasTM

OK
Attachment #8483530 - Flags: review?(longsonr) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16e9746d9b6

I fixed a few other bugs that I found browsing random SVGs before pushing, and added a comment to nsISVGChildFrame::PaintSVG to describe how it fits into the way SVG paints.
Keywords: leave-open
(In reply to Jonathan Watt [:jwatt] from comment #13)
> I fixed a few other bugs

I don't see any tests being added :(
(Assignee)

Comment 15

4 years ago
Yeah, I should probably have done that but I didn't keep track of those SVGs since fixing this bug has taken longer than expected. :/
https://hg.mozilla.org/mozilla-central/rev/a16e9746d9b6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1127507
You need to log in before you can comment on or make changes to this bug.