Bug 71820 – Linecaps wrong for zero length lines

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen Chenney]

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

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 116145 [details]
An even more thorough test.

[Stephen Chenney]

Created attachment 122336 [details]
Patch

DO NOT COMMIT pending Skia changes rolling in

[Stephen Chenney]

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.

[Nikolas Zimmermann]

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.

[Stephen Chenney]

Created attachment 123159 [details]
Patch

DO NOT COMMIT pending Skia changes rolling in

[Stephen Chenney]

(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.

[Stephen Chenney]

Created attachment 123745 [details]
Patch

Updated patch. Ready for commit.

[Nikolas Zimmermann]

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.

[Stephen Chenney]

Created attachment 123788 [details]
Patch

Review issues addressed. Commit this.

[Stephen Chenney]

Thanks for the comments Nikolas. Now on to the related bugs. :-)

[WebKit Review Bot]

Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11344049

[Early Warning System Bot]

Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11159045

[Gyuyoung Kim]

Comment on attachment 123788 [details]
Patch

Attachment 123788 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11342049

[Stephen Chenney]

Created attachment 123831 [details]
Patch

Hate it when I forget to add a file

[Early Warning System Bot]

Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11177104

[Gyuyoung Kim]

Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11294109

[WebKit Review Bot]

Comment on attachment 123831 [details]
Patch

Attachment 123831 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11264126

[Stephen Chenney]

Created attachment 123847 [details]
Patch

Argh. Third time is a charm

[WebKit Review Bot]

Comment on attachment 123847 [details]
Patch

Clearing flags on attachment: 123847

Committed r105878: <http://trac.webkit.org/changeset/105878>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

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!

[Stephen Chenney]

(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.

[Balazs Kelemen]

(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