Fix edge case bugs of canvas arc.

Fix edge case bugs of canvas arc.

The canvas spec [1] defines how to draw the circumference of arc with various
conditions of startAngle, engAngle and anticlockwise. Current implementation
does not satisfy the spec.

[1] http://www.w3.org/TR/2dcontext2/

TEST=fast/canvas/canvas-arc-circumference.html
BUG=130260
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156647

[dshwang, 2013-07-10 05:26:26]

It is a prerequisite of CL 14298022.
Could you review?

[dshwang, 2013-07-10 11:42:27]

On 2013/07/10 05:26:26, dshwang wrote:
> It is a prerequisite of CL 14298022.
> Could you review?

Ref test that I made failed in mac and win. I give up ref test. I'll update the
test using ctx.getImageData.

I also need to investigate why 2d.path.arc.angle.3.html and
2d.path.arc.angle.5.html are failed

[dshwang, 2013-07-10 12:17:29]

On 2013/07/10 11:42:27, dshwang wrote:
> On 2013/07/10 05:26:26, dshwang wrote:
> > It is a prerequisite of CL 14298022.
> > Could you review?
> 
> Ref test that I made failed in mac and win. I give up ref test. I'll update
the
> test using ctx.getImageData.
> 
> I also need to investigate why 2d.path.arc.angle.3.html and
> 2d.path.arc.angle.5.html are failed

Currently 2d.path.arc.angle.3.html and 2d.path.arc.angle.5.html fail in Blink,
so expected results are wrong.
This patch makes both pass!! :)

In addition, I change my test from pixel test to text based test.

[alph, 2013-07-10 13:06:29]

lgtm w/ comments.
Dongseong, you should add another reviewer to the patch as I'm not an owner of
this stuff.

https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):

https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
LayoutTests/fast/canvas/canvas-arc-circumference.html:25: ctx.arc(60, 50, 30,
-Math.PI, Math.PI, false);
How about drawing e.g. 100 arcs in a loop to form 10 by 10 matrix, where rows
correspond to various start angles and columns correspond to various end angles.
So the main diagonal should have arcs degenerated to just dots, as the angles
are equal, etc.
The angles list should include values that produce various interesting corner
cases of sweeps, i.e. -4pi, -2pi, -pi/2, 0, p2/2, 2*pi, 0.000001, etc.
For each matrix element you can draw two concentric arcs: an anticlockwise with
radius of e.g. 20 and a clockwise with radius e.g. 30. Then the resulting bitmap
could be compared to an expectation image.

That approach should require less coding and cover more corner cases. wdyt?

https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
Source/core/html/canvas/CanvasPathMethods.cpp:153: return startAngle + (twoPi -
fmodf(startAngle - endAngle, twoPi));
I might think of a rare corner case when e.g.
anticlockwise = false;
startAngle = twoPi;
endAngle = 0;
One would expect it to degenerate to a single dot, as both start and end angles
point to the same location of the circle. Your code draws the entire circle.
This is however is a kind of bifurcation point, so if you decrease startEngle a
bit it should really draw the (almost) entire circle. Though I'm not sure if
it's worth fixing it.

[dshwang, 2013-07-10 13:36:42]

Thx for valuable review!

https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):

https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
LayoutTests/fast/canvas/canvas-arc-circumference.html:25: ctx.arc(60, 50, 30,
-Math.PI, Math.PI, false);
On 2013/07/10 13:06:29, alph wrote:
> How about drawing e.g. 100 arcs in a loop to form 10 by 10 matrix, where rows
> correspond to various start angles and columns correspond to various end
angles.
> So the main diagonal should have arcs degenerated to just dots, as the angles
> are equal, etc.
> The angles list should include values that produce various interesting corner
> cases of sweeps, i.e. -4pi, -2pi, -pi/2, 0, p2/2, 2*pi, 0.000001, etc.
> For each matrix element you can draw two concentric arcs: an anticlockwise
with
> radius of e.g. 20 and a clockwise with radius e.g. 30. Then the resulting
bitmap
> could be compared to an expectation image.
> 
> That approach should require less coding and cover more corner cases. wdyt?

