377085 - line svg element getBBox() returns bad rect

Status: RESOLVED FIXED

Description

[daniel newton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070410 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070410 Minefield/3.0a4pre

It just started happening in this nightly

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[daniel newton]

Created attachment 261198 [details]
testcase

Comment 2

[Robert Longson [:longsonr]]

Basically SVG wants to get the bounding box of a line without taking its stroke-width into account.

nsSVGPathGeometryFrame::GetBBox calls context.SetLineWidth(0) for a line with no fill. If the line width is zero then _cairo_gstate_stroke_extents returns 0 as the extent. 

This worked when bug 367503 was written (that changed things to set the line width to 0 rather than 0.0001). The upgrade to cairo 1.4.2 (re?)introduced the line_width check and broke it.

The cairo change was http://gitweb.freedesktop.org/?p=cairo.git;a=commit;h=133183d858aa632da3cec2a789dcc1e1203d778b

Comment 3

[Robert Longson [:longsonr]]

Created attachment 261253 [details] [diff] [review]
possible fix for cairo

Comment 4

[Carl Worth]

Here's a thought:

Instead of going through the tremendous effort to tessellate to trapezoids, (and worry about caps and joins, etc.), shouldn't the implementation of cairo_stroke_extents for a line width of 0.0 just iterate over the path itself and find the extents?

And look, isn't that exactly what the existing _cairo_path_fixed_bounds function does? (Oddly enough, this function appears to be unused currently.)

Would someone like to give that a quick try?

-Carl

Comment 5

[tor]

(In reply to comment #4)
> Instead of going through the tremendous effort to tessellate to trapezoids,
> (and worry about caps and joins, etc.), shouldn't the implementation of
> cairo_stroke_extents for a line width of 0.0 just iterate over the path itself
> and find the extents?

The problematic bit is the bezier/cubic curves.  While a loose bound could be calculated easily by checking the points of the control polygon, the SVG specification requires a tight bounding box (http://www.w3.org/TR/SVG11/types.html#InterfaceSVGLocatable).

Comment 6

[tor]

Further thought - we could avoid that problem by flattening the path first.  Takes a bit of memory, but cairo would probably be doing the same thing internally.

Comment 7

[Carl Worth]

(In reply to comment #5)
> The problematic bit is the bezier/cubic curves.  While a loose bound could be
> calculated easily by checking the points of the control polygon, the SVG
> specification requires a tight bounding box
> (http://www.w3.org/TR/SVG11/types.html#InterfaceSVGLocatable).

Right, but it's simple enough to flatten the curves while computing the bounds.
I'm actually a bit surprised that _cairo_path_fixed_bounds isn't doing that already---it's obviously wrong the way it's written now.

Oh, and a nice tweak would be to make a _cairo_path_fixed_interpret_flat variant of _cairo_path_fixed_interpret to just do the flattening on behalf of the caller. I think as is we've got the flattening implemented more than once in cairo already.

-Carl

Comment 8

[Carl Worth]

(In reply to comment #6)
> Further thought - we could avoid that problem by flattening the path first. 
> Takes a bit of memory, but cairo would probably be doing the same thing
> internally.

Cairo can flatten while iterating over the path, so won't require an entire copy of the path. So that might make it a nice place to do this.

-Carl

Comment 10

[Robert Longson [:longsonr]]

I think blocking is unlikely to be accepted without someone stepping up to provide a patch.

Comment 11

[daniel newton]

This is a regression which causes previously working code to fail (hoping that ups the ante? :)

Comment 13

[Jonathan Watt [:jwatt] (needinfo? me)]

CC'ing Brian in the hope that he might have time to help out on this blocker.

Comment 14

[Brian Ewins]

(In reply to comment #13)
> CC'ing Brian in the hope that he might have time to help out on this blocker.
> 

I've had a look and I can see what Carl's getting at, as he says, all the pieces are already there. I have a bit of time tonight and tomorrow morning but the Scotland v Italy game will probably blot out the rest of the weekend :) . Anyway I'll see if I can get this done this evening.

Comment 15

[Brian Ewins]

Here's what I have so far:
http://gitweb.freedesktop.org/?p=users/brianewins/cairo;a=shortlog;h=path-extents

the top two commits are relative to current HEAD, only a couple of commits out of synch with where you are right now.

This adds cairo_path_extents() to the public api, which gets the extents of the path as opposed to cairo_fill_extents() which gets the inked extents. The patches also add unit tests, and it looks ok, but YMMV.

The big disclaimer though: this api is up for discussion right now on the cairo mailing list, it could get veto'd, might not appear in the next version, etc etc. You're just looking at my private changes here.

Let me know if this helps.

Comment 16

[Jonathan Watt [:jwatt] (needinfo? me)]

Created attachment 290147 [details] [diff] [review]
Brian's cairo changes applied to Mozilla

Seems to work for me Brian. Here's your patch with the additional changes required to use it in Mozilla.

Comment 17

[Jonathan Watt [:jwatt] (needinfo? me)]

I've been playing with various attachments to old bugs that should hit this code path. E.g. attachment 222885 [details].

Comment 18

[Brian Ewins]

(In reply to comment #16)
> Created an attachment (id=290147) [details]
> Brian's cairo changes applied to Mozilla
> 
> Seems to work for me Brian. Here's your patch with the additional changes
> required to use it in Mozilla.
> 

You can drop these lines from nsSVGPathGeometryFrame::GetBBox too:
   if (IsDegeneratePath(extent)) {
     context.SetLineWidth(0);
     extent = context.GetUserStrokeExtent();
   }
... they'll never do anything now (the extents returned here are always degenerate)

Comment 20

[Robert Longson [:longsonr]]

This patch does not fix the testcase in bug 381745 or bug 313180.

I think bug 381745 needs to be dealt with by the SVG code. The bounding box is calculated for the gradient which has a zero height for the line and then the gradient fails. So I think we should reopen bug 381745.

Bug 313180 is caused by us asking for a bounding box when nothing is drawn. The cairo code then returns success and random numbers. I'm not sure that is entirely reasonable.

Comment 21

[Robert Longson [:longsonr]]

bug 381745 is invalid (explanation in the bug) so there is just the testcase from bug 313180 to think about.

Comment 22

[Brian Ewins]

Carl's pushed a cleaned-up version of the patch above into cairo:
http://gitweb.freedesktop.org/?p=cairo;a=log;h=326342962daa694d876c03194e8a6c1b13f9a8d2

this will be in cairo 1.6. The semantics of the new cairo_path_extents call are slightly changed - spurious move_to's were contributing to path_extents in the above patch; now they don't. Pasting the docs for completeness:

 * Computes a bounding box in user-space coordinates covering the
 * points on the current path. If the current path is empty, returns
 * an empty rectangle ((0,0), (0,0)).  Stroke parameters, surface
 * dimensions and clipping are not taken into account.
 *
 * Contrast with cairo_fill_extents() and cairo_stroke_extents() which
 * return the extents of only the area that would be "inked" by
 * the corresponding drawing operations.
 *
 * The result of cairo_path_extents() is defined as equivalent to the
 * limit of cairo_stroke_extents() with CAIRO_LINE_CAP_ROUND as the
 * line width approaches 0.0, (but never reaching the empty-rectangle
 * returned by cairo_stroke_extents() for a line width of 0.0).
 *
 * Specifically, this means that zero-area sub-paths such as
 * cairo_move_to();cairo_line_to() segments, (even degenerate cases
 * where the coordinates to both calls are identical), will be
 * considered as contributing to the extents. However, a lone
 * cairo_move_to() will not contribute to the results of
 * cairo_path_extents().

so, usual story, this will need wired in when gecko synchs up with 1.6.

Comment 23

[Robert Longson [:longsonr]]

What does the cleaned up patch return for the bounds of an empty surface?

Comment 24

[Robert Longson [:longsonr]]

(In reply to comment #23)
> What does the cleaned up patch return for the bounds of an empty surface?
> 
Don't mind me. Should have read more carefully. The answer is in the comments.

Comment 25

[Robert Longson [:longsonr]]

Created attachment 299406 [details] [diff] [review]
updated patch

This is what's left after the cairo update with Brian's work landed. This incorporates Brian's IsDegenerate not required comment.

Comment 26

[Robert Longson [:longsonr]]

checked in.