Add support for new canvas ellipse method.

Add support for new canvas ellipse method.

Canvas v5 API adds a new path segment type: ellipse.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse

Add ellipse API into Path.h because Canvas v5 adds following API also.
   path.ellipse(x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise)

When we support Path primitives, we can reuse Path::ellipse.

BUG=130260
TEST=fast/canvas/canvas-ellipse-360-winding.html,
     fast/canvas/canvas-ellipse-circumference.html,
     fast/canvas/canvas-ellipse-connecting-line.html,
     fast/canvas/canvas-ellipse-zero-lineto.html,
     fast/canvas/canvas-ellipse.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156828

[Stephen Chennney, 2013-04-26 14:33:24]

While this is a relatively benign API change, it still needs to go through an
"Intent to Implement" phase. See the relevant postings on the blink-dev mailing
list.

[Stephen Chennney, 2013-04-26 14:51:02]

Link to the relevant spec please.

Are there W3C tests for this new functionality?

Testing doesn't cover a great range of cases. In particular, I do not see enough
tests for degenerate ellipses.

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasPathMethods.cpp:137: return sa - 2 * piFloat;
This is not correct. If they differ by more than 4 pi you will fail. The right
way to do this is with a while loop.

And what about cases where you are told to go anticlockwise and the angle is
less than -2 pi (or negative at all?).

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasPathMethods.cpp:188:
m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle,
anticlockwise);
You should catch the common case of an entire ellipse, and use the optimized
path for that.

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasRenderingContext2D.idl:127: [RaisesException] void
ellipse([Default=Undefined] optional float x,
What good is an optional default=undefined value? From the code it seems that
you must define all of the parameters in order to draw anything.

[Stephen Chennney, 2013-04-26 14:51:31]

On 2013/04/26 14:51:02, Stephen Chenney wrote:
> Link to the relevant spec please.

Sorry, I see you have that.

[Stephen Chennney, 2013-04-26 14:57:56]

Corrected myself.

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasPathMethods.cpp:137: return sa - 2 * piFloat;
I see, it is correct according to the spec. But then you know you have a
complete ellipse, so it should be optimizable somehow.

[dshwang, 2013-04-26 15:23:33]

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasPathMethods.cpp:188:
m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle,
anticlockwise);
On 2013/04/26 14:51:03, Stephen Chenney wrote:
> You should catch the common case of an entire ellipse, and use the optimized
> path for that.

Thank you for review, Stephen. Yes, your opinion is great.
However, without CanvasPathMethods::ellipse, adjustEndAngle() is just extracted
from CanvasPathMethods::arc(). I mean that CanvasPathMethods::arc() behavior is
not changed. And your opinion is possible to apply arc() also. How about
separating optimization patch for both arc() and ellipse()?

If you say ok, I'll file it.

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasRenderingContext2D.idl:127: [RaisesException] void
ellipse([Default=Undefined] optional float x,
On 2013/04/26 14:51:03, Stephen Chenney wrote:
> What good is an optional default=undefined value? From the code it seems that
> you must define all of the parameters in order to draw anything.

Aha, I see. I'll put [Default=Undefined] carefully after reading spec again.

BTW, I'll file another bug to remove [Default=Undefined] from other API (e.g.
arc()). I followed arc() without thought.

[dshwang, 2013-04-26 15:26:04]

On 2013/04/26 14:33:24, Stephen Chenney wrote:
> While this is a relatively benign API change, it still needs to go through an
> "Intent to Implement" phase. See the relevant postings on the blink-dev
mailing
> list.

Thank you for directing. I'll go through "Intent to Implement". And I'll search
there are W3C tests for this new functionality.

[Stephen Chennney, 2013-04-26 16:02:04]

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasPathMethods.cpp:188:
m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle,
anticlockwise);
On 2013/04/26 15:23:33, dshwang wrote:
> On 2013/04/26 14:51:03, Stephen Chenney wrote:
> > You should catch the common case of an entire ellipse, and use the optimized
> > path for that.
> 
> Thank you for review, Stephen. Yes, your opinion is great.
> However, without CanvasPathMethods::ellipse, adjustEndAngle() is just
extracted
> from CanvasPathMethods::arc(). I mean that CanvasPathMethods::arc() behavior
is
> not changed. And your opinion is possible to apply arc() also. How about
> separating optimization patch for both arc() and ellipse()?
> 
> If you say ok, I'll file it.

