Closed Bug 1075399 Opened 6 years ago Closed 6 years ago

Improve the code for handling zero-length SVG subpaths

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 1 open bug)

Details

Attachments

(4 files)

No description provided.
Right now we can end up inserting lots of useless zero length segments into the path data which is a waste of memory and requires the backends to do unnecessary work. This stops us from doing that. We will still insert a little _line_ segment so simulate zero-length circles/squares when necessary via the MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS_TO_DT code.
Attachment #8498016 - Flags: review?(longsonr)
I just realised we only have zero-length subpath tests for lines with square caps:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/stroke-linecap-square-w-zero-length-segs-01.svg

I'll write a round caps equivalent in a separate patch coming up.
If we put opacity 0.1 markers on the marker-mid of the paths this will result in us drawing fewer markers won't it which will be a visible change? Will we be consistent with other UAs? The spec does not say we can omit such segments.
Attachment #8498016 - Attachment description: part 1 - Don't insert zero length segments unless it's necessary → part 2 - Don't insert zero length segments unless it's necessary
(In reply to Robert Longson from comment #3)
> If we put opacity 0.1 markers on the marker-mid of the paths this will
> result in us drawing fewer markers won't it which will be a visible change?
> Will we be consistent with other UAs? The spec does not say we can omit such
> segments.

True, but we're now getting into the realm of the ridiculous. Multiple consecutive zero length paths, with markers. I can't imagine anyone doing that unless they're specifically writing browser tests, in which case maybe they consider it a good use of their time to try and get that behavior specified (I wouldn't :) ). If we have to support that (I hope not - the WG has many more things to consider that average authors might actually care about!) then we can insert zero length straight lines in an |else| block in all the cases rather than inserting longer segment types.
Attachment #8498016 - Flags: review?(longsonr) → review+
Comment on attachment 8498048 [details] [diff] [review]
part 1 - Add more reftests for zero-length SVG subpaths

>+
>+
>+  <!-- Column 4: test loan movetos -->

What's a loan moveto? Did you mean lone?

r=longsonr if you change the comment to something that makes sense.
Attachment #8498048 - Flags: review?(longsonr) → review+
Turns out that although cairo may have drawn zero length subpaths when the line caps were round, some of the Moz2D backends do not, so the new tests fail.
Attachment #8498159 - Flags: review?(longsonr)
Attachment #8498159 - Flags: review?(longsonr) → review+
Back when I wrote the zero length stuff I made an exception for arc for some reason. Chrome and IE don't seem to make any such exception, and the spec doesn't make an exception. So we should draw zero length caps for arc too.
Attachment #8498204 - Flags: review?(longsonr)
Attachment #8498204 - Flags: review?(longsonr) → review+
Comment on attachment 8498159 [details] [diff] [review]
simulate zero length SVG subpath for stroke-linecap=round

This change revealed that layout/reftests/svg/path-01.svg has been failing on Windows with D2D because both the zero length path and the ref file have been failing to rendering anything. Now that this patch makes the test correctly render a circle when we're using D2D I've had to mark the test as failing (the reference still doesn't render). For other platforms I also had to add a bit of fuzzing since the little line that is inserted into the path makes in not quite a perfect circle. So that this fuzzing wasn't excessive I reduced the value of tinyLength in ApproximateZeroLengthSubpathSquareCaps.
(In reply to Jonathan Watt [:jwatt] from comment #9)
> This change revealed that layout/reftests/svg/path-01.svg has been failing
> on Windows with D2D because both the zero length path and the ref file have
> been failing to rendering anything.

This test came from bug 510177. The reason the ref is failing is because of the stroke being much wider than the size of the shape (bug 653762 and bug 896248). I've attached the ref to bug 653762.
Bah. I forgot to revert my testing changes to the ref file. Disabled the test temporarily before I realised that's what the issue was:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c5d5b7a4606

Once the tree reopens (for various bustages) I'll revert the ref changes and re-enable the test.
Although perhaps we should keep the ref changes (makes the test a circle) since refs are supposed to be robust, and this one clearly isn't. We'd then want to add a test to test what the ref is failing on.
Depends on: 1077791
Depends on: 1080688
No longer depends on: 1080688
You need to log in before you can comment on or make changes to this bug.