good idea! I'll do it. I hope that making bitmaps is less painful than WebKit...

https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
File Source/core/html/canvas/CanvasPathMethods.cpp (right):

https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
Source/core/html/canvas/CanvasPathMethods.cpp:153: return startAngle + (twoPi -
fmodf(startAngle - endAngle, twoPi));
On 2013/07/10 13:06:29, alph wrote:
> I might think of a rare corner case when e.g.
> anticlockwise = false;
> startAngle = twoPi;
> endAngle = 0;
> One would expect it to degenerate to a single dot, as both start and end
angles
> point to the same location of the circle. Your code draws the entire circle.
> This is however is a kind of bifurcation point, so if you decrease startEngle
a
> bit it should really draw the (almost) entire circle. Though I'm not sure if
> it's worth fixing it. 

Wow, you have eagle eye. If you want to follow the spec completely, we should
fix. So I'll fix. :)

[dshwang, 2013-07-10 14:07:44]

On 2013/07/10 13:36:42, dshwang wrote:
> Thx for valuable review!
> 
>
https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
> File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):
> 
>
https://codereview.chromium.org/18286007/diff/6001/LayoutTests/fast/canvas/ca...
> LayoutTests/fast/canvas/canvas-arc-circumference.html:25: ctx.arc(60, 50, 30,
> -Math.PI, Math.PI, false);
> On 2013/07/10 13:06:29, alph wrote:
> > How about drawing e.g. 100 arcs in a loop to form 10 by 10 matrix, where
rows
> > correspond to various start angles and columns correspond to various end
> angles.
> > So the main diagonal should have arcs degenerated to just dots, as the
angles
> > are equal, etc.
> > The angles list should include values that produce various interesting
corner
> > cases of sweeps, i.e. -4pi, -2pi, -pi/2, 0, p2/2, 2*pi, 0.000001, etc.
> > For each matrix element you can draw two concentric arcs: an anticlockwise
> with
> > radius of e.g. 20 and a clockwise with radius e.g. 30. Then the resulting
> bitmap
> > could be compared to an expectation image.
> > 
> > That approach should require less coding and cover more corner cases. wdyt?
> 
> good idea! I'll do it. I hope that making bitmaps is less painful than
WebKit...

Before I start to make new test based on pixel test, I want to hear other
Owners' opinion.
^ Stephen Chenney, Stephen White, how do you think?

It is because new test has pros and cons.
pros: cover various case.
cons: pixel test.

>
https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
> File Source/core/html/canvas/CanvasPathMethods.cpp (right):
> 
>
https://codereview.chromium.org/18286007/diff/6001/Source/core/html/canvas/Ca...
> Source/core/html/canvas/CanvasPathMethods.cpp:153: return startAngle + (twoPi
-
> fmodf(startAngle - endAngle, twoPi));
> On 2013/07/10 13:06:29, alph wrote:
> > I might think of a rare corner case when e.g.
> > anticlockwise = false;
> > startAngle = twoPi;
> > endAngle = 0;
> > One would expect it to degenerate to a single dot, as both start and end
> angles
> > point to the same location of the circle. Your code draws the entire circle.
> > This is however is a kind of bifurcation point, so if you decrease
startEngle
> a
> > bit it should really draw the (almost) entire circle. Though I'm not sure if
> > it's worth fixing it. 
> 
> Wow, you have eagle eye. If you want to follow the spec completely, we should
> fix. So I'll fix. :)

