Quadratic and bezier curves with coincident endpoints rendered incorrectly

• Component: Canvas
• Status: RESOLVED FIXED

[Kenneth Russell]

CanvasRenderingContext2D::quadraticCurveTo and bezierCurveTo contain incorrect optimizations which skip adding the incoming curve if its endpoint is the same as the path's current point. It is easy to construct a Bezier curve with coincident endpoints and non-zero area by moving its control points; similarly, a degenerate quadratic curve with coincident endpoints but a different control point should be rendered as a line. The attached test case from https://code.google.com/p/chromium/issues/detail?id=160277 shows the problem for bezier curves, but the same problem exists for quadratic curves.

[Kenneth Russell]

Created attachment 180544 [details]
Test case

[youenn fablet]

I came across this bug description when searching for a potential bug related to CanvasRenderingContext2D::arcTo.
If that is not already done, I can propose a patch fixing this bug and adding corresponding test cases. Probably the test cases could follow the style of the current canvas test suite, something like a green rectangle generation:

<canvas id="myCanvas" width="100" height="50">
</canvas>
<script>
var canvas = document.getElementById('myCanvas');
var ctx = canvas.getContext('2d');
ctx.beginPath();
ctx.moveTo(0, 0);
ctx.bezierCurveTo(100, 0, 0, 50, 0, 0);
ctx.lineWidth = 150;
ctx.strokeStyle = '#0f0';
ctx.stroke();
</script>

[Kenneth Russell]

(In reply to comment #2)
> I came across this bug description when searching for a potential bug related to CanvasRenderingContext2D::arcTo.
> If that is not already done, I can propose a patch fixing this bug and adding corresponding test cases. Probably the test cases could follow the style of the current canvas test suite, something like a green rectangle generation:
> 
> <canvas id="myCanvas" width="100" height="50">
> </canvas>
> <script>
> var canvas = document.getElementById('myCanvas');
> var ctx = canvas.getContext('2d');
> ctx.beginPath();
> ctx.moveTo(0, 0);
> ctx.bezierCurveTo(100, 0, 0, 50, 0, 0);
> ctx.lineWidth = 150;
> ctx.strokeStyle = '#0f0';
> ctx.stroke();
> </script>

It would be great if you would propose a patch and tests. Nobody has taken this bug yet.

I think it would be best if you could write the tests using getImageData() and verifying that a couple of pixels are rendered a certain color, or at all, rather than as pixel tests. That way the precise antialiasing algorithm for curve rendering won't affect the test results.

[youenn fablet]

(In reply to comment #3)
Ok, I will study it in the coming days.

[youenn fablet]

Created attachment 182139 [details]
Patch

[Stephen White]

Comment on attachment 182139 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * fast/canvas/canvas-bezier-same-endpoint-expected.txt: Added.
> +        * fast/canvas/canvas-bezier-same-endpoint.html: Added.
> +        * fast/canvas/canvas-quadratic-same-endpoint-expected.txt: Added.
> +        * fast/canvas/canvas-quadratic-same-endpoint.html: Added.

Looks like these files didn't get attached to the patch.

[Benjamin Poulain]

Comment on attachment 182139 [details]
Patch

You forgot to attach the new tests and expectations.

[youenn fablet]

Created attachment 182310 [details]
Patch

[youenn fablet]

New patch with all files this time.
The tests are working with my local WebKit-Qt.
One test is failing with my local WebKit-EFL, I will investigate this further.

[Build Bot]

Comment on attachment 182310 [details]
Patch

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

New failing tests:
fast/canvas/canvas-quadratic-same-endpoint.html

[Kenneth Russell]

Comment on attachment 182310 [details]
Patch

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

Thanks for the tests. They need a little more work and a clean EWS run.

> LayoutTests/fast/canvas/canvas-bezier-same-endpoint.html:39
> +shouldBeBlue(70,70);

Per the other test below, I think sampling exactly on the line / degenerate curve is too fragile. I think you should sample a few pixels which cross the line, and assert that at least one of them was actually drawn (i.e., wasn't the background color).

> LayoutTests/fast/canvas/canvas-bezier-same-endpoint.html:45
> +shouldBeBlue(75,75);

Same here.

> LayoutTests/fast/canvas/canvas-bezier-same-endpoint.html:49
> +ctx.stroke();

Please test filling of a curve as well as stroking. A degenerate bezier can cover significant area so you can sample the interior in a few places and ensure it was drawn.

> LayoutTests/fast/canvas/canvas-quadratic-same-endpoint.html:37
> +shouldBeYellow(50,50);

Because this test is failing on Mac it looks like it is too fragile. I think you should sample a few pixels which cross the degenerate curve, and assert that at least one was drawn (i.e., wasn't the background color). Also, please test filling as well as stroking the path.

[youenn fablet]

I opened a new bug (https://bugs.webkit.org/show_bug.cgi?id=107118), as the uploaded patch seems to uncover potential issues with back end graphic libraries (such as Qt and cairo) in the rendering of quad curves with same endpoints.

I will increase the lineWidth within the tests to strengthen the tests.
I will update TestExpectations so that mac ews runs fine.

[youenn fablet]

Created attachment 184468 [details]
Patch

[youenn fablet]

Created attachment 184493 [details]
Patch

[youenn fablet]

Created attachment 184738 [details]
Patch

[youenn fablet]

Created attachment 184748 [details]
Patch

[Kenneth Russell]

Comment on attachment 184748 [details]
Patch

Very nice. Thanks for persisting with this patch. Did you want this submitted to the commit queue? Mark cq? in Patch Details if so.

[WebKit Review Bot]

Comment on attachment 184748 [details]
Patch

Clearing flags on attachment: 184748

Committed r141500: <http://trac.webkit.org/changeset/141500>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.