1063486 - SVG Path - Cubic bezier rendering Issue (missing some of its fill) with matching end points

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Thomas Brierley]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/37.0.2062.94 Chrome/37.0.2062.94 Safari/537.36

Steps to reproduce:

1. Create a path.

2. Add data starting with an M command followed by 2 or more C commands that end exactly on the first M command.

3. This bug also depends on the position of the points between the end points but the specific conditions are not as clear.

Example Path:

<path d="M -20,-10 20,-10 20,10 C 10,10 -10,10 -20,10 C -30,10 -30,-10 -20,-10" />

http://jsbin.com/heqalukimuxi/1/edit


Actual results:

The last cubic bezier command is partially clipped (This is not due to an incorrect viewBox or masks etc).


Expected results:

The last cubic bezier command should be drawn correctly.

Comment 1

[Alice0775 White]

If disabled HWA, I can reproduce the problem on Linux and Windows7.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (from jsbin URL)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (curve on the left should be filled in with blue, and shouldn't have stroke clipped)

Comment 4

[Daniel Holbert [:dholbert]]

I think there's a duplicate of this bug floating around, but I can't find it at the moment.

Comment 5

[Alice0775 White]

Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/2968d19b0165
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre
Bad:
http://hg.mozilla.org/mozilla-central/rev/f236632a9747
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2968d19b0165&tochange=f236632a9747

Regressed by:
f236632a9747	Jeff Muizelaar — Bug 542605. Update cairo to 12d521df8acc483b2daa844d4f05dc2fe2765ba6. r=vlad,jwatt,bas Reland after fixing quartz related clipping bug and a bunch of other ones

Comment 6

[Tom Klein]

Attachment: bug1063486.diff

When the Cairo path bounds code (_cairo_path_bounder_curve_to) processed over a curve, it wasn't setting current_point to the end of the curve if the curve already lied within the bounds of the previous path segments.  That led to setting the starting point of the next curve to be processed incorrectly, which led to processing an incorrect curve and then to incorrect bounds.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8611020 [details] [diff] [review]
bug1063486.diff

This probably needs to be reviewed by someone who's more familiar with the cairo code than I am.  Tagging jrmuizel as a start.

(I'm also not sure if we take patches directly to our cairo import, vs. try to get them fixed upstream & pull the fix in when we next grab a cairo update. The latter way is slower but cleaner, and it means we won't accidentally stomp on this patch & regress this bug when we next get a cairo update.)

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Has this been fixed in upstream?

Comment 9

[Tom Klein]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Has this been fixed in upstream?

Woops, I wasn't aware that was the actual cairo code I was looking at (slightly embarassing).  They've reworked path_bounder in the most recent cairo code, but _cairo_path_bounder_curve_to sets the current point on every call now, so yes, I would expect that it is fixed upstream.

Now I see gfx/cairo/README, so it looks like we do sometimes patch our copy of cairo?  Is that decided on a case by case basis?

And how should I address (possible) cairo bugs in the future?  Submit a patch here as "proof" and get some guidance, or report/patch on cairo and document that here, or...?  Thanks!

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tom Klein from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > Has this been fixed in upstream?
> 
> Woops, I wasn't aware that was the actual cairo code I was looking at
> (slightly embarassing).  They've reworked path_bounder in the most recent
> cairo code, but _cairo_path_bounder_curve_to sets the current point on every
> call now, so yes, I would expect that it is fixed upstream.
> 
> Now I see gfx/cairo/README, so it looks like we do sometimes patch our copy
> of cairo?  Is that decided on a case by case basis?

We do do this. It should be ok to do in this case.

> 
> And how should I address (possible) cairo bugs in the future?  Submit a
> patch here as "proof" and get some guidance, or report/patch on cairo and
> document that here, or...?  Thanks!

If you can work upstream and then we can take the patch from upstream that's preferred. However, our copy of cairo is quite old so this is not always practical.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8611020 [details] [diff] [review]
bug1063486.diff

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

It would be great if you could create a Moz2D test for this issue. There are examples in gfx/2d/unittests

Comment 12

[Tom Klein]

Attachment: Patch with test added

Added a test.

I had to add TestCairo.cpp to gfx/tests/gtest/moz.build to get those tests to run - I'm not sure if they weren't running previously or if I just wasn't invoking things correctly?

Comment 13

[Tom Klein]

Thanks Jeff.  I don't have try privileges yet unless, if appropriate, somebody would like to vouch for me on bug 1169882.  It's also not clear to me what try run is warranted in this case - maybe 
try: -b do -p linux,win32 -u reftest,crashtest,mochitests -t none ?
I'd welcome any thoughts/guidance on that - thanks!

Comment 14

[Robert Longson [:longsonr]]

I've vouched for you. The try line you've got seems fine to me.

Comment 15

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=073e3105d4b1

Comment 16

[Tom Klein]

Thanks Robert!

Comment 17

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/495a84cefc23

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/495a84cefc23