934183 - Convert the bounds calculation code for SVG geometry to use Moz2D Path

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: WIP

We should convert SVG geometry bounds calculations from gfxContext to Moz2D.

Comment 1

[Jonathan Watt [:jwatt]]

This doesn't block bug 933019 or bug 987190 anymore since I unblocked them by fixing bug 1034399.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Jonathan Watt [:jwatt]]

(Path caching is a future patch/bug.)

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Add some guards to protect against the different behavior of Moz2D backends for empty Path objects

Comment 5

[Robert Longson [:longsonr]]

s/poist list/points list/

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8494508 [details] [diff] [review]
patch

Review of attachment 8494508 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/SVGLineElement.cpp
@@ +127,5 @@
>    GetAnimatedLengthValues(&x1, &y1, &x2, &y2, nullptr);
>  
>    pathBuilder->MoveTo(Point(x1, y1));
>    pathBuilder->LineTo(Point(x2, y2));
> +  pathBuilder->Close();

This doesn't seem right.. closing this would mean the line gets stroked once in each direction. Was this needed for some reason?

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Bas Schouten (:bas.schouten) from comment #6)
> This doesn't seem right.. closing this would mean the line gets stroked once
> in each direction. Was this needed for some reason?

Yeah, sorry, that was me doing experiments for zero length paths and forgetting to remove that.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8494556 [details] [diff] [review]
Add some guards to protect against the different behavior of Moz2D backends for empty Path objects

I suppose this is better for an SVG peer, since Bas doesn't know this code so well.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: convert the bounds calculation code for SVG geometry to use Moz2D Path

Comment 10

[Robert Longson [:longsonr]]

What happens if you have a line with zero length or an arc with 0 radius or other degenerate cases? Are all backends OK with those? Can we get tests for such things?

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8494556 [details] [diff] [review]
Add some guards to protect against the different behavior of Moz2D backends for empty Path objects

Cancelling review on the guard part while I think some things through some more.

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 8497255 [details] [diff] [review]
convert the bounds calculation code for SVG geometry to use Moz2D Path

>-    bbox.UnionEdges(nsSVGUtils::PathExtentsToMaxStrokeExtents(pathExtents,
>-                                                              this,
>-                                                              ThebesMatrix(aToBBoxUserspace)));
>   }
> 

Is this nsSVGUtils::PathExtentsToMaxStrokeExtents signature dead code after this patch?

Comment 13

[Robert Longson [:longsonr]]

Comment on attachment 8497255 [details] [diff] [review]
convert the bounds calculation code for SVG geometry to use Moz2D Path

>-    bbox.UnionEdges(nsSVGUtils::PathExtentsToMaxStrokeExtents(pathExtents,
>-                                                              this,
>-                                                              ThebesMatrix(aToBBoxUserspace)));

r=longsonr if you check whether nsSVGUtils::PathExtentsToMaxStrokeExtents with this signature is now dead and remove that signature if it is.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8494556 [details] [diff] [review]
Add some guards to protect against the different behavior of Moz2D backends for empty Path objects

I ended up fixing the issues this patch was intended to address over in bug 1075399.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #13)
> check whether nsSVGUtils::PathExtentsToMaxStrokeExtents
> with this signature is now dead and remove that signature if it is.

It is, and I'll remove it.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: remove lots of dead Thebes code paths

This code is also dead.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: remove lots of dead Thebes code paths

Comment 18

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7b5e239d1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c26c6d5b2fb

I ended up leaving the code using PathExtentsToMaxStrokeExtents for now with the Path::GetStrokedBounds() code ifdef'ed out. The comment in the code explains why:

    // This disabled code is how we would calculate the stroke bounds using
    // Moz2D Path::GetStrokedBounds(). Unfortunately at the time of writing it
    // there are two problems that prevent us from using it.
    //
    // First, it seems that some of the Moz2D backends are really dumb. Not
    // only do some GetStrokeOptions() implementations sometimes significantly
    // overestimate the stroke bounds, but if an argument is passed for the
    // aTransform parameter then they just return bounds-of-transformed-bounds.
    // These two things combined can lead the bounds to be unacceptably
    // oversized, leading to massive over-invalidation.
    //
    // Second, the way we account for non-scaling-stroke by transforming the
    // path using the transform to the outer-<svg> element is not compatible
    // with the way that nsSVGPathGeometryFrame::Reflow() inserts a scale into
    // aToBBoxUserspace and then scales the bounds that we return.

We're at least at the point where we aren't using the gfxContext any more.

I'm going to do some Try pushes shortly to see if we can now get rid of the scale nsSVGPathGeometryFrame::Reflow inserts into aToBBoxUserspace. I'm also investigating the GetStrokeOptions() issues, and changing PathExtentsToMaxStrokeExtents so that it doesn't use mStrokeMiterlimit for shapes that can't be affected by stroke-miterlimit so that we get better bounds for them.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f7b5e239d1e
https://hg.mozilla.org/mozilla-central/rev/7c26c6d5b2fb