You can certainly change both ellipse and arc. I'm not certain that it will
behave correctly due to possible differences in Skia behavior, but I would
certainly like you to try. We are actively attempting to use Skia optimized
primitives whenever we can.

[dshwang, 2013-04-29 12:46:02]

Hi, The second patch is WIP because I'll add various tests. I need to feedback
for implementation. :)

> Are there W3C tests for this new functionality?
> Testing doesn't cover a great range of cases. In particular, I do not see
enough
> tests for degenerate ellipses.
1. Unfortunately, there are not any standard tests for canvas ellipse API. So
I'll make various tests based on arc tests.

2. I sent "Intent to Implement and Ship" to blink-dev:
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/blink-dev/Z...

> You should catch the common case of an entire ellipse, and use the optimized
> path for that.
3. I add the optimization code of an entire ellipse in Path::addEllipse()
instead of CanvasPathMethods::ellipse(), because I need other clients using
Path::addEllipse() to have the benefit of the code.

> What good is an optional default=undefined value?
4. I removed all [default=undefined] because all parameters are mandatory.

[dshwang, 2013-07-09 13:00:07]

Hi, long time no see.

I update this patch.

I add 4 tests more. So now there are 6 tests.
There are two kinds.

1. 5 tests are ellipse version of 5 arc tests. These tests uses html ref test.
I implement ellipse js function: ellipseUsingArc() in
js-ellipse-implementation.js. Expected html uses ellipseUsingArc(). It means
that let's check ellipse implementation using arc because arc implementation is
correct.

fast/canvas/canvas-ellipse-360-winding.html, <- ellipse clone of
canvas-arc-360-winding.html
fast/canvas/canvas-ellipse-connecting-line.html, <- ellipse clone of
canvas-arc-connecting-line.html
fast/canvas/canvas-ellipse-zero-lineto.html <- ellipse clone of
canvas-arc-zero-lineto.html
fast/canvas/ellipse-crash.html, <- ellipse clone of arc-crash.html
fast/canvas/ellipse360.html <- ellipse clone of arc360.html

2. Common canvas test using getImageData() api.
It means that this test does not depend on both arc impl and ellipseUsingArc().
fast/canvas/canvas-ellipse.html <- this test uses ellipse in a bit complex
manner.

[aandrey, 2013-07-09 13:49:55]

FYI

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa ==
endAngle) {
sa -> startAngle

[Stephen White, 2013-07-09 13:58:07]

https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap...
File Source/core/platform/graphics/Path.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap...
Source/core/platform/graphics/Path.cpp:313: if (!rotation && !startAngle &&
SkScalarNearlyEqual(twoPiScalar, SkScalarAbs(endAngleScalar))) {
If we're doing an approximate comparison for the end angle, shouldn't we also be
doing an approximate comparison for the start angle? Or exact for both?

[alph, 2013-07-09 14:16:00]

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa +
rot), y + rx * sinf(sa + rot));
rx used twice here.
Also I suppose rotation should also rotate ellipse axes, while here it just
affects starting point.

[dshwang, 2013-07-09 14:42:04]

thx you for review :)

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa ==
endAngle) {
On 2013/07/09 13:49:56, aandrey wrote:
> sa -> startAngle
oops, thx!

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa +
rot), y + rx * sinf(sa + rot));
On 2013/07/09 14:16:00, alph wrote:
> rx used twice here.
> Also I suppose rotation should also rotate ellipse axes, while here it just
> affects starting point.

thx for review. Yeah, using rx twice is wrong.

how about as follows:?
lineTo(x + rx * cosf(sa + rot), y + ry * sinf(sa + rot));
The point (x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)) is on the ellipse.

It means to draw line to start point.
It is because arc() also draw line to start point.
line 156: lineTo(x + r * cosf(startAngle), y + r * sinf(startAngle));

The canvas spec does not describe this edge case. So I just match ellipse to
arc.

