Bug 54549 - cairo_close_path should not change extents on a "empty" path
cairo_close_path should not change extents on a "empty" path
Status: RESOLVED FIXED
Product: cairo
Classification: Unclassified
Component: general
1.12.2
Other All
: medium normal
Assigned To: Carl Worth
cairo-bugs mailing list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-05 11:42 UTC by Jussi Kukkonen
Modified: 2012-09-05 16:04 UTC (History)
1 user (show)

See Also:


Attachments
path-fixed: do not let temporary lines affect path extents (1.49 KB, patch)
2012-09-05 13:19 UTC, Jussi Kukkonen
Details | Splinter Review
path-fixed: do not let temporary lines affect path extents (1.62 KB, patch)
2012-09-05 13:22 UTC, Jussi Kukkonen
Details | Splinter Review
path-fixed: do not let temporary lines affect path extents (1.65 KB, patch)
2012-09-05 13:38 UTC, Jussi Kukkonen
Details | Splinter Review
Test case for degenerate closed path (1.27 KB, patch)
2012-09-05 16:00 UTC, Andrea Canciani
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jussi Kukkonen 2012-09-05 11:42:32 UTC
This code:
	cairo_move_to (cr, -1, 1);
	cairo_path_extents (cr, &x1, &y1, &x2, &y2);
	printf ("extents before close_path: %f %f %f %f\n", x1,y1,x2,y2);
	cairo_close_path (cr);
	cairo_path_extents (cr, &x1, &y1, &x2, &y2);
	printf ("extents after close_path: %f %f %f %f\n", x1,y1,x2,y2);
will output
	extents before close_path: 0.000000 0.000000 0.000000 0.000000
	extents after close_path: -1.000000 0.000000 0.000000 1.000000

That seems like a bug to me: Shouldn't the boundingbox still be zero sized after cairo_close_path().

This started happening at commit 166453c1ab.
Comment 1 Jussi Kukkonen 2012-09-05 13:19:34 UTC
Created attachment 66672 [details] [review]
path-fixed: do not let temporary lines affect path extents

This seems to do the trick.
Comment 2 Jussi Kukkonen 2012-09-05 13:22:59 UTC
Created attachment 66673 [details] [review]
path-fixed: do not let temporary lines affect path extents

Oops, fix style to match rest of file
Comment 3 Jussi Kukkonen 2012-09-05 13:33:11 UTC
Found one more way to break this:
  data="M 1 1 Z M 1 2 Z"
The Z in the middle screws things up.
Comment 4 Chris Wilson 2012-09-05 13:36:34 UTC
Not quite. The issue is that the zero-extents are wrong. The path exists but is degenerate (if you stroke it, it should generate a point for example).

So I think the fix is:

diff --git a/src/cairo-path-fixed.c b/src/cairo-path-fixed.c
index 459c680..c7b1cab 100644
--- a/src/cairo-path-fixed.c
+++ b/src/cairo-path-fixed.c
@@ -419,6 +419,7 @@ _cairo_path_fixed_move_to (cairo_path_fixed_t  *path,
     path->has_current_point = TRUE;
     path->current_point.x = x;
     path->current_point.y = y;
+    path->last_move_point = path->current_point;
 
     return CAIRO_STATUS_SUCCESS;
 }
Comment 5 Jussi Kukkonen 2012-09-05 13:38:55 UTC
Created attachment 66676 [details] [review]
path-fixed: do not let temporary lines affect path extents

...and this one works for all the combinations of M and Z I could think of.
Comment 6 Jussi Kukkonen 2012-09-05 13:50:31 UTC
> Not quite. The issue is that the zero-extents are wrong. The path exists but is
> degenerate (if you stroke it, it should generate a point for example).

Yeah, that looks more correct -- the zero sized extent is now in the correct location. 

I confirm your patch passes the webkit tests I was looking at as well as the ones I cooked up. Thanks.
Comment 7 Chris Wilson 2012-09-05 13:57:35 UTC
commit b0c466e27afcec230b2c9436eeb924c05123a544
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Sep 5 14:55:55 2012 +0100

    path: Update last_move_point after move-to
    
    Reported-and-tested-by: Jussi Kukkonen <jku@linux.intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54549
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I should also write a test-case. :|
Comment 8 Andrea Canciani 2012-09-05 16:00:19 UTC
Created attachment 66684 [details] [review]
Test case for degenerate closed path

I can confirm that the patch from Chris is the right one (yes, in Cairo semantics, a degenerate path can have non-zero path extents).

Here is the testcase.
Comment 9 Behdad Esfahbod 2012-09-05 16:04:14 UTC
You should differentiate between stroke extents and clip/fill extents.