Bug 30994 – Better Ellipses

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: SVG

[Jeff Schiller]

Created attachment 42274 [details]
Patch to use ellipse-drawing

I noticed that ellipses are actually drawn as 100-sided polygons instead of arc paths.

Surely this was done for a reason, so I was hoping to find out what that reason was.  Is it proven to be better performing across the various 2D graphics APIs that WebKit supports?  Perhaps to better support mobile devices the 100-sided polygon approach was chosen?

I'm attaching a patch that uses path.addEllipse instead of the 100 addLineTo() commands.  I will also add a simple test case.  Compiling these changes into Safari I can actually tell the difference between Chrome rendering and WebKit-rendering on the test case.  In Chrome I can definitely see "corners" in the ellipse.

I also took the opportunity to add some 'const' declarations into the WebCore/platform/graphics/Path.cpp in the vain hope that it might actually help compilers optimize things a little better.  I did not actually confirm whether the compiled assembly _is_ better, but at the very least I've never heard of 'const' hurting performance.  What's the culture here?  Do these types of things need to be separate patches?

[Jeff Schiller]

Created attachment 42275 [details]
Test case exhibiting the problem

In current WebKit I can actually detect "corner"s in this ellipse arc.  The attached patch fixes that.

[Jeff Schiller]

Created attachment 42277 [details]
Updated patch, removed old code and commented out version.  Also updated to tip.

Please look at this patch and not the first one I submitted.

[Jeff Schiller]

Note that this was marked as SVG, but I believe this patch would also affect the HTML Canvas element.

[Dirk Schulze]

I would realy prefer to let the platforms draw the ellipses/circles. They could do it faster but more important, the rendering gets better.
But I guess this will break hunderts of LayoutTests. Because atm we draw a couple of lines, and with your patch we draw just the ellipse.
I would prefer to change all LayoutTests and the way we create them for ellipses and circles.

[Jeff Schiller]

795.95s total testing time

11454 test cases (99%) succeeded
115 test cases (<1%) had incorrect layout
17 test cases (<1%) had stderr output

Dirk is right, many tests break with this patch.  Here's a fragment of one diff (svg/W3C-SVG-1.1/shapes-circle-01-t) after the patch has been applied:

-      RenderPath {circle} at (49.50,49.50) size 101.00x101.00 [stroke={[type=SOLID] [color=#000000]}] [data="M150.00,100.00 L149.90,103.14 L149.61,106.27 L149.11,109.37 L148.43,112.43 L147.55,115.45 L146.49,118.41 L145.24,121.29 L143.82,124.09 L142.22,126.79 L140.45,129.39 L138.53,131.87 L136.45,134.23 L134.23,136.45 L131.87,138.53 L129.39,140.45 L126.79,142.22 L124.09,143.82 L121.29,145.24 L118.41,146.49 L115.45,147.55 L112.43,148.43 L109.37,149.11 L106.27,149.61 L103.14,149.90 L100.00,150.00 L96.86,149.90 L93.73,149.61 L90.63,149.11 L87.57,148.43 L84.55,147.55 L81.59,146.49 L78.71,145.24 L75.91,143.82 L73.21,142.22 L70.61,140.45 L68.13,138.53 L65.77,136.45 L63.55,134.23 L61.47,131.87 L59.55,129.39 L57.78,126.79 L56.18,124.09 L54.76,121.29 L53.51,118.41 L52.45,115.45 L51.57,112.43 L50.89,109.37 L50.39,106.27 L50.10,103.14 L50.00,100.00 L50.10,96.86 L50.39,93.73 L50.89,90.63 L51.57,87.57 L52.45,84.55 L53.51,81.59 L54.76,78.71 L56.18,75.91 L57.78,73.21 L59.55,70.61 L61.47,68.13 L63.55,65.77 L65.77,63.55 L68.13,61.47 L70.61,59.55 L73.21,57.78 L75.91,56.18 L78.71,54.76 L81.59,53.51 L84.55,52.45 L87.57,51.57 L90.63,50.89 L93.73,50.39 L96.86,50.10 L100.00,50.00 L103.14,50.10 L106.27,50.39 L109.37,50.89 L112.43,51.57 L115.45,52.45 L118.41,53.51 L121.29,54.76 L124.09,56.18 L126.79,57.78 L129.39,59.55 L131.87,61.47 L134.23,63.55 L136.45,65.77 L138.53,68.13 L140.45,70.61 L142.22,73.21 L143.82,75.91 L145.24,78.71 L146.49,81.59 L147.55,84.55 L148.43,87.57 L149.11,90.63 L149.61,93.73 L149.90,96.86 Z"]

+      RenderPath {circle} at (49.50,49.50) size 101x101 [stroke={[type=SOLID] [color=#000000]}] [data="M150.00,100.00 L150.00,100.00 C150.00,127.61,127.61,150.00,100.00,150.00 C72.39,150.00,50.00,127.61,50.00,100.00 C50.00,72.39,72.39,50.00,100.00,50.00 C127.61,50.00,150.00,72.39,150.00,100.00 C150.00,100.00,150.00,100.00,150.00,100.00 Z"]

I'm willing to do the work here, but I have several questions:

1) Do we agree this patch is a good idea (rendering-wise, speed-wise)?  Is it ok to do this for all platforms?

2) I realize now that one benefit (and maybe the reason?) of the 100-sided polygon approach is that the layout is consistent across all platforms and is therefore easier to test.  With my patch, we let the platforms decide what to do: in CG's case we see several bezier curves being used but we'd see something different in cairo/qt/etc.  Do we need to make platform-specific expected files? 

3) Is it ok to just take the output of the patched WebKit and update the mac-specific results?  I'm not clear on how to do that nor what the policy is.  Is it up to the individual platforms to add their 'expected' files in this case and we just need to do the 'mac' ones here?

[Dirk Schulze]

On the one hand I find it useful to have this data from te platforms and compare this to each other, but on the other hand we don't have results of paths in Canvas. Can we do it without the platform path data, and just the RenderPath {circle} at (49.50,49.50) size 101x101 [stroke={[type=SOLID]
[color=#000000]}]?
What about simulate ellipses and circles with bezier curves in general? bezier curves should be supported by all platforms.

[Nikolas Zimmermann]

Comment on attachment 42277 [details]
Updated patch, removed old code and commented out version.  Also updated to tip.

I like the introduction of the halfWidth/halfHeight variables. The s/float/const float/ changes are unnecessary though, just makes the code harder to read.
The createEllipse changes are great though. As discussed in private, we need to change DRT dumping of shapes first, before landing this patch makes sense.
(Shouldn't affect any textual outputs, after we change shapes to not dump path data)

[Jeff Schiller]

Is this now a dupe of Bug 46052?

[Andreas Kling]

(In reply to comment #8)
> Is this now a dupe of Bug 46052?

That's correct! Thanks for the CC :)

*** This bug has been marked as a duplicate of bug 46052 ***