Bug 79932 – Make use of CG rounded-rect primitives

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tim Horton]

We should allow CG to optimize for use of rounded rectangles instead of always constructing them ourselves.

<rdar://problem/9274953>

[Tim Horton]

Created attachment 129500 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 129500 [details]
patch

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

> Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm:181
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

Not sure why you need the PLATFORM(MAC) here. This is a .mm file.

[Tim Horton]

Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment.

[Mustafizur Rahaman( :rahaman)]

(In reply to comment #3)
> Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment.

This is giving a build break on Windows port for revision: 109309 & the error is
\platform\graphics\cg\PathCG.cpp(244) : error C3861: 'wkCGPathAddRoundedRect': identifier not found.

Did any one notice this before?

[Mustafizur Rahaman( :rahaman)]

(In reply to comment #4)
> (In reply to comment #3)
> > Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment.
> 
> This is giving a build break on Windows port for revision: 109309 & the error is
> \platform\graphics\cg\PathCG.cpp(244) : error C3861: 'wkCGPathAddRoundedRect': identifier not found.
> 
> Did any one notice this before?

Upon further investigation I found that, the cause of the build break is that the changes in WebCore are inside
#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD), where the expected behavior is that these changes should work for LION & above(as per ChangeLog), but because of the above conditional the changes are applied for non-MAC ports also.

So, I was thinking the correct fix should be something like
#if PLATFORM(MAC)
#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
Proposed Changes..
#endif
#endif

If any one can please comment on this, I can create a new issue & submit a patch for the same, Thanks.

[Nikolas Zimmermann]

Reopening bug. Some pixel test results for SVG now needs rebaselines.
One test broke, that's why I'm reopening: svg/custom/dasharrayOrigin.svg.
It uses rounded <rects> and stroke-dasharray - it seems that combination doesn't work when using the optimized primitives from CG.

[Nikolas Zimmermann]

(In reply to comment #5)
> #if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD), where the expected behavior is that these changes should work for LION & above(as per ChangeLog), but because of the above conditional the changes are applied for non-MAC ports also.
That only affects CG/Win, as win is the only non-mac port using CoreGraphics.
I think your idea is correct, unless Tim can supply the needed function for win as well.

[Nikolas Zimmermann]

Created attachment 129683 [details]
Follow-up patch

[Nikolas Zimmermann]

(In reply to comment #8)
> Created an attachment (id=129683) [details]
> Follow-up patch

This includes the PLATFORM(MAC) fix mentioned by Mustafizur Rahaman as well, which should fix CG/Win builds again.
This patch lets my Lion SVG pixel baseline pass with --tolerance 0 again.

I've switched to the old code path for SVG again, as dashed strokes seem to fail with the new native approach. Maybe it'll never work as SVG expects it.

[Nikolas Zimmermann]

Comment on attachment 129683 [details]
Follow-up patch

(In reply to comment #9)
> I've switched to the old code path for SVG again, as dashed strokes seem to fail with the new native approach. Maybe it'll never work as SVG expects it.

I've discussed this with Zoltan on IRC -- I'm holding back this patch, we should discuss with Tim/Simon first if wkCGPathAddRoundedRect can be fixed, if not, we can revisit my patch, but only pass the 'PreferBeziers..' argument if the SVG stroke-dasharray is set, otherwise use the default PreferNative method.

I'm going to roll this out instead for now, to have passing tests again.

[Nikolas Zimmermann]

Reverted r109255 for reason:
Breaks rounded rects with dashed strokes in SVG

Committed r109340: <http://trac.webkit.org/changeset/109340>

Note: I didn't try, but if we have test cases of rounded rects in <canvas> with webkitLineDash set, then this should be broken as well.

Needs Apple comments, what this fast-path actually supports :-)

[Tim Horton]

(In reply to comment #11)
> Reverted r109255 for reason:
> Breaks rounded rects with dashed strokes in SVG
> 
> Committed r109340: <http://trac.webkit.org/changeset/109340>
> 
> Note: I didn't try, but if we have test cases of rounded rects in <canvas> with webkitLineDash set, then this should be broken as well.
> 
> Needs Apple comments, what this fast-path actually supports :-)

Canvas doesn't have an intrinsic rounded rectangle, so it's not affected.

What did you see? All I see is that the origin of the line dash is incorrect. I'm not completely sure how to fix it, but I'm not sure that that calls into question what the fast path supports (after all, it is just modifying the path, in the end).

[Tim Horton]

From IRC:

thorton: Wild|AWAY: what would you prefer!
Wild|AWAY: thorton: platformAddRoundedRect maybe? which can then use fallbackAddRoundedRect, if it has no platform impl
Wild|AWAY: s/fallbackAddRoundedRect/addRoundedRectsCreatedByBeziers/, and put that in Path.cpp
Wild|AWAY: and the platformAdd.. in the PathCG.cpp / PathQt.cpp (not now, but in future) etc, ..

[Tim Horton]

I think we should land this plus Niko's patch, plus the Windows fix, plus the minor refactor that Niko wants, ending with it enabled in all cases except SVG. All of the CSS tests passed, and CSS doesn't specify line dash origin; Canvas can't hit this code because it has neither a rounded rectangle primitive nor dashed line support.

The path created by wkCGPathAddRoundedRect currently starts in the middle of the righthand segment (the 'origin' if you were to split this into quadrants/consider it as a circle). The path created by addBeziersForRoundedRect starts at the left of the topmost segment.

To adjust the dash origin, we would have to somehow plumb knowledge of which rounded rectangle method was used through from Path to GraphicsContext; specifically, we'd need that knowledge when setting the line dash settings. Then, we can offset the phase of the line dash by the correct distance (I've worked it out for the case where rx and ry are equal -- and that works -- but not the more general case). This seems quite irritating, though, since knowledge of the Path and the code that set the line dash are separated by quite a gap.

[Tim Horton]

Created attachment 132936 [details]
patch

[WebKit Review Bot]

Attachment 132936 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/Path.h:155:  The parameter name "strategy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 132939 [details]
style fix

[Tim Horton]

Landed again in http://trac.webkit.org/changeset/111600