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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tomxor, Assigned: twointofive)

Tracking

({regression, testcase})

32 Branch
mozilla41
x86_64
All
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
If disabled HWA, I can reproduce the problem on Linux and Windows7.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Created attachment 8485035 [details]
testcase 1 (from jsbin URL)
Created attachment 8485043 [details]
testcase 2 (curve on the left should be filled in with blue, and shouldn't have stroke clipped)
Attachment #8485043 - Attachment description: testcase 2 → testcase 2 (curve on the left should be filled in with blue, and shouldn't have stroke clipped)
I think there's a duplicate of this bug floating around, but I can't find it at the moment.
Summary: SVG Path - Cubic bezier rendering Issue with matching end points. → SVG Path - Cubic bezier rendering Issue (missing some of its fill) with matching end points

Comment 5

4 years ago
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
Blocks: 542605
Keywords: regression, testcase
(Assignee)

Comment 6

3 years ago
Created attachment 8611020 [details] [diff] [review]
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.
Assignee: nobody → twointofive
Attachment #8611020 - Flags: review?(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.)
Attachment #8611020 - Flags: review?(dholbert) → review?(jmuizelaar)
Has this been fixed in upstream?
Flags: needinfo?(twointofive)
(Assignee)

Comment 9

3 years ago
(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!
Flags: needinfo?(twointofive)
(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 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
(Assignee)

Comment 12

3 years ago
Created attachment 8612714 [details] [diff] [review]
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?
Attachment #8611020 - Attachment is obsolete: true
Attachment #8611020 - Flags: review?(jmuizelaar)
Attachment #8612714 - Flags: review?(jmuizelaar)
Attachment #8612714 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 13

3 years ago
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!
I've vouched for you. The try line you've got seems fine to me.
(Assignee)

Comment 16

3 years ago
Thanks Robert!
https://hg.mozilla.org/mozilla-central/rev/495a84cefc23
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Duplicate of this bug: 648748
Duplicate of this bug: 1056784
(Assignee)

Updated

3 years ago
Duplicate of this bug: 620793
(Assignee)

Updated

3 years ago
Duplicate of this bug: 647979
(Assignee)

Updated

3 years ago
Duplicate of this bug: 681909
(Assignee)

Updated

3 years ago
Duplicate of this bug: 743578
(Assignee)

Updated

3 years ago
Duplicate of this bug: 664383
You need to log in before you can comment on or make changes to this bug.