After rethinking, I'll not change the code, although it violates the spec.
It is because that many web developers seems to use ctx.arc(x, y, r, 0,
2*Math.PI, true) to draw circle.
For example, philips also made some mistakes.
2d.line.join.round.html:30:ctx.arc(30, 20, 10-tol, 0, 2*Math.PI, true);
2d.line.join.round.html:53:ctx.arc(80, 20, 10+tol, 0, 2*Math.PI, true);
2d.path.arc.empty.html:20:ctx.arc(200, 25, 5, 0, 2*Math.PI, true);
2d.path.arc.nonempty.html:21:ctx.arc(200, 25, 5, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:19:ctx.arc(Infinity, 0, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:20:ctx.arc(-Infinity, 0, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:21:ctx.arc(NaN, 0, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:22:ctx.arc(0, Infinity, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:23:ctx.arc(0, -Infinity, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:24:ctx.arc(0, NaN, 50, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:25:ctx.arc(0, 0, Infinity, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:26:ctx.arc(0, 0, -Infinity, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:27:ctx.arc(0, 0, NaN, 0, 2*Math.PI, true);
2d.path.arc.nonfinite.html:34:ctx.arc(Infinity, Infinity, 50, 0, 2*Math.PI,
true);
2d.path.arc.nonfinite.html:35:ctx.arc(Infinity, Infinity, Infinity, 0,
2*Math.PI, true);
2d.path.arc.nonfinite.html:42:ctx.arc(Infinity, 0, Infinity, 0, 2*Math.PI,
true);
2d.path.arc.nonfinite.html:49:ctx.arc(0, Infinity, Infinity, 0, 2*Math.PI,
true);

If I satisfy the spec, bunch of sites will be broken..

[dshwang, 2013-07-10 16:53:01]

Hi, could you review?

https://codereview.chromium.org/18286007/diff/16001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):

https://codereview.chromium.org/18286007/diff/16001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-arc-circumference.html:30: 
Make canvas-arc-circumference.html cover 160 cases as alph suggested.

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

https://codereview.chromium.org/18286007/diff/16001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:156: */
Add comment for alph's edge case.

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

Stephen, could you have a chance to look at this CL?

[Stephen White, 2013-08-22 15:08:29]

https://codereview.chromium.org/18286007/diff/16001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):

https://codereview.chromium.org/18286007/diff/16001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-arc-circumference.html:30: 
Please add a line to LayoutTests/TestExpectations to prevent the bots going red
on this new test; something like:

crbug.com/130260 fast/canvas/canvas-arc-circumference.html [ NeedsRebaseline ]

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

https://codereview.chromium.org/18286007/diff/16001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasPathMethods.cpp:138: * If the anticlockwise
argument is false and endAngle-startAngle is equal to or greater than 2π, or,
I'm not sure if unicode characters in C++ source are kosher. A quick search
doesn't tell me either way, but since this is only a comment, perhaps we should
just use "pi" to be on the safe side.

[dshwang, 2013-08-22 19:13:20]

Thx for your review

On 2013/08/22 15:08:29, Stephen White wrote:
>
https://codereview.chromium.org/18286007/diff/16001/LayoutTests/fast/canvas/c...
> File LayoutTests/fast/canvas/canvas-arc-circumference.html (right):
>
> crbug.com/130260 fast/canvas/canvas-arc-circumference.html [ NeedsRebaseline ]

It's interesting. done.

>
https://codereview.chromium.org/18286007/diff/16001/Source/core/html/canvas/C...
> File Source/core/html/canvas/CanvasPathMethods.cpp (right):
> 
>
https://codereview.chromium.org/18286007/diff/16001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasPathMethods.cpp:138: * If the anticlockwise
> argument is false and endAngle-startAngle is equal to or greater than 2π, or,
> I'm not sure if unicode characters in C++ source are kosher. A quick search
> doesn't tell me either way, but since this is only a comment, perhaps we
should
> just use "pi" to be on the safe side.

ok, I used pi.

[dshwang, 2013-08-23 14:50:22]

Hi, could you review again?

[Stephen White, 2013-08-23 15:08:38]

On 2013/08/23 14:50:22, dshwang wrote:
> Hi, could you review again?

Could you re-upload the patch without the PNG file, so the trybots can test it? 
Thanks!

[Stephen White, 2013-08-23 15:09:33]

On 2013/08/23 15:08:38, Stephen White wrote:
> On 2013/08/23 14:50:22, dshwang wrote:
> > Hi, could you review again?
> 
> Could you re-upload the patch without the PNG file, so the trybots can test
it? 
> Thanks!

Ahh, I see you did, thanks.

[dshwang, 2013-08-23 15:10:36]

On 2013/08/23 15:08:38, Stephen White wrote:
> On 2013/08/23 14:50:22, dshwang wrote:
> > Hi, could you review again?
> 
> Could you re-upload the patch without the PNG file, so the trybots can test
it? 
> Thanks!

Yeah, I re-uploaded without png file.
bot complained that Binary file support is temporarilly disabled due to a bug.
Please commit blindly the binary files first then commit the source change as a
separate CL. Sorry for the annoyance.

[dshwang, 2013-08-23 15:12:07]

If you think this CL is fine, could you give me lgtm?
Thanks!

[Stephen White, 2013-08-23 15:12:44]

LGTM; let's see what the trybots say.

[Stephen White, 2013-08-23 15:19:58]

Note: both Firefox and IE give different results for the last 5 or so images in
each row of your canvas-arc-circumference test.

[Stephen White, 2013-08-23 15:34:24]

Looks like you'll need to add a TestExpectations line for the virtual/gpu
flavour of your test:

crbug.com/130260 virtual/gpu/fast/canvas/canvas-arc-circumference.html [
NeedsRebaseline ]

(This is a "virtual" copy of your test, as run on the accelerated path.)

[dshwang, 2013-08-23 16:03:40]

On 2013/08/23 15:19:58, Stephen White wrote:
> Note: both Firefox and IE give different results for the last 5 or so images
in
> each row of your canvas-arc-circumference test.

In this CL, I changed the end point is on the point of end angle, although
endAngle-startAngle is greater than 2pi.
When endAngle-startAngle is greater than 2pi, Firefox and IE consider the end
point as the start point. I think it's wrong.
How do you think?

The spec just said as follows:
http://www.w3.org/TR/2dcontext2/#dom-context-2d-arc
The points at startAngle and endAngle along this circle's circumference,
measured in radians clockwise from the ellipse's semi-major axis, are the start
and end points respectively.

I can report this issue to FF and IE.

[dshwang, 2013-08-23 16:04:04]

On 2013/08/23 15:34:24, Stephen White wrote:
> Looks like you'll need to add a TestExpectations line for the virtual/gpu
> flavour of your test:
> 
> crbug.com/130260 virtual/gpu/fast/canvas/canvas-arc-circumference.html [
> NeedsRebaseline ]
> 
> (This is a "virtual" copy of your test, as run on the accelerated path.)

Yes, I'll add. Thx.

[Stephen White, 2013-08-23 16:37:10]

On 2013/08/23 16:03:40, dshwang wrote:
> On 2013/08/23 15:19:58, Stephen White wrote:
> > Note: both Firefox and IE give different results for the last 5 or so images
> in
> > each row of your canvas-arc-circumference test.
> 
> In this CL, I changed the end point is on the point of end angle, although
> endAngle-startAngle is greater than 2pi.
> When endAngle-startAngle is greater than 2pi, Firefox and IE consider the end
> point as the start point. I think it's wrong.
> How do you think?

Hmmm. I guess you could argue that "If the anticlockwise argument false and
endAngle-startAngle is equal to or greater than 2π [...] then the arc is the
whole circumference of this ellipse." means that the start point and end point
of the ellipse must be the same, since otherwise you either have to:

1) Continue the path from the start to the end point (again), possibly causing
double-rasterization, or
2) Add a moveTo() to skip to the end point.

Solution 1) might cause darkening of paths with alpha on the double-rasterized
region, and solution 2) might cause strange behaviour in the end caps for wide
lines, depending on the mitering style.  If I read correctly, your code does
solution 1? If so, we should check its behaviour for lines with alpha.

> The spec just said as follows:
> http://www.w3.org/TR/2dcontext2/#dom-context-2d-arc
> The points at startAngle and endAngle along this circle's circumference,
> measured in radians clockwise from the ellipse's semi-major axis, are the
start
> and end points respectively.
> 
> I can report this issue to FF and IE.

[dshwang, 2013-08-23 16:57:21]

On 2013/08/23 16:37:10, Stephen White wrote:
> On 2013/08/23 16:03:40, dshwang wrote:
> > On 2013/08/23 15:19:58, Stephen White wrote:
> > > Note: both Firefox and IE give different results for the last 5 or so
images
> > in
> > > each row of your canvas-arc-circumference test.
> > 
> > In this CL, I changed the end point is on the point of end angle, although
> > endAngle-startAngle is greater than 2pi.
> > When endAngle-startAngle is greater than 2pi, Firefox and IE consider the
end
> > point as the start point. I think it's wrong.
> > How do you think?
> 
> Hmmm. I guess you could argue that "If the anticlockwise argument false and
> endAngle-startAngle is equal to or greater than 2π [...] then the arc is the
> whole circumference of this ellipse." means that the start point and end point
> of the ellipse must be the same, since otherwise you either have to:
> 
> 1) Continue the path from the start to the end point (again), possibly causing
> double-rasterization, or
> 2) Add a moveTo() to skip to the end point.
> 
> Solution 1) might cause darkening of paths with alpha on the double-rasterized
> region, and solution 2) might cause strange behaviour in the end caps for wide
> lines, depending on the mitering style.  If I read correctly, your code does
> solution 1? If so, we should check its behaviour for lines with alpha.

I chose option 1.
Fortunately, darkening of paths with alpha does not happen because skia handles
it well.

I think we have two option.
1) change CL consistent with FF and IE. and argue the end point issue in W3C
bugzilla.

