Bug 108763 – Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10

• Component: Canvas
• Status: RESOLVED WORKSFORME

[Jonathan Olson]

Created attachment 186249 [details]
HTML for reproduction of the miterLimit issue, explained in comments

Creating and drawing with a CanvasRenderingContext2D uses an incorrect intial miterLimit of 4 for stroking paths.

Steps to reproduce:
1) View http://phet.colorado.edu/files/phet-scene/tests/browsers/chrome-miter-limit.html or the attached chrome-miter-limit.html in Chrome / Chromium (24.x, 26.x, trunk)

Actual Results:
The first 2 drawn line joins are cut off with a bevel instead of being mitered.

Expected Results:
All 3 line joins should have the mitered join visible.

Build Date & Platform:
Chromium build 180186 with WebKit 141577. Also confirmed as buggy in Chrome across platforms (24.x, Win, Mac, Linux, Android) and Canary (26.x, Win, Linux)
Doesn't occur on iOS 6.1 Safari or latest Lion Safari (may be PlatformContextSkia specific)

Additional Information:
I'll attempt to attach my patches for the code and a test case soon. At least when using Skia on my build setup, it appears that CanvasRenderingContext2D.cpp has its state initialized with
a miterLimit of 10, but its GraphicsContext (PlatformContextSkia) has its miterLimit initialized as 4, and they do not seem to synchronize this value. Thus while
canvas.miterLimit === 10 in JS, shapes are initially stroked with a miterLimit of 4 until canvas.miterLimit is given a value different from 10.

Of note, the layout test fast/canvas/canvas-incremental-repaint-diffs.html shows the buggy behavior for all of the Chromium platforms, while for qt, efl, efl-wk1 and mac
platforms the expected png correctly renders the miter. This layout test will fail with the above patch when testing Chromium due to the pixel comparison, and the
beveled PNG examples should ideally be removed.

The attached chrome-miter-limit.html (for the reproduction steps) explains in comments the specific sources of the buggy behavior.

This is my first bug report for WebKit, so my apologies if the code needs to be patched completely differently or if there are other problems with this bug submission.

[Jonathan Olson]

Created attachment 186250 [details]
Proposed patch for the miterLimit issue. Works for me, but there may be a better design.

[Jonathan Olson]

Created attachment 186251 [details]
Proposed test case for the miterLimit issue, with expected text output included

[Jonathan Olson]

Also possibly important to note that in the HTML bug-reproduction, it draws a black full-alpha line join, then green half-alpha line join, then a red half-alpha line join, all overlapping each other. Expected appearance should be a solid orange line join, as is viewable on IE9+, Firefox 18, latest Opera, latest Safari, etc.

When it fails, the mitered area will appear pink due to the last line join having the correct miterLimit internally.

[Jonathan Olson]

My diffs against WebKit checked out from Chromium look to be failing. I'll have those in the main WebKit source (and up-to-date) soon.

[Build Bot]

Comment on attachment 186251 [details]
Proposed test case for the miterLimit issue, with expected text output included

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

[Jonathan Olson]

Created attachment 186255 [details]
Proposed fix and test case (with expected txt result)

[Build Bot]

Comment on attachment 186255 [details]
Proposed fix and test case (with expected txt result)

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

New failing tests:
fast/canvas/canvas-miterLimit.html

[WebKit Review Bot]

Comment on attachment 186255 [details]
Proposed fix and test case (with expected txt result)

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

New failing tests:
fast/canvas/canvas-miterLimit.html
fast/canvas/canvas-incremental-repaint.html
platform/chromium/virtual/gpu/fast/canvas/canvas-miterLimit.html

[Build Bot]

Comment on attachment 186255 [details]
Proposed fix and test case (with expected txt result)

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

[Jonathan Olson]

Created attachment 186272 [details]
Patch: Proposed fix, test case, expected output, and removal of old test case expected PNGs with incorrect miterLimit handling

[Jonathan Olson]

Created attachment 186273 [details]
Patch

[Jonathan Olson]

The patch won't apply because of attempting to remove binary files (which EWS apparently does not do?), but the removals are necessary to remove incorrect expected PNGs for the layout tests to pass.

[Tony Chang]

