Bug 71820

Summary: Linecaps wrong for zero length lines
Product: WebKit Reporter: Stephen Chenney <schenney>
Component: SVGAssignee: Stephen Chenney <schenney>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, j.tosovsky, kbalazs, rwlbuis, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 73021, 75703    
Bug Blocks: 64675, 73690, 76931    
Attachments:
Description Flags
Test case for zero-length stroke linecaps
none
Test case for all zero-length-path rendering cases.
none
An even more thorough test.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Stephen Chenney 2011-11-08 08:29:28 PST
Created attachment 114079 [details]
Test case for zero-length stroke linecaps

The SVG standard for line caps defines the behavior for zero length lines and round or square caps. See
http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/painting-control-04-f.html

However, when multiple zero length lines appear in the same path, the linecap drawing is incorrect. See attachment, that in WebKit correctly shows the top row but fails to show the following two rows.

This is also http://code.google.com/p/chromium/issues/detail?id=68319
Comment 1 Stephen Chenney 2011-11-21 14:06:14 PST
Created attachment 116133 [details]
Test case for all zero-length-path rendering cases.

This test case covers all of the zero-length single path cases. Currently we fail on "m 0 0 z".

Firefox fails on all but the "m 0 0" case.

It looks like Qt fails on all of the round cap cases, but works for all of the square cap cases, which is really odd.
Comment 2 Stephen Chenney 2011-11-21 15:04:45 PST
Created attachment 116145 [details]
An even more thorough test.
Comment 3 Stephen Chenney 2012-01-12 16:35:03 PST
Created attachment 122336 [details]
Patch

DO NOT COMMIT pending Skia changes rolling in
Comment 4 Stephen Chenney 2012-01-19 07:40:44 PST
Nudge. If the review is too large, I can break out the initial refactoring changes and then add the actual feature fix in a second patch.
Comment 5 Nikolas Zimmermann 2012-01-19 07:52:46 PST
Comment on attachment 122336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122336&action=review

