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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: 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).

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Remove the "Skipping eUserSpaceToParent transforms makes no sense" asserts

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Add a nsLayoutUtils::PointToGfxPoint method

Similar to the RectToGfxRect method just above it, but for points.

Comment 3

[Robert Longson [:longsonr]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4d8dcdb876
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b71fdd51f0

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 6

[Jonathan Watt [:jwatt]]

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

Comment 7

[Jonathan Watt [:jwatt]]

(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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d4d8dcdb876
https://hg.mozilla.org/mozilla-central/rev/a5b71fdd51f0

Comment 9

[Robert Longson [:longsonr]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

I like the idea of separate ctors. Can we do that in a follow-up though? This patch is big enough as it is.

Comment 11

[Markus Stange [:mstange]]

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 12

[Robert Longson [:longsonr]]

Comment on attachment 8483530 [details] [diff] [review]
Make PaintSVG painting work by passing transforms down, rather than walking up the tree using GetCanvasTM

OK

Comment 13

[Jonathan Watt [:jwatt]]

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.

Comment 14

[Markus Stange [:mstange]]

(In reply to Jonathan Watt [:jwatt] from comment #13)
> I fixed a few other bugs

I don't see any tests being added :(

Comment 15

[Jonathan Watt [:jwatt]]

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. :/

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a16e9746d9b6