https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap...
File Source/core/platform/graphics/Path.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap...
Source/core/platform/graphics/Path.cpp:313: if (!rotation && !startAngle &&
SkScalarNearlyEqual(twoPiScalar, SkScalarAbs(endAngleScalar))) {
On 2013/07/09 13:58:08, Stephen White wrote:
> If we're doing an approximate comparison for the end angle, shouldn't we also
be
> doing an approximate comparison for the start angle? Or exact for both?

I made it with intention. Let me explain.

First of all, float a = 0 is just zero, not 0.0000000000001.

Sencond, I'm not sure that Math.PI * 2 in javascript equals 2 * piFloat in cpp.
Could I ensure they are exact in X86, X64 and ARM?

In addition, I can not expect how web developer use 2Pi. Smart developers will
use Math.PI * 2. However, I found that some code use something like 3.1415 * 2.
For example, fast/canvas/canvas-transforms-during-path.html

So imho, this code looks good to me. Could you feedback again? :)

[alph, 2013-07-09 14:43:08]

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa ==
endAngle) {
If one of radii is not zero I'd expect the stroked ellipse to degenerate into a
line, not to the empty thing.

[alph, 2013-07-09 15:16:35]

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa +
rot), y + rx * sinf(sa + rot));
On 2013/07/09 14:42:05, dshwang wrote:
> On 2013/07/09 14:16:00, alph wrote:
> > rx used twice here.
> > Also I suppose rotation should also rotate ellipse axes, while here it just
> > affects starting point.
> 
> thx for review. Yeah, using rx twice is wrong.
> 
> how about as follows:?
> lineTo(x + rx * cosf(sa + rot), y + ry * sinf(sa + rot));
> The point (x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)) is on the
ellipse.
> 
> It means to draw line to start point.
> It is because arc() also draw line to start point.
> line 156: lineTo(x + r * cosf(startAngle), y + r * sinf(startAngle));
> 
> The canvas spec does not describe this edge case. So I just match ellipse to
> arc.

I don't like it. It works for arc because its radii are the same.
If you do it this way for ellipse, then in rotated case with rx!=ry you'll be
getting distinct points when startAndle is close to endAngle and when they are
equal. So if someone makes an animation where endAngle decreases down to
startAngle, suddenly it will jump to a distinct location when endAngle reaches
startAngle. That doesn't seem what people would expect.

Here's an example:
x = 0
y = 0
rx = 100
ry = 5
rot = pi/2
startAngle = 0
endAngle = 1e-6
The point should be (0;100), while for endAngle=0 you'll be getting (0;5)

[dshwang, 2013-07-09 15:48:01]

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:215: degenerateEllipse(this, x, y,
radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise);
@alph yes, you are right. I understand what you say.

How about this implementation?

Or if we want simpler implementation with losing some precision, below solution
can work.
Replace below code to degenerateEllipse(...)
if (!radiusX)
    radiusX = 0.00001;
if (!radiusY)
    radiusY = 0.00001;

Which do you prefer?

[dshwang, 2013-07-09 15:57:35]

On 2013/07/09 15:48:01, dshwang wrote:

You don't have to explain axis or something. I understand now it is wrong.

btw, I want your feedback about this approach.

[alph, 2013-07-09 16:21:23]

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:178: path->lineTo(x + radiusX *
cosf(endAngle + rotation), y + radiusY * sinf(endAngle + rotation));
Here's a counter example:
rotation = x = y = 0;
startAngle = -pi/4;
endAngle = pi/4;
radiusX = 100;
radiusY = 0;
The line should go from (sqrt(2)/2*100; 0) to (100; 0).
You draw just a single point of (sqrt(2)/2*100; 0).

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:213: lineTo(x + radiusX *
cosf(startAngle + rotation), y + radiusY * sinf(startAngle + rotation));
It still does not take into account axes rotation. How about something like
this:
float cos, sin;
sincosf(startAngle, &sin, &cos);
float x1 = radiusX * cos;
float y1 = radiusY * sin;
sincosf(rotation, &sin, &cos);
float x2 = x1 * cos - y1 * sin;
float y2 = x1 * sin + y1 * cos;
lineTo(x + x2, y + y2);

https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:215: degenerateEllipse(this, x, y,
radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise);
On 2013/07/09 15:48:02, dshwang wrote:
> @alph yes, you are right. I understand what you say.
> 
> How about this implementation?
> 
> Or if we want simpler implementation with losing some precision, below
solution
> can work.
> Replace below code to degenerateEllipse(...)
> if (!radiusX)
>     radiusX = 0.00001;
> if (!radiusY)
>     radiusY = 0.00001;
> 
> Which do you prefer?

degenerateEllipse already looks scary to me despite it still doesn't take into
account axes rotation. I don't like scary code as bugs might hide in it. ;-)

[dshwang, 2013-07-09 16:37:21]

