Add support for new canvas ellipse method

• Component: Canvas
• Status: RESOLVED FIXED

[Dean Jackson]

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

[Radar WebKit Bug Importer]

<rdar://problem/11159172>

[Ian 'Hixie' Hickson]

arcTo() is also expanded to do ellipsoid corners.

[Dongseong Hwang]

Created attachment 150303 [details]
patch

[Dongseong Hwang]

Created attachment 150304 [details]
Test: ellipse does not cause crash.

[Dongseong Hwang]

Comment on attachment 150303 [details]
patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:142
> +    void arc(float x, float y, float r, float sa, float ea, bool anticlockwise, ExceptionCode&);

anticlockwise is right. It is not related to this patch, but it is tiny refactoring.

> Source/WebCore/platform/graphics/Path.cpp:106
> +    }

This is copied from CanvasRenderingContext2D::arc.
I think it is right policy of edge case, but I want to know the opinion of experts of GraphicsContext.

[Dongseong Hwang]

I added 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.

However, Path::ellipse is little bit slow because it uses Path::transform twice.
Path::ellipse is platform independent.

Each platform can implement fast platform-dependent implementation.

[Simon Fraser (smfr)]

Why is the "ellipse does not crash" test in a separate patch?

[Dongseong Hwang]

(In reply to comment #7)
> Why is the "ellipse does not crash" test in a separate patch?

I have no reason. I did follow Bug 80674.
If merging is better, I'll do it.

[Dongseong Hwang]

I'll very appreciate if Dean or Simon take a look at this bug.

[Tim Horton]

Comment on attachment 150303 [details]
patch

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

> LayoutTests/ChangeLog:6
> +        [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse

This footnote notation is a bit unusual.

> LayoutTests/fast/canvas/script-tests/canvas-ellipse.js:10
> +  return x * 3.141592653589 / 180;

You should probably at least use Math.PI here.

> Source/WebCore/ChangeLog:6
> +        [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse

Again with the footnote notation.

> Source/WebCore/ChangeLog:16
> +        However, Path::ellipse is little bit slow because it uses Path::transform twice.
> +        Path::ellipse is platform independent.
> +        Each platform can implement fast platform-dependent implementation.

I don't think this commentary belongs in the changelog.

> Source/WebCore/ChangeLog:32
> +        * html/canvas/CanvasRenderingContext2D.cpp:
> +        (WebCore::CanvasRenderingContext2D::ellipse):
> +        (WebCore):
> +        * html/canvas/CanvasRenderingContext2D.h:
> +        (CanvasRenderingContext2D):
> +        * html/canvas/CanvasRenderingContext2D.idl:
> +        * platform/graphics/Path.cpp:
> +        (WebCore::Path::addEllipse):
> +        (WebCore):
> +        * platform/graphics/Path.h:
> +        (Path):

Comments per-function here might be nice.

> Source/WebCore/platform/graphics/Path.cpp:112
> +    const int unit = 1;

What is this variable for? The argument you're passing it to is a float, so this should be too. Also, perhaps a name like unitCircleRadius? I was quite confused what was going on here. It also seems a bit odd to create a variable here for no reason, but I guess it's for clarity?

[Dongseong Hwang]

Comment on attachment 150303 [details]
patch

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

>> LayoutTests/ChangeLog:6
>> +        [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse
> 
> This footnote notation is a bit unusual.

Thank you for reviewing.
Ok, I'll remove it.

>> LayoutTests/fast/canvas/script-tests/canvas-ellipse.js:10
>> +  return x * 3.141592653589 / 180;
> 
> You should probably at least use Math.PI here.

Ok.

>> Source/WebCore/ChangeLog:32
>> +        (Path):
> 
> Comments per-function here might be nice.

Ok.

>> Source/WebCore/platform/graphics/Path.cpp:112
>> +    const int unit = 1;
> 
> What is this variable for? The argument you're passing it to is a float, so this should be too. Also, perhaps a name like unitCircleRadius? I was quite confused what was going on here. It also seems a bit odd to create a variable here for no reason, but I guess it's for clarity?

Yes, just for clarity. I'll change to unitCircleRadius.

[Dongseong Hwang]

Created attachment 152414 [details]
Patch

[Early Warning System Bot]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13235405

[Early Warning System Bot]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13241372

[WebKit Review Bot]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13236417

[Build Bot]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13235406

[Gustavo Noronha (kov)]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13240390

[Gyuyoung Kim]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13245315

[Build Bot]

Comment on attachment 152414 [details]
Patch

Attachment 152414 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13230529

[Dongseong Hwang]

Created attachment 152438 [details]
patch v.4

Make up my mistake about build failure.

[Dirk Schulze]

Comment on attachment 152438 [details]
patch v.4

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

The patch looks good to me. It won't apply to trunk so. Please update the patch for a new review.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:925
> +void CanvasRenderingContext2D::ellipse(float x, float y, float rX, float rY, float rot, float sa, float ea, bool anticlockwise, ExceptionCode& ec)

just rx and ry. No capitalized letters.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'

Do we do the same on arc? We shouldn't differ here.

[Dongseong Hwang]

Created attachment 188791 [details]
Patch

[Dongseong Hwang]

Thank you for review!

(In reply to comment #21)
> (From update of attachment 152438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152438&action=review
> just rx and ry. No capitalized letters.
Done.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945
> > +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'
> 
> Do we do the same on arc? We shouldn't differ here.
Extract new function: adjustEndAngle().

[WebKit Review Bot]

Comment on attachment 188791 [details]
Patch

Attachment 188791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16617001

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[Build Bot]

Comment on attachment 188791 [details]
Patch

Attachment 188791 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16608301

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[Dongseong Hwang]

Created attachment 188985 [details]
Patch

[Dongseong Hwang]

(In reply to comment #25)
> (From update of attachment 188791 [details])
> Attachment 188791 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16608301
> 
> New failing tests:
> inspector/profiler/canvas2d/canvas2d-api-changes.html

Make the test pass

[EFL EWS Bot]

Comment on attachment 188985 [details]
Patch

Attachment 188985 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16612546

[Early Warning System Bot]

Comment on attachment 188985 [details]
Patch

Attachment 188985 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16629235

[Early Warning System Bot]

Comment on attachment 188985 [details]
Patch

Attachment 188985 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16610600

[WebKit Review Bot]

Comment on attachment 188985 [details]
Patch

Attachment 188985 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16610601

[WebKit Review Bot]

Comment on attachment 188985 [details]
Patch

Attachment 188985 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16613507

[Dongseong Hwang]

Created attachment 189028 [details]
Patch

[Rik Cabanier]

(In reply to comment #33)
> Created an attachment (id=189028) [details]
> Patch

Let's wait on this until we can sort this API out.
As currently defined, the path is not closed and it's annoying that you have to pass in all the parameters to draw a simple circle.

[Dirk Schulze]

Comment on attachment 189028 [details]
Patch

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

This particular feature is still under discussion on the mailing lists. I even doubt that it is that important for web authors at all. However, you do not even follow the specification by making all values optional. I would like to ask you to send a mail to webkit-dev and start a discussion there.

> Source/WebCore/ChangeLog:1
> [phone]  Huang Dongsung  <[e-mail]>

Is it Hwang or Huang? Are you doing this for company100 or intel?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:130
>          m_path.addArcTo(p1, p2, r);

You may want to add your copyright.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:136
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'.
> +    if (anticlockwise && sa - ea >= 2 * piFloat)

This seems to change the behavior of arc as well. Do you have tests for arc?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:163
> +    float adjustedEndAngle = adjustEndAngle(sa, ea, anticlockwise);

This seems unnecessary. Just pass it to the next function call.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:167
> +void CanvasPathMethods::ellipse(float x, float y, float rx, float ry, float rot, float sa, float ea, bool anticlockwise, ExceptionCode& ec)

Can you rename ea, sa to endAngle and startAngle please? (In arc as well.)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:174
> +    if (rx < 0 || ry < 0) {
> +        ec = INDEX_SIZE_ERR;

I am not sure if we want to have an exception in Canvas at all. It may was an mistake to do it in arc in the first place and should be fixed there.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:179
> +        // The ellipse is empty but we still need to draw the connecting line to start point.

So, when you do it here, why do you have the same code in Path again? This seems redundant.

> Source/WebCore/html/canvas/CanvasPathMethods.h:53
> +    void ellipse(float x, float y, float rx, float ry, float rot, float sa, float ea, bool anticlockwise, ExceptionCode&);

startAngle and endAngle please.

> Source/WebCore/platform/graphics/Path.cpp:96
> +void Path::addEllipse(const FloatPoint& p, float radiusX, float radiusY, float rotation, float startAngle, float endAngle, bool anticlockwise)

Hm. Some platforms have direct implementations for ellipse. It might be good to support the platform ellipse implementations.

> Source/WebCore/platform/graphics/Path.cpp:104
> +        else if (p1 != currentPoint())
> +            addLineTo(p1);

Is that specified? Can you add a link to the ChangeLog to the paragraph of the spec please?

> Source/WebCore/platform/graphics/Path.cpp:109
> +    AffineTransform ellipseTransform = AffineTransform().rotate(rad2deg(rotation)).scale(radiusX, radiusY);

Is the origin in the middle of the ellipse? Otherwise, don't you need to set the origin to p before rotating and scaling the ellipse?

> LayoutTests/fast/canvas/ellipse-crash.html:16
> +        context.ellipse(10, 10, 20, 20, 0, 20, 1.0/0.0, true);

Can't you just use Math.NaN and Math.Infinite? That would make more sense here.

> LayoutTests/fast/canvas/script-tests/canvas-ellipse.js:20
> +ctx.ellipse(200, 200, 100, 150, deg2rad(20), deg2rad(-180), deg2rad(100), false);

Why not use rad values directly like Math.PI * 2, Math.PI * 0.5 and so on. I think you have by far not enough tests. You do not test all values and it is not so easy to say if the drawing is correct even for this simple case. It is also interesting to know how ellipse interacts with other path arguments. Maybe it is possible to do a ref test. (Ref test would have a DRT call as well to tell to wait for the completion of the rendering. But not sure how it would look like.)

[Dongseong Hwang]

(In reply to comment #35)
> (From update of attachment 189028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189028&action=review
> 
> This particular feature is still under discussion on the mailing lists. I even doubt that it is that important for web authors at all. However, you do not even follow the specification by making all values optional. I would like to ask you to send a mail to webkit-dev and start a discussion there.
btw, I'll submit the patch removing all optional in other api as well as arc in Bug 115723 .

Long time no see. It is a mistake that all values are optional. I just copied from arc() without thinking. Which mail did you ask me to send to webkit-dev: 'intent to shipment' or 'all values optional'?
In addition, I'm working in progress about this in Blink also: https://codereview.chromium.org/14298022/

> Is it Hwang or Huang? Are you doing this for company100 or intel?

Now, I'm dongseong hwang in Intel :)

> > Source/WebCore/html/canvas/CanvasPathMethods.cpp:136
> > +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'.
> > +    if (anticlockwise && sa - ea >= 2 * piFloat)
> 
> This seems to change the behavior of arc as well. Do you have tests for arc?

There is no logical change to arc and I tested via existing layout test.

> > Source/WebCore/html/canvas/CanvasPathMethods.cpp:163
> > +    float adjustedEndAngle = adjustEndAngle(sa, ea, anticlockwise);
> 
> This seems unnecessary. Just pass it to the next function call.

I extracted adjustEndAngl() from arc() to reuse it in ellipse().

> Can you rename ea, sa to endAngle and startAngle please? (In arc as well.)

> I am not sure if we want to have an exception in Canvas at all. It may was an mistake to do it in arc in the first place and should be fixed there.

> So, when you do it here, why do you have the same code in Path again? This seems redundant.

> startAngle and endAngle please.

I'll apply your opinions.

> > Source/WebCore/platform/graphics/Path.cpp:96
> > +void Path::addEllipse(const FloatPoint& p, float radiusX, float radiusY, float rotation, float startAngle, float endAngle, bool anticlockwise)
> 
> Hm. Some platforms have direct implementations for ellipse. It might be good to support the platform ellipse implementations.

I think that each port maintainer can implement it in Path[port].cpp if they want, after guarding '#if !USE(port)' in above code.
Moreover, when all ports implement ellipse, above code can be removed.

> > Source/WebCore/platform/graphics/Path.cpp:104
> > +        else if (p1 != currentPoint())
> > +            addLineTo(p1);
> 
> Is that specified? Can you add a link to the ChangeLog to the paragraph of the spec please?

I'll verify again that it is correct.

> > Source/WebCore/platform/graphics/Path.cpp:109
> > +    AffineTransform ellipseTransform = AffineTransform().rotate(rad2deg(rotation)).scale(radiusX, radiusY);
> 
> Is the origin in the middle of the ellipse? Otherwise, don't you need to set the origin to p before rotating and scaling the ellipse?
the origin is (0,0) and the middle of the ellipse is p. I draw arc at 'mapped p' as follows, so we don't need to set the origin to p.
addArc(inverseEllipseTransform.mapPoint(p), 1 /* unit circle */, startAngle, endAngle, anticlockwise);

> > LayoutTests/fast/canvas/ellipse-crash.html:16
> > +        context.ellipse(10, 10, 20, 20, 0, 20, 1.0/0.0, true);
> 
> Can't you just use Math.NaN and Math.Infinite? That would make more sense here.

I see. I just followed arc-crash.html, so I'll change both.

> > LayoutTests/fast/canvas/script-tests/canvas-ellipse.js:20
> > +ctx.ellipse(200, 200, 100, 150, deg2rad(20), deg2rad(-180), deg2rad(100), false);
> 
> Why not use rad values directly like Math.PI * 2, Math.PI * 0.5 and so on.

Good advice!

> I think you have by far not enough tests. You do not test all values and it is not so easy to say if the drawing is correct even for this simple case. It is also interesting to know how ellipse interacts with other path arguments. Maybe it is possible to do a ref test. (Ref test would have a DRT call as well to tell to wait for the completion of the rendering. But not sure how it would look like.)

Blink requested me the same and I agree. Unfortunately, there are no standard tests for ellipse yet. I'm preparing various ellipse tests. Ref test is good suggestion!

[Sam Weinig]

Created attachment 232674 [details]
Patch

[Sam Weinig]

(In reply to comment #37)
> Created an attachment (id=232674) [details]
> Patch

I updated the patch and imported the tests added in blink.

[Build Bot]

Comment on attachment 232674 [details]
Patch

Attachment 232674 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6650132057554944

New failing tests:
fast/canvas/canvas-ellipse-connecting-line.html

[Build Bot]

Created attachment 232676 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232674 [details]
Patch

Attachment 232674 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6271279434825728

New failing tests:
fast/canvas/canvas-ellipse-connecting-line.html

[Build Bot]

Created attachment 232677 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232674 [details]
Patch

Attachment 232674 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5399344702291968

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html
fast/canvas/canvas-ellipse-connecting-line.html

[Build Bot]

Created attachment 232678 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dirk Schulze]

Comment on attachment 232674 [details]
Patch

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

The patch looks really great. Some snippets inline. Please fix the build breakage for GTK and EFL before landing. Just move the code from PathCG to Path.cpp as described later and it works everywhere. r=me with the changes below.

> Source/WebCore/ChangeLog:19
> +        Convience for passing a FloatPoint instead of two floats.

convenience?

> Source/WebCore/ChangeLog:23
> +        is greater than 0 and less than 2pi, and the the endAngle is at most 2pi

s/the the/the/

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:217
> +        if (!anticlockwise) {
> +            for (float angle = startAngle - fmodf(startAngle, piOverTwoFloat) + piOverTwoFloat; angle < endAngle; angle += piOverTwoFloat)
> +                lineTo(transform.mapPoint(FloatPoint(radiusX * cosf(angle), radiusY * sinf(angle))));
> +        } else {
> +            for (float angle = startAngle - fmodf(startAngle, piOverTwoFloat); angle > endAngle; angle -= piOverTwoFloat)
> +                lineTo(transform.mapPoint(FloatPoint(radiusX * cosf(angle), radiusY * sinf(angle))));
> +        }

It took me a bit to understand why you are doing this. Especially to get dash arrays and line caps right we must have the multiple moveTos across the otherwise straight line, but a short comment would help :).

IMO the spec should have a comment as well. Just to warn implementations. Couldn't find it in the spec when I reviewed the patch.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:285
> +void Path::addArc(const FloatPoint& p, float radius, float startAngle, float endAngle, bool clockwise)

The other implementations (at least PathCairo) need the same function with NotImplemented(). And the tests need to be skipped.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:303
> +    AffineTransform transform;
> +    transform.translate(p.x(), p.y()).rotate(rad2deg(rotation)).scale(radiusX, radiusY);
> +
> +    CGAffineTransform cgTransform = transform;
> +    CGPathAddArc(ensurePlatformPath(), &cgTransform, 0, 0, 1, startAngle, endAngle, anticlockwise);

This could actually mode to Path.cpp. The other graphic libraries don't have ellipse either and would do the same. This would also make the other bots build. You would need to transform the Path context:

transform(transform);
addArc(...);

Then the comment above gets superfluous.

> LayoutTests/ChangeLog:5
> +

Some crazy tests :P

It seems that some of the tests were submitted to Blink but I couldn't find them on this bug report. Could you add a link to the Blink commit (I think https://chromiumcodereview.appspot.com/14298022). Or mention the author please?

[Dean Jackson]

Created attachment 247555 [details]
Patch

[Dean Jackson]

Uploading the patch again to check test results.

[Dean Jackson]

Committed r180790: <http://trac.webkit.org/changeset/180790>

[Myles C. Maxfield]

fast/canvas/canvas-ellipse-zero-lineto.html is failing on windows.

[Csaba Osztrogonác]

(In reply to comment #48)
> Committed r180790: <http://trac.webkit.org/changeset/180790>
It broke the EFL and GTK build as the EWS noticed. Additionally the reviewer asked you to fix it before landing. Why did you break the build intentionally? Please don't do it next time and fix the build right now. Thanks. It's not WK2 code, you aren't allowed to break the build any time.

[Csaba Osztrogonác]

Buildfix landed in http://trac.webkit.org/changeset/180817