Bug 34560 - path operator fixup incorrectly applied to stroked paths in pdf/ps backends
Summary: path operator fixup incorrectly applied to stroked paths in pdf/ps backends
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: pdf backend (show other bugs)
Version: 1.10.0
Hardware: All All
: medium normal
Assignee: Adrian Johnson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-22 03:53 UTC by kellner
Modified: 2011-03-18 03:24 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
test program (617 bytes, application/octet-stream)
2011-02-22 03:53 UTC, kellner
Details

Description kellner 2011-02-22 03:53:37 UTC
Created attachment 43648 [details]
test program

The function _cairo_path_fixed_is_box() in cairo-path-fixed.c is 
used in some backends (pdf and ps, maybe more) during a stroke
operation.

The operator fixup seems to be intended to complete a path prior 
to filling.  When this fixup is also applied to stroke operations,
it may cause incorrect output.

The test program tries to draw 3 lines.  The pdf and ps backends 
in cairo 1.10.0 (and master branch) draw a rectangle instead, while
the svg output is Ok.
Comment 1 Andrea Canciani 2011-03-08 01:33:44 UTC
I wrote a test based on your and an hack to fix it here:
http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/newtests

The problem is not _cairo_path_fixed_is_box(), but _cairo_path_fixed_is_rectangle() in cairo-pdf-operators.c

The correct fix would probably be to implement a _cairo_path_fixed_iter_is_stroke_box() based on
_cairo_path_fixed_iter_is_fill_box() and use them to emit rectangles when targetting ps/pdf, but I'm not sure if it is worth the additional code.

I believe we can just emit explicit move_to/line_to commands to describe rectangles in ps/pdf and get rid of _cairo_path_fixed_is_rectangle().
Comment 2 Andrea Canciani 2011-03-08 01:36:28 UTC
Uh, as usual I was almost forgetting...
What is your name?
The test has been rewritten (to make it smaller and avoid transparent background), but you found the bug and you should be credited!
Comment 3 Adrian Johnson 2011-03-08 02:30:56 UTC
(In reply to comment #1)
> I believe we can just emit explicit move_to/line_to commands to describe
> rectangles in ps/pdf and get rid of _cairo_path_fixed_is_rectangle().

It is better to have a few extra lines of code to keep this optimization. It makes a big difference to the PDF file size when there is a very large number of rectangles. eg the test cases in bug 13433.
Comment 4 Andrea Canciani 2011-03-08 02:39:01 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > I believe we can just emit explicit move_to/line_to commands to describe
> > rectangles in ps/pdf and get rid of _cairo_path_fixed_is_rectangle().
> 
> It is better to have a few extra lines of code to keep this optimization. It
> makes a big difference to the PDF file size when there is a very large number
> of rectangles. eg the test cases in bug 13433.

Currently we only optimize paths composed of a single rectangle. Is this enough
or should we also optimize paths composed of multiple rectangles and/or
rectangles and other path elements?

If we want this optimization in the most generic form (rectangles in a generic ops sequence), we might want to have a fill-path-interpreter and a stroke-path-interpreter which optionally support additional operations, like rectangles.

If we just want to optimize multiple-rectangles paths, we probably want _cairo_path_fixed_iter_is_stroke_box()

If we just want to optimize paths with a single rectangle, we can fix _cairo_path_fixed_is_rectangle().
Comment 5 Adrian Johnson 2011-03-08 02:47:42 UTC
(In reply to comment #4)
> Currently we only optimize paths composed of a single rectangle. Is this enough
> or should we also optimize paths composed of multiple rectangles and/or
> rectangles and other path elements?

Optimizing single rectangles is probably enough as it is the most common case. The other cases are nice to have but I don't think it is worth spending a lot of time or line of code on them.
Comment 6 Andrea Canciani 2011-03-08 03:20:50 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Currently we only optimize paths composed of a single rectangle. Is this enough
> > or should we also optimize paths composed of multiple rectangles and/or
> > rectangles and other path elements?
> 
> Optimizing single rectangles is probably enough as it is the most common case.
> The other cases are nice to have but I don't think it is worth spending a lot
> of time or line of code on them.

I pushed a commit that fixes the single-rectangle optimization:
http://cgit.freedesktop.org/~ranma42/cairo/commit/?h=wip/newtests&id=2e6fe3fc940afe5c8e66741635802df2de7f95a9

The other cases are more complex, so I'll defer optimizing them until they are found to be interesting.
Comment 7 kellner 2011-03-09 00:19:13 UTC
(In reply to comment #2)
> What is your name?
Simon Kellner

> The test has been rewritten (to make it smaller and avoid transparent
> background), but you found the bug and you should be credited!
Ah, I didn't see the test directory when investigating this bug
Comment 8 Andrea Canciani 2011-03-18 02:53:16 UTC
(In reply to comment #7)
> (In reply to comment #2)
> > What is your name?
> Simon Kellner
> 
> > The test has been rewritten (to make it smaller and avoid transparent
> > background), but you found the bug and you should be credited!
> Ah, I didn't see the test directory when investigating this bug

I cleaned up and updated the branch
http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/newtests

The test case now reference you as the author (and is under the MIT
licence, which is the default for cairo tests).

If this (attribution, licence) is ok for you, I'll push it to master (and
probably it is also safe to backport it to 1.10).
Comment 9 kellner 2011-03-18 03:10:13 UTC
(In reply to comment #8)
> http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/newtests
> 
> The test case now reference you as the author (and is under the MIT
> licence, which is the default for cairo tests).
> 
> If this (attribution, licence) is ok for you, I'll push it to master (and
> probably it is also safe to backport it to 1.10).

Both attribution and licence are fine with me.  I would have been Ok
even with a less prominent attribution.
Comment 10 Andrea Canciani 2011-03-18 03:24:16 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/newtests
> > 
> > The test case now reference you as the author (and is under the MIT
> > licence, which is the default for cairo tests).
> > 
> > If this (attribution, licence) is ok for you, I'll push it to master (and
> > probably it is also safe to backport it to 1.10).
> 
> Both attribution and licence are fine with me.  I would have been Ok
> even with a less prominent attribution.

Fixed in master by:

commit c0fe55651565fa63586b7e4d675149a98c7e549c
Author: Andrea Canciani <ranma42@gmail.com>
Date:   Tue Mar 8 10:26:06 2011 +0100

    path: Fix _cairo_path_fixed_is_rectangle()
    
    __cairo_path_fixed_is_rectangle() is used by the PS and PDF backends
    to check if a path is equivalent to a rectangle when stroking. This is
    different from being a rectangle when filling, because of the implicit
    close_path appended to every subpath when filling.
    
    Fixes stroke-open-box.
    
    See https://bugs.freedesktop.org/show_bug.cgi?id=34560


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.