Thx for kind explanation about rotation.
As I mentioned comment #18, I understand :)

And I still have two questions.

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:244: */
Which do you think right: degenerateEllipse() or approximation(radiusX = 0.0001)
?

And do you think degenerateEllipse() still looks scary?

[alph, 2013-07-09 17:18:17]

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:191: sweep = std::fmod(-sweep, 
piFloat * 2);
I'm not sure how to map negative sweep. Unfortunately the specification is not
clear on that. We should probably mimic the behavior of m_path.addEllipse
function.

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint);
It now looks much better. But unfortunately this stuff just won't work. See my
counter example to the previous patch.
How about this approach:
1. canonicalize startAngle and endAngle, so that
  0 <= startAngle < 2*pi
  0 <= endAngle - startAngle <= 2*pi

2. then run something like:
lineTo(<point-for-startAngle>);
for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle; angle
+= pi/2)
  lineTo(<point-for-angle>);
lineTo(<point-for-endAngle>);

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:244: */
On 2013/07/09 16:37:22, dshwang wrote:
> Which do you think right: degenerateEllipse() or approximation(radiusX =
0.0001)
> ?
> 
> And do you think degenerateEllipse() still looks scary?

This way degenerateEllipse looks much better. I'd go for it. ;-)

[dshwang, 2013-07-09 17:40:52]

Thx for review.

I'll make degenerateEllipse more succinct, and make
fast/canvas/canvas-ellipse-connecting-line.html cover more extensively.

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:191: sweep = std::fmod(-sweep, 
piFloat * 2);
On 2013/07/09 17:18:18, alph wrote:
> I'm not sure how to map negative sweep. Unfortunately the specification is not
> clear on that. We should probably mimic the behavior of m_path.addEllipse
> function.

The spec said
"
If the anticlockwise argument is false and endAngle-startAngle is equal to or
greater than 2π, or, if the anticlockwise argument is true and
startAngle-endAngle is equal to or greater than 2π, then the arc is the whole
circumference of this ellipse.

Otherwise, the arc is the path along the circumference of this ellipse from the
start point to the end point, going anti-clockwise if the anticlockwise argument
is true, and clockwise otherwise. Since the points are on the ellipse, as
opposed to being simply angles from zero, the arc can never cover an angle
greater than 2π radians.
"

adjustEndAngle() can handle sweep angle in the manner which the spec want. I'll
reuse adjustEndAngle().

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint);
On 2013/07/09 17:18:18, alph wrote:
> It now looks much better. But unfortunately this stuff just won't work. See my
> counter example to the previous patch.
> How about this approach:
> 1. canonicalize startAngle and endAngle, so that
>   0 <= startAngle < 2*pi
>   0 <= endAngle - startAngle <= 2*pi
> 
> 2. then run something like:
> lineTo(<point-for-startAngle>);
> for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle;
angle
> += pi/2)
>   lineTo(<point-for-angle>);
> lineTo(<point-for-endAngle>);

Good idea! I buy it. I'll change.

On the other hands, when it comes to your previous counter example.
This implementation is right :)

quote your example.
> Here's a counter example:
> rotation = x = y = 0;
> startAngle = -pi/4;
> endAngle = pi/4;
> radiusX = 100;
> radiusY = 0;
> The line should go from (sqrt(2)/2*100; 0) to (100; 0).
> You draw just a single point of (sqrt(2)/2*100; 0).

Good example, but the premise is inconsistent with the result.
you may want endAngle to be 0, not pi/4.

The amended example,
rotation = x = y = 0;
startAngle = -pi/4;
endAngle = 0;
radiusX = 100;
radiusY = 0;

The result is the line from (sqrt(2)/2*100; -sqrt(2)/2*100) to (100; 0).

This implementation will draw the line
from lineTo(path, center + rotationMatrix.mapPoint(ellipsePoint(radiusX,
radiusY, startAngle)));
to   lineTo(path, endPoint);
The result of the implementation equals to what we expect.

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:244: */
On 2013/07/09 17:18:18, alph wrote:
> On 2013/07/09 16:37:22, dshwang wrote:
> > Which do you think right: degenerateEllipse() or approximation(radiusX =
> 0.0001)
> > ?
> > 
> > And do you think degenerateEllipse() still looks scary?
> 
> This way degenerateEllipse looks much better. I'd go for it. ;-)

Good choice :)

