Closed
Bug 1075399
Opened 10 years ago
Closed 10 years ago
Improve the code for handling zero-length SVG subpaths
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jwatt, Assigned: jwatt)
References
(Depends on 1 open bug)
Details
Attachments
(4 files)
(deleted),
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8498048 -
Flags: review?(longsonr)
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8498016 -
Flags: review?(longsonr) → review+
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8498159 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 8•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8498204 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70d8b88af2a6
https://hg.mozilla.org/mozilla-central/rev/8f7fc1fdc6e4
https://hg.mozilla.org/mozilla-central/rev/a6976dea9d56
https://hg.mozilla.org/mozilla-central/rev/78b5cff9b699
https://hg.mozilla.org/mozilla-central/rev/4c5d5b7a4606
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•