Bug 79932 - Make use of CG rounded-rect primitives
Summary: Make use of CG rounded-rect primitives
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-29 12:18 PST by Tim Horton
Modified: 2012-03-21 13:42 PDT (History)
4 users (show)

See Also:


Attachments
patch (84.30 KB, patch)
2012-02-29 13:15 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Follow-up patch (6.76 KB, patch)
2012-03-01 05:24 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
patch (14.10 KB, patch)
2012-03-20 17:11 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
style fix (14.09 KB, patch)
2012-03-20 17:16 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-02-29 12:18:53 PST
We should allow CG to optimize for use of rounded rectangles instead of always constructing them ourselves.

<rdar://problem/9274953>
Comment 1 Tim Horton 2012-02-29 13:15:51 PST
Created attachment 129500 [details]
patch
Comment 2 Simon Fraser (smfr) 2012-02-29 13:19:10 PST
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.
Comment 3 Tim Horton 2012-02-29 13:36:50 PST
Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment.
Comment 4 Mustafizur Rahaman( :rahaman) 2012-03-01 01:23:03 PST
(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?
Comment 5 Mustafizur Rahaman( :rahaman) 2012-03-01 02:35:59 PST
(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.
Comment 6 Nikolas Zimmermann 2012-03-01 03:19:07 PST
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.
Comment 7 Nikolas Zimmermann 2012-03-01 03:59:34 PST
(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.
Comment 8 Nikolas Zimmermann 2012-03-01 05:24:09 PST
Created attachment 129683 [details]
Follow-up patch
Comment 9 Nikolas Zimmermann 2012-03-01 05:25:59 PST
(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.
Comment 10 Nikolas Zimmermann 2012-03-01 05:37:01 PST
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.
Comment 11 Nikolas Zimmermann 2012-03-01 05:48:53 PST
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 :-)
Comment 12 Tim Horton 2012-03-01 13:34:57 PST
(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).
Comment 13 Tim Horton 2012-03-01 13:52:22 PST
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, ..
Comment 14 Tim Horton 2012-03-01 16:25:16 PST
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.
Comment 15 Tim Horton 2012-03-20 17:11:29 PDT
Created attachment 132936 [details]
patch
Comment 16 WebKit Review Bot 2012-03-20 17:14:11 PDT
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.
Comment 17 Tim Horton 2012-03-20 17:16:44 PDT
Created attachment 132939 [details]
style fix
Comment 18 Tim Horton 2012-03-21 13:42:13 PDT
Landed again in http://trac.webkit.org/changeset/111600