[alph, 2013-07-09 18:16:10]

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint);
On 2013/07/09 17:40:53, dshwang wrote:
> On 2013/07/09 17:18:18, alph wrote:
> > It now looks much better. But unfortunately this stuff just won't work. See
my
> > counter example to the previous patch.
> > How about this approach:
> > 1. canonicalize startAngle and endAngle, so that
> >   0 <= startAngle < 2*pi
> >   0 <= endAngle - startAngle <= 2*pi
> > 
> > 2. then run something like:
> > lineTo(<point-for-startAngle>);
> > for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle;
> angle
> > += pi/2)
> >   lineTo(<point-for-angle>);
> > lineTo(<point-for-endAngle>);
> 
> Good idea! I buy it. I'll change.

Just though that it's probably better to have two loops. One for clockwise and
another for anticlockwise case. Otherwise if you exchange the start and end
points to make end > start, you'll have to make two additional moveTo's to real
start point in the beginning and to real end point at the end to not break the
path ordering.

> 
> On the other hands, when it comes to your previous counter example.
> This implementation is right :)
> 
> quote your example.
> > Here's a counter example:
> > rotation = x = y = 0;
> > startAngle = -pi/4;
> > endAngle = pi/4;
> > radiusX = 100;
> > radiusY = 0;
> > The line should go from (sqrt(2)/2*100; 0) to (100; 0).
> > You draw just a single point of (sqrt(2)/2*100; 0).
> 
> Good example, but the premise is inconsistent with the result.
> you may want endAngle to be 0, not pi/4.

I meant the line should go through points:
start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4
then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4]
and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4

So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0)
But your original code won't go through (100; 0). That's what I meant.

> The amended example,
> rotation = x = y = 0;
> startAngle = -pi/4;
> endAngle = 0;
> radiusX = 100;
> radiusY = 0;
> 
> The result is the line from (sqrt(2)/2*100; -sqrt(2)/2*100) to (100; 0).

Hmm, I don't get it. y of the first point cannot be non zero as long as radiusY
is 0. I.e. all the arc points have y=0.

> 
> This implementation will draw the line
> from lineTo(path, center + rotationMatrix.mapPoint(ellipsePoint(radiusX,
> radiusY, startAngle)));
> to   lineTo(path, endPoint);
> The result of the implementation equals to what we expect.

[dshwang, 2013-07-10 05:29:54]

On 2013/07/09 18:16:10, alph wrote:
> I meant the line should go through points:
> start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4
> then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4]
> and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4
> 
> So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0)
> But your original code won't go through (100; 0). That's what I meant.

> 
> Hmm, I don't get it. y of the first point cannot be non zero as long as
radiusY
> is 0. I.e. all the arc points have y=0.

Hi, now I totally understand. Thx for your endurance making me understand :)

I found that arc has a bug while I followed your instruction about ellipse.
Could you review Issue 18286007 before reviewing this CL?

[dshwang, 2013-07-10 13:03:20]

On 2013/07/09 18:16:10, alph wrote:
> I meant the line should go through points:
> start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4
> then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4]
> and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4
> 
> So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0)
> But your original code won't go through (100; 0). That's what I meant.
> 

Hi, I seem to complete degenerateEllipse implementation. The impl pass your
example. And I improve canvas-ellipse-zero-lineto.html to cover various edge
cases of degenerateEllipse.

Also I add new test: fast/canvas/canvas-ellipse-circumference.html that is
similar to canvas-arc-circumference.html that I added in
http://crrev.com/18286007/

This CL depends on crrev.com/18286007/

[alph, 2013-07-10 13:38:03]

Thanks for addressing my concerns.

https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right):

https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore();
Again, as for the arc test, I suggest drawing a 2d array of arcs with various
start and end angles.

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:223:
canonicalizeAngle(&startAngle, &endAngle);
It seems that canonicalizeAngle should depend on anticlockwise, as endAngle
should be more than startAngle if !anticlockwise, and opposite otherwise.

Also I suggest to just inline it here and get rid of taking address operations
on startAngle, endAngle, as they might prohibit some compiler optimizations.

[dshwang, 2013-07-10 14:19:56]

Thx for review, alph :)

https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right):

https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore();
On 2013/07/10 13:38:04, alph wrote:
> Again, as for the arc test, I suggest drawing a 2d array of arcs with various
> start and end angles.

