Bug 47246 – Improve ellipse path generation

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Shane Stephens]

Improve ellipse path generation

[Shane Stephens]

Created attachment 69887 [details]
Patch

[Shane Stephens]

The attached patch removes some multiplications and divisions from an inner loop in svg's ellipse path generation, replacing them with a single addition of a pre-calculated delta.  This provides a 5-10% increase in frame-rate on themaninblue's svg performance test (http://www.themaninblue.com/writing/perspective/2010/03/22/).

This is a short-term fix that will be superseded by some of the changes discussed in 30994, 44374, and 46052.  However, this change only affects a handful of layout tests and is hence reasonably easy to commit.

[Shane Stephens]

Created attachment 69892 [details]
Patch

[Dirk Schulze]

Comment on attachment 69892 [details]
Patch

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

This doesn't change any pixel results? Also did you update to the latest DRT updates on trunk? Something went wrong with applying the patch to the bots.

> WebCore/platform/graphics/Path.cpp:235
> +    static const unsigned step = 100;

Statics should be at the top, right after the header includes. please name it something like gCircleFragments.

> WebCore/platform/graphics/Path.cpp:240
> +    float delta = 2.0 * piFloat / static_cast<float>(step);
> +    for (unsigned i = 1; i < step; i++) {

float delta = 2 * ....

++i instead of i++

[Shane Stephens]

Created attachment 70042 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3159:  Duplicate expectation. svg/W3C-SVG-1.1/animate-elem-80-t.svg  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3172:  Duplicate expectation. svg/clip-path/clip-in-mask-objectBoundingBox.svg  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3173:  Duplicate expectation. svg/clip-path/clip-in-mask-userSpaceOnUse.svg  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3182:  Duplicate expectation. svg/filters/filter-clip.svg  [test/expectations] [5]
Total errors found: 4 in 36 files


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

[Dirk Schulze]

Comment on attachment 70042 [details]
Patch

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

The algorithm looks good. But I guess you have to take git-svn or something like that to create patches that can be handled by the bots.

You also did not answer the question. Did you update to the latest results? Does this really not change pixel tests?

> WebCore/platform/graphics/Path.cpp:39
>  static const float QUARTER = 0.552f; // approximation of control point positions on a bezier
>                                // to simulate a quarter of a circle.

When you are on it, can you rename QUARTER please? Should match the naming schema of consts (Beginning with a g). Quarter is not really meaningful. Maybe you find a better name. And align the comments please

> WebCore/platform/graphics/Path.cpp:42
> +// number of path segments used to approximate circles and ellipses.
> +static const unsigned gEllipseFragments = 100;

Not sure if we need the comment at all. The name says all.

[Dirk Schulze]

(In reply to comment #6)
> Attachment 70042 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> LayoutTests/platform/chromium/test_expectations.txt:3159:  Duplicate expectation. svg/W3C-SVG-1.1/animate-elem-80-t.svg  [test/expectations] [5]
> LayoutTests/platform/chromium/test_expectations.txt:3172:  Duplicate expectation. svg/clip-path/clip-in-mask-objectBoundingBox.svg  [test/expectations] [5]
> LayoutTests/platform/chromium/test_expectations.txt:3173:  Duplicate expectation. svg/clip-path/clip-in-mask-userSpaceOnUse.svg  [test/expectations] [5]
> LayoutTests/platform/chromium/test_expectations.txt:3182:  Duplicate expectation. svg/filters/filter-clip.svg  [test/expectations] [5]
> Total errors found: 4 in 36 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

What does it mean?

[Shane Stephens]

Created attachment 70044 [details]
Patch

[Shane Stephens]

Dirk - sorry, I was trying to get the style checker passing before getting you to review again.  I guess I should be clearing the review flag until the patch is ready to go?  At any rate, the duplicate expectations are now fixed.

In response to your earlier question about pixel tests, there aren't any webkit pixel-by-pixel comparisons, and the lines in test_expectations indicate that the chromium build should temporarily expect these tests to fail.  Once this patch lands I can extract the new images from our buildbots and remove those lines from test_expectations as a separate patch.

I'll respond to your latest set of change requests now and re-upload.

[Shane Stephens]

Created attachment 70053 [details]
Patch

[Shane Stephens]

Argh, sorry someone *just* got in a bunch of changes to all the test expectations.  It'll take me a while to fix this merge conflict, and it's getting late here.  I'll do it tomorrow morning, Australian time.

[Shane Stephens]

Created attachment 70172 [details]
Patch

[Sam Weinig]

Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?

[Sam Weinig]

(In reply to comment #14)
> Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?

Ignore this, wrong bug.

[Shane Stephens]

Closing this bug as 69517 fixes the problem.

[Dirk Schulze]

Comment on attachment 70172 [details]
Patch

clearing flags