(In reply to comment #12)
> The patch won't apply because of attempting to remove binary files (which EWS apparently does not do?), but the removals are necessary to remove incorrect expected PNGs for the layout tests to pass.

It looks like you created this patch from an SVN checkout.  If you use Tools/Scripts/svn-create-patch, it will make a diff file that the EWS bots should know how to apply.  You can also use "Tools/Scripts/webkit-patch upload", which should also create the patch and upload it to the bug.

[Jonathan Olson]

Attachment 186272 [details] (2nd-to-last patch) was created from Tools/Scripts/svn-create-patch and manually uploaded, and attachment 186273 [details] (last patch) was uploaded using 'Tools/Scripts/webkit-patch upload'.

Maybe it's a SVN issue, since I have SVN 1.6.6 installed on the VM. I'll try upgrading to see if that helps.

[Jonathan Olson]

Created attachment 186451 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186451 [details]
Patch

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

New failing tests:
fast/canvas/canvas-miterLimit.html
fast/canvas/canvas-incremental-repaint.html
platform/chromium/virtual/gpu/fast/canvas/canvas-miterLimit.html

[Jonathan Olson]

Created attachment 186469 [details]
Patch

[Build Bot]

Comment on attachment 186469 [details]
Patch

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

New failing tests:
fast/canvas/canvas-miterLimit.html

[WebKit Review Bot]

Comment on attachment 186469 [details]
Patch

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

New failing tests:
fast/canvas/canvas-incremental-repaint.html

[Jonathan Olson]

It notes that the fast/canvas/canvas-incremental-repaint.html test case is failing (and the old expected PNG results were bad due to this bug).

Any tips on fixing the patch so the test case passes?

[Dirk Schulze]

Comment on attachment 186469 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
> +        https://bugs.webkit.org/show_bug.cgi?id=108763
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/canvas/canvas-miterLimit.html

Can you be a bit more explicit and add more description please? A changelog should contain a description of the changes and why you changed it.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:139
> +    applyLineStyles(); // Ensure the GraphicsContext is initialized with consistent line styles, as values like miterLimit may be different.

comments in the line before.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:599
> +void CanvasRenderingContext2D::applyLineStyles() const

It is not called anywhere else, an inline function where you pass the Graphics context and state might be enough.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:608
> +    c->setStrokeThickness(state().m_lineWidth);
> +    c->setLineCap(state().m_lineCap);
> +    c->setLineJoin(state().m_lineJoin);
> +    c->setMiterLimit(state().m_miterLimit);
> +}

Are there problems with the other default value as well? If so, can you add more tests to cover that please?

> LayoutTests/ChangeLog:4
> +        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
> +        https://bugs.webkit.org/show_bug.cgi?id=108763

Please add one or two lines of description.

> LayoutTests/ChangeLog:18
> +        * platform/chromium-android/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-linux-x86/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-linux/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-mac-lion/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-mac-snowleopard/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-mac/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-win-xp/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/chromium-win/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> +        * platform/mac-snowleopard/fast/canvas/canvas-incremental-repaint-expected.png: Removed.

Is this cleanup? This should be mentioned in the change log as well.

> LayoutTests/fast/canvas/canvas-miterLimit-expected.txt:5
> +CONSOLE MESSAGE: line 19: The following alpha should be 255, since the unit rectangle is completely in the mitered join bounds.
> +CONSOLE MESSAGE: line 19: Since the drawn angle is 13 degrees, a mitered join will be drawn if miterLimit is above 8.833671471997569.
> +CONSOLE MESSAGE: line 19: The default CanvasRenderingContext2D miterLimit value is 10, but some GraphicsContexts are created with a default of 4.
> +CONSOLE MESSAGE: line 19: If miterLimit is initialized incorrectly, this will render as a bevel join instead and will be transparent (0).
> +CONSOLE MESSAGE: line 19: imageData.data[3] = 255

We don't use write() and test with console messages. Please look at other tests in the folder like canvas-clearRect.

[Jonathan Olson]