Yes, I think your idea is great.

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:223:
canonicalizeAngle(&startAngle, &endAngle);
On 2013/07/10 13:38:04, alph wrote:
> It seems that canonicalizeAngle should depend on anticlockwise, as endAngle
> should be more than startAngle if !anticlockwise, and opposite otherwise.
> 
> Also I suggest to just inline it here and get rid of taking address operations
> on startAngle, endAngle, as they might prohibit some compiler optimizations.

canonicalizeAngle() is independent from whether anticlockwise or not.
canonicalizeAngle just translates pair(startAngle, endAngle) so that 0 <=
startAngle < 2*PI.

adjustEndAngle() guarantees relevant startAngle and endAngle, and
adjustEndAngle() works before this routine.

I respect your optimization concern, but imho it seems to be premature
optimization.
degenerateEllipse() would be rarely called, so I prefer a bit more readability.

[dshwang, 2013-07-10 14:28:43]

On 2013/07/10 14:19:56, dshwang wrote:
>
https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c...
> LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore();
> On 2013/07/10 13:38:04, alph wrote:
> > Again, as for the arc test, I suggest drawing a 2d array of arcs with
various
> > start and end angles.
> 
> Yes, I think your idea is great.

@alph, do you know good reference tests that I can follow. I don't want to
reinvent wheel :)

[alph, 2013-07-10 15:22:29]

> @alph, do you know good reference tests that I can follow. I don't want to
reinvent wheel :)

No, I don't know.

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:223:
canonicalizeAngle(&startAngle, &endAngle);
On 2013/07/10 14:19:57, dshwang wrote:
> On 2013/07/10 13:38:04, alph wrote:
> > It seems that canonicalizeAngle should depend on anticlockwise, as endAngle
> > should be more than startAngle if !anticlockwise, and opposite otherwise.
> > 
> > Also I suggest to just inline it here and get rid of taking address
operations
> > on startAngle, endAngle, as they might prohibit some compiler optimizations.
> 
> canonicalizeAngle() is independent from whether anticlockwise or not.
> canonicalizeAngle just translates pair(startAngle, endAngle) so that 0 <=
> startAngle < 2*PI.

Sorry, missed that.
 
> adjustEndAngle() guarantees relevant startAngle and endAngle, and
> adjustEndAngle() works before this routine.
> 
> I respect your optimization concern, but imho it seems to be premature
> optimization.
> degenerateEllipse() would be rarely called, so I prefer a bit more
readability.

[dshwang, 2013-08-25 15:57:53]

@senorblanco Could you have a chance to look at this CL?

[Stephen White, 2013-08-27 14:07:36]

I appreciate the extensive testing, but I wouldn't bother duplicating
arc360.html (ellipse360) or arc-crash.html. The former is probably subsumed by
canvas-arc-circumference.html (so canvas-ellipse-circumference in your new
patch), and the latter was a regression test for a crash in the old arc code.
Both are likely redundant with tests you already have.

[Stephen White, 2013-08-27 14:23:49]

https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro...
File LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html (right):

