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.
Created attachment 180544 [details] Test case
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>
(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.
(In reply to comment #3) Ok, I will study it in the coming days.
Created attachment 182139 [details] Patch
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.
Comment on attachment 182139 [details] Patch You forgot to attach the new tests and expectations.
Created attachment 182310 [details] Patch
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.
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
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.
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.
Created attachment 184468 [details] Patch
Created attachment 184493 [details] Patch
Created attachment 184738 [details] Patch
Created attachment 184748 [details] Patch
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.
Comment on attachment 184748 [details] Patch Clearing flags on attachment: 184748 Committed r141500: <http://trac.webkit.org/changeset/141500>
All reviewed patches have been landed. Closing bug.