(In reply to comment #21)
> (From update of attachment 186469 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186469&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
> > +        https://bugs.webkit.org/show_bug.cgi?id=108763
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Test: fast/canvas/canvas-miterLimit.html
> 
> Can you be a bit more explicit and add more description please? A changelog should contain a description of the changes and why you changed it.

Sorry, I had tried to add the following changelog entries, but it appears it was wiped out in the latest patch. I'll submit a new patch with the tweaks recommended, and with fixed changelog entries. They were as below:

(Source/WebCore/ChangeLog)
[phone]  Jonathan Olson  <[e-mail]>
+
+        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
+        https://bugs.webkit.org/show_bug.cgi?id=108763
+
+        Reviewed by NOBODY (OOPS!).
+
+        Certain GraphicsContexts (namely PlatformContextSkia) are initialized with some line style
+        attributes (in this case miterLimit) that are different from the Canvas defaults, and
+        CanvasRenderingContext2D did not properly initialize these values. Creating a ContextRenderingContext2D
+        (in JS with getCanvas( '2d ') ) and drawing would use an improper miterLimit of 4 (SVG default)
+        instead of the Canvas default of 10. This change adds applyLineStyles() which properly initializes
+        these values in the GraphicsContext on CanvasRenderingContext2D construction. It includes more
+        than just the miterLimit as a safety check.

and

(LayoutTests/ChangeLog)
[phone]  Jonathan Olson  <[e-mail]>
+
+        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
+        https://bugs.webkit.org/show_bug.cgi?id=108763
+
+        Reviewed by NOBODY (OOPS!).
+
+        Certain GraphicsContexts (namely PlatformContextSkia) are initialized with some line style
+        attributes (in this case miterLimit) that are different from the Canvas defaults, and
+        CanvasRenderingContext2D did not properly initialize these values. Creating a ContextRenderingContext2D
+        (in JS with getCanvas( '2d ') ) and drawing would use an improper miterLimit of 4 (SVG default)
+        instead of the Canvas default of 10. This change adds applyLineStyles() which properly initializes
+        these values in the GraphicsContext on CanvasRenderingContext2D construction. It includes more
+        than just the miterLimit as a safety check.
+
+        canvas-incremental-repaint-expected.html included a line join that has an angle that is beveled with
+        a miterLimit of 4 but mitered with a miterLimit of 10, so I've removed the incorrectly-beveled test
+        PNG cases (predominantly chromium, due to Skia).

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:139
> > +    applyLineStyles(); // Ensure the GraphicsContext is initialized with consistent line styles, as values like miterLimit may be different.
> 
> comments in the line before.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:599
> > +void CanvasRenderingContext2D::applyLineStyles() const
> 
> It is not called anywhere else, an inline function where you pass the Graphics context and state might be enough.

Or would it be better to just inline this inside the one call site (in the CanvasRenderingContext2D constructor?)

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:608
> > +    c->setStrokeThickness(state().m_lineWidth);
> > +    c->setLineCap(state().m_lineCap);
> > +    c->setLineJoin(state().m_lineJoin);
> > +    c->setMiterLimit(state().m_miterLimit);
> > +}
> 
> Are there problems with the other default value as well? If so, can you add more tests to cover that please?

I am not aware of any problems besides the miterLimit issue, I was just trying to be safe by keeping that initialization inside CanvasRenderingContext2D, instead of relying on each GraphicsContext's default to remain forever the same as the Canvas spec requires.

> 
> > LayoutTests/ChangeLog:4
> > +        Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10
> > +        https://bugs.webkit.org/show_bug.cgi?id=108763
> 
> Please add one or two lines of description.
> 
> > LayoutTests/ChangeLog:18
> > +        * platform/chromium-android/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-linux-x86/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-linux/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-mac-lion/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-mac-snowleopard/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-mac/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-win-xp/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/chromium-win/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> > +        * platform/mac-snowleopard/fast/canvas/canvas-incremental-repaint-expected.png: Removed.
> 
> Is this cleanup? This should be mentioned in the change log as well.

This will also be fixed in the changelog as noted above.

> 
> > LayoutTests/fast/canvas/canvas-miterLimit-expected.txt:5
> > +CONSOLE MESSAGE: line 19: The following alpha should be 255, since the unit rectangle is completely in the mitered join bounds.
> > +CONSOLE MESSAGE: line 19: Since the drawn angle is 13 degrees, a mitered join will be drawn if miterLimit is above 8.833671471997569.
> > +CONSOLE MESSAGE: line 19: The default CanvasRenderingContext2D miterLimit value is 10, but some GraphicsContexts are created with a default of 4.
> > +CONSOLE MESSAGE: line 19: If miterLimit is initialized incorrectly, this will render as a bevel join instead and will be transparent (0).
> > +CONSOLE MESSAGE: line 19: imageData.data[3] = 255
> 
> We don't use write() and test with console messages. Please look at other tests in the folder like canvas-clearRect.

Ok, thanks! I'll fix this.

[Dirk Schulze]

Seems to be fixed.