Sorry it took so long, this is a really nice patch - it only needs some style tweaks I think.
Do you see any negative performance impact for common-cases? I think it doesn't impose any burden for non-zero-length subpath.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> +    Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin();
> +    while (iter != m_zeroLengthLinecapLocations.end()) {

vectors should be accessed by index, this is preferred.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> +        float strokeWidth = this->strokeWidth();

This shouldn't be inside the loop, or directly use strokeWidth().

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
> +                                const Color& fallbackColor, const bool nonScalingStroke, const AffineTransform& nonScalingStrokeTransform,

We don't constify POD types usually.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:317
> +    Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin();
> +    while (iter != m_zeroLengthLinecapLocations.end()) {

Should walk vector by index.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:437
> +    Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin();

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:475
> +class ZeroLengthSubpathFinder {

This needs to go into its own file, similar to SVGMarkerFoobar.cpp etc.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:489
> +        switch (element->type) {

Good job!

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:506
> +            if (subpathFinder->m_lastPoint != element->points[0]
> +                || element->points[0] != element->points[1])

I'd rather not wrap lines here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:515
> +            if (subpathFinder->m_lastPoint != element->points[0]
> +                || element->points[0] != element->points[1]

ditto.

> Source/WebCore/svg/SVGPathSegListBuilder.h:53
> +    // Used in UnalteredParsing/NormalizedParsing modes.

You could land this seperated, to make the patch a bit smaller.
Comment 6 Stephen Chenney 2012-01-19 11:45:36 PST
Created attachment 123159 [details]
Patch

DO NOT COMMIT pending Skia changes rolling in
Comment 7 Stephen Chenney 2012-01-19 12:00:59 PST
(In reply to comment #5)
> (From update of attachment 122336 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122336&action=review
> 
> Sorry it took so long, this is a really nice patch - it only needs some style tweaks I think.
> Do you see any negative performance impact for common-cases? I think it doesn't impose any burden for non-zero-length subpath.

Thanks for the review. I know you are very busy with all the additional traffic coming your way.

I did not do any significant performance checking, but for the most common case (non-degenerate paths) the only significant cost is an iteration over the path at the time it is created from the element. Given everything else that goes on in that situation, such as constructing parsers and path builders, the cost seems very reasonable.

Let me know if you have ideas on how to be more concrete about the performance cost. 

> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> > +    Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin();
> > +    while (iter != m_zeroLengthLinecapLocations.end()) {
> 
> vectors should be accessed by index, this is preferred.

Done, everywhere.

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> > +        float strokeWidth = this->strokeWidth();
> 
> This shouldn't be inside the loop, or directly use strokeWidth().

Done

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
> > +                                const Color& fallbackColor, const bool nonScalingStroke, const AffineTransform& nonScalingStrokeTransform,
> 
> We don't constify POD types usually.

I think I understood your comment. Please check and let me know if I did what you expected.

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:475
> > +class ZeroLengthSubpathFinder {
> 
> This needs to go into its own file, similar to SVGMarkerFoobar.cpp etc.

Done. Create RenderSVGSubpathData.h

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:506
> > +            if (subpathFinder->m_lastPoint != element->points[0]
> > +                || element->points[0] != element->points[1])
> 
> I'd rather not wrap lines here.

Done.

> > Source/WebCore/svg/SVGPathSegListBuilder.h:53
> > +    // Used in UnalteredParsing/NormalizedParsing modes.
> 
> You could land this seperated, to make the patch a bit smaller.

I would rather not, just to avoid the overhead. Let me know if you would rather them be separate.

There are also some extra expectations changes. There has been some thrashing in that area of the expectations, so the patch may not apply.

Again, please do not CQ+ because we need to get a Skia roll in for chromium, then a chromium deps update for WebKit.
Comment 8 Stephen Chenney 2012-01-24 09:02:49 PST
Created attachment 123745 [details]
Patch

Updated patch. Ready for commit.
Comment 9 Nikolas Zimmermann 2012-01-24 11:20:11 PST
Comment on attachment 123745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123745&action=review

r=me with additional comments:

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:139
> +    // FIXME: This is not correct for round linecaps

Missing period. I'd prefer if you'd link to a bug report here, can you fix that before landing?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:432
> +    for (size_t i = 0; i < m_zeroLengthLinecapLocations.size(); ++i) {

No need for the braces, they should be removed.

> Source/WebCore/rendering/svg/RenderSVGSubpathData.h:29
> +class RenderSVGSubpathData {

Oh, this should be named SVGSubpathData, or SVGZeroLengthSubpathHandler, but no Render* prefix. For SVG, everything starting with Render is a RenderObject dervied object. Stuff like SVGMarkerData, etc. is not.
Comment 10 Stephen Chenney 2012-01-24 13:03:59 PST
Created attachment 123788 [details]
Patch

Review issues addressed. Commit this.
Comment 11 Stephen Chenney 2012-01-24 13:04:46 PST
Thanks for the comments Nikolas. Now on to the related bugs. :-)
Comment 12 WebKit Review Bot 2012-01-24 13:22:02 PST
Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11344049
Comment 13 Early Warning System Bot 2012-01-24 13:24:00 PST
Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11159045
Comment 14 Gyuyoung Kim 2012-01-24 13:30:31 PST
Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11342049
Comment 15 Stephen Chenney 2012-01-24 15:46:44 PST
Created attachment 123831 [details]
Patch

Hate it when I forget to add a file
Comment 16 Early Warning System Bot 2012-01-24 16:24:19 PST
Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11177104
Comment 17 Gyuyoung Kim 2012-01-24 16:27:02 PST
Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11294109
Comment 18 WebKit Review Bot 2012-01-24 16:36:38 PST
Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11264126
Comment 19 Stephen Chenney 2012-01-24 17:06:34 PST
Created attachment 123847 [details]
Patch

Argh. Third time is a charm
Comment 20 WebKit Review Bot 2012-01-25 07:31:53 PST
Comment on attachment 123847 [details]
Patch

Clearing flags on attachment: 123847

Committed r105878: <http://trac.webkit.org/changeset/105878>
Comment 21 WebKit Review Bot 2012-01-25 07:32:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Balazs Kelemen 2012-01-25 09:35:53 PST
it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks!
Comment 23 Stephen Chenney 2012-01-25 09:49:37 PST
(In reply to comment #22)
> it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks!

There are text differences in the bounding boxes that I believe are due to a different problem.  I've filed https://bugs.webkit.org/show_bug.cgi?id=77020 to track it.

You can either skip the test for now, or accept the new results pending further investigation. For Chromium we will be accepting then fixing.
Comment 24 Balazs Kelemen 2012-01-25 10:03:42 PST
(In reply to comment #23)
> (In reply to comment #22)
> > it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks!
> 
> There are text differences in the bounding boxes that I believe are due to a different problem.  I've filed https://bugs.webkit.org/show_bug.cgi?id=77020 to track it.
> 
> You can either skip the test for now, or accept the new results pending further investigation. For Chromium we will be accepting then fixing.

I have skipped them for now: http://trac.webkit.org/changeset/105889