https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro...
LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html:49: "ellipse",
Rather than just adding this, we should mark this test as expected to fail in
TestExpectations. I believe this test is here to let the devtools folks know
they have to do something on their side; see the text below: "If this test
should ever fail, we should re-examine the Canvas 2D state saving/restoring
logic in the InjectedScriptModule to include any latest changes to the API."

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle
- fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle +=
halfPiFloat)
I'm a little confused by this loop (and the one below). Isn't a degenerate
ellipse a single line? Perhaps adding a comment (and/or a reference to the spec,
although I couldn't find this algorithm there) might help.

[Justin Novosad, 2013-08-27 17:51:01]

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle
- fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle +=
halfPiFloat)
On 2013/08/27 14:23:50, Stephen White wrote:
> I'm a little confused by this loop (and the one below). Isn't a degenerate
> ellipse a single line? Perhaps adding a comment (and/or a reference to the
spec,
> although I couldn't find this algorithm there) might help.
I think the idea to to generate a flat quad to draw said line? 
Definitely needs more explaining.  Also I am not 100% convinced this does the
right thing in all cases

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:251: if (!isTransformInvertible())
This early exit is not in the spec.  If the implementation can't handle
non-invertible transforms, then that should be fixed.  Transforming an
elliptical arc by a non-invertible transform is perfectly tractable.  It gives
an area of zero, so ignoring it is fine when the path is filled. When stroked
however, the ellipse should be rendered as a line.

[dshwang, 2013-08-27 18:45:25]

Thanks for review! I submitted Patch Set 8 that applies your review!

On 2013/08/27 14:07:36, Stephen White wrote:
> I appreciate the extensive testing, but I wouldn't bother duplicating
> arc360.html (ellipse360) or arc-crash.html. The former is probably subsumed by
> canvas-arc-circumference.html (so canvas-ellipse-circumference in your new
> patch), and the latter was a regression test for a crash in the old arc code.
> Both are likely redundant with tests you already have.

Good points! I removes ellipse360 and ellipse-crash.

On 2013/08/27 14:23:49, Stephen White wrote:
>
https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro...
> File LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html
(right):
> 
>
https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro...
> LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html:49:
"ellipse",
> Rather than just adding this, we should mark this test as expected to fail in
> TestExpectations. I believe this test is here to let the devtools folks know
> they have to do something on their side; see the text below: "If this test
> should ever fail, we should re-examine the Canvas 2D state saving/restoring
> logic in the InjectedScriptModule to include any latest changes to the API."

Good to know. Done.

>
https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
> File Source/core/html/canvas/CanvasPathMethods.cpp (right):
> 
>
https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle =
startAngle
> - fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle +=
> halfPiFloat)
> I'm a little confused by this loop (and the one below). Isn't a degenerate
> ellipse a single line? Perhaps adding a comment (and/or a reference to the
spec,
> although I couldn't find this algorithm there) might help.

I added detail comments for degenerateEllipse().

[dshwang, 2013-08-27 18:54:16]

Thx for your review!

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle
- fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle +=
halfPiFloat)
On 2013/08/27 17:51:03, junov wrote:
> On 2013/08/27 14:23:50, Stephen White wrote:
> > I'm a little confused by this loop (and the one below). Isn't a degenerate
> > ellipse a single line? Perhaps adding a comment (and/or a reference to the
> spec,
> > although I couldn't find this algorithm there) might help.
> I think the idea to to generate a flat quad to draw said line? 
> Definitely needs more explaining.  Also I am not 100% convinced this does the
> right thing in all cases

I added detail explanations as comments.
To verify this implementation, I made
fast/canvas/canvas-ellipse-connecting-line.html that covers extensive cases.

And @alph reviewed it very carefully.

https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:251: if (!isTransformInvertible())
On 2013/08/27 17:51:03, junov wrote:
> This early exit is not in the spec.  If the implementation can't handle
> non-invertible transforms, then that should be fixed.  Transforming an
> elliptical arc by a non-invertible transform is perfectly tractable.  It gives
> an area of zero, so ignoring it is fine when the path is filled. When stroked
> however, the ellipse should be rendered as a line.

Good concern.
However, all primitives early exit as well as ellipse: moveTo, lineTo,
quadraticCurveTo, bezierCurveTo, arcTo, arc, rect.

I also can not find the spec about invertible current transform.

If ellipse has problem, all primitives have also problem. We need to handle them
together in the future if you are right.

[Justin Novosad, 2013-08-27 22:45:19]

On 2013/08/27 18:54:16, dshwang wrote:

> Good concern.
> However, all primitives early exit as well as ellipse: moveTo, lineTo,
> quadraticCurveTo, bezierCurveTo, arcTo, arc, rect.
> 
> I also can not find the spec about invertible current transform.
> 
> If ellipse has problem, all primitives have also problem. We need to handle
them
> together in the future if you are right.

Ok, I buy that.  I did a search and found that the issue was raised on WhatWG:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-July/040185.html
Let's see where that goes...

Nice ASCII art in the comments by the way!

I'm not a Blink owner, but for what it's worth: lgtm for the Source/core
changes.

[Stephen White, 2013-08-28 01:05:32]

LGTM. Thanks!

[commit-bot: I haz the power, 2013-08-28 05:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/64001

[dshwang, 2013-08-28 05:40:56]

Thx Stephen, junov and alph for your sincere review! :D

[commit-bot: I haz the power, 2013-08-28 06:25:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/78001

[dshwang, 2013-08-28 06:26:20]

canvas-ellipse-360-winding.html failed in virtual/gpu test because the test is
html ref test and some pixels is different.
I changed the test to text ref test like canvas-arc-360-winding.html.

Patch Set 9 for landing.

[commit-bot: I haz the power, 2013-08-28 07:59:55]

Change committed as 156828