Summary: | Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10 | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Olson <jonathan.olson> | ||||||||||||||||||
Component: | Canvas | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED WORKSFORME | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, dglazkov, dino, krit, ojan.autocc, rniwa, simon.fraser, tony, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Jonathan Olson
2013-02-03 00:45:04 PST
Created attachment 186250 [details]
Proposed patch for the miterLimit issue. Works for me, but there may be a better design.
Created attachment 186251 [details]
Proposed test case for the miterLimit issue, with expected text output included
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. 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. 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 Created attachment 186255 [details]
Proposed fix and test case (with expected txt result)
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 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 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 Created attachment 186272 [details]
Patch: Proposed fix, test case, expected output, and removal of old test case expected PNGs with incorrect miterLimit handling
Created attachment 186273 [details]
Patch
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. (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. 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. Created attachment 186451 [details]
Patch
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 Created attachment 186469 [details]
Patch
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 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 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? 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. (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) +2013-02-04 Jonathan Olson <olsonsjc@gmail.com> + + 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) +2013-02-04 Jonathan Olson <olsonsjc@gmail.com> + + 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. Seems to be fixed. |