Closed Bug 934183 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
We should convert SVG geometry bounds calculations from gfxContext to Moz2D.
Summary: Convert SVG geometry bounds calculations from gfxContext to Moz2D → Convert the bounds calculation code for SVG geometry to use Moz2D Path
No longer blocks: 935008
Blocks: 933019
Blocks: 987190
This doesn't block bug 933019 or bug 987190 anymore since I unblocked them by fixing bug 1034399.
No longer blocks: 933019, 987190
Attached patch patch (obsolete) — Splinter Review
Attachment #826373 - Attachment is obsolete: true
Attachment #8494508 - Flags: review?(bas)
(Path caching is a future patch/bug.)
s/poist list/points list/
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?
(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.
Attachment #8494508 - Attachment is obsolete: true
Attachment #8494508 - Flags: review?(bas)
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.
Attachment #8494556 - Flags: review?(bas) → review?(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 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.
Attachment #8494556 - Flags: review?(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 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.
Attachment #8497255 - Flags: review?(longsonr) → review+
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.
Attachment #8494556 - Attachment is obsolete: true
(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.
This code is also dead.
Attachment #8498269 - Flags: review?(longsonr)
Attachment #8498269 - Attachment is obsolete: true
Attachment #8498269 - Flags: review?(longsonr)
Attachment #8498271 - Flags: review?(longsonr)
Attachment #8498271 - Flags: review?(longsonr) → review+
Blocks: 1077119
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.
https://hg.mozilla.org/mozilla-central/rev/2f7b5e239d1e
https://hg.mozilla.org/mozilla-central/rev/7c26c6d5b2fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.