2) live with this CL. and argue the end point issue in W3C bugzilla.

I assume you want option 1), and I can change CL quickly.

[Stephen White, 2013-08-23 17:11:44]

On 2013/08/23 16:57:21, dshwang wrote:
> On 2013/08/23 16:37:10, Stephen White wrote:
> > On 2013/08/23 16:03:40, dshwang wrote:
> > > On 2013/08/23 15:19:58, Stephen White wrote:
> > > > Note: both Firefox and IE give different results for the last 5 or so
> images
> > > in
> > > > each row of your canvas-arc-circumference test.
> > > 
> > > In this CL, I changed the end point is on the point of end angle, although
> > > endAngle-startAngle is greater than 2pi.
> > > When endAngle-startAngle is greater than 2pi, Firefox and IE consider the
> end
> > > point as the start point. I think it's wrong.
> > > How do you think?
> > 
> > Hmmm. I guess you could argue that "If the anticlockwise argument false and
> > endAngle-startAngle is equal to or greater than 2π [...] then the arc is the
> > whole circumference of this ellipse." means that the start point and end
point
> > of the ellipse must be the same, since otherwise you either have to:
> > 
> > 1) Continue the path from the start to the end point (again), possibly
causing
> > double-rasterization, or
> > 2) Add a moveTo() to skip to the end point.
> > 
> > Solution 1) might cause darkening of paths with alpha on the
double-rasterized
> > region, and solution 2) might cause strange behaviour in the end caps for
wide
> > lines, depending on the mitering style.  If I read correctly, your code does
> > solution 1? If so, we should check its behaviour for lines with alpha.
> 
> I chose option 1.
> Fortunately, darkening of paths with alpha does not happen because skia
handles
> it well.
> 
> I think we have two option.
> 1) change CL consistent with FF and IE. and argue the end point issue in W3C
> bugzilla.
> 
> 2) live with this CL. and argue the end point issue in W3C bugzilla.
> 
> I assume you want option 1), and I can change CL quickly.

Actually, I'm fine with keeping it as-is, if Skia takes care of the overdraw. I
think it's more correct your way. But it would be good to send a note to whatwg
to clarify this case.

[dshwang, 2013-08-23 17:24:15]

On 2013/08/23 17:11:44, Stephen White wrote:
> Actually, I'm fine with keeping it as-is, if Skia takes care of the overdraw.
I
> think it's more correct your way. But it would be good to send a note to
whatwg
> to clarify this case.

Ok, I'll land this cl after adding strokeStyle="rgba(255,255,255,0.5)" to the
test to verify future skia regression.

I'll send a note to whatwg, FF, and IE also.

[commit-bot: I haz the power, 2013-08-23 17:29:40]

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

[dshwang, 2013-08-23 17:47:51]

I filed the end point issue in w3c bugzilla.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23050

[commit-bot: I haz the power, 2013-08-23 18:10:46]

Change committed as 156647