Canvas v5 allows image smoothing to be disabled. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-imagesmoothingenabled
<rdar://problem/11159427>
(In reply to comment #0) > Canvas v5 allows image smoothing to be disabled. > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-imagesmoothingenabled Work for this feature is about to start for the Chromium port. Are there any ongoing efforts in WebKit of which we should be aware?
Not that I know of. I haven't checked recently, but there was some discussion about whether the nearest-neighbour approach should apply everywhere (like patterns etc). I wish I'd linked to the thread mentioning it :(
Created attachment 142344 [details] Patch
Comment on attachment 142344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142344&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2276 > +{ The usual WebKit way would be to do an early out if there's no change: if (m_imageSmoothingEnabled == enabled) return; That should also simplify some logic below, since you can omit both of the secondary checks in the if clauses (I think). > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:235 > + attribute boolean imageSmoothingEnabled; I think this should have the webkit prefix for now, since the spec is still draft (someone correct me if I'm wrong). > LayoutTests/fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-82804.html:21 > + function repaintTest() Repaint tests have a specific meaning in LayoutTests (tests that invalidate a portion of the document and test what is repainted); I'd just call this "draw()" or something instead to avoid confusion. Similarly, runRepaintTest() -> runTests(). > LayoutTests/fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-82804.html:77 > +</html> If this is a pixel test, it should have expected pixel results for at least one platform (as well as text results, even if they're empty).
Comment on attachment 142344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142344&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:235 >> + attribute boolean imageSmoothingEnabled; > > I think this should have the webkit prefix for now, since the spec is still draft (someone correct me if I'm wrong). Yeah, I agree. In this case there isn't a huge chance behaviour will change, but you never know (the name could change, for example). >> LayoutTests/fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-82804.html:77 >> +</html> > > If this is a pixel test, it should have expected pixel results for at least one platform (as well as text results, even if they're empty). Is there a reason why this needs to be a pixel test? Especially in the disabled case, it should be easy enough to test pixels each side of a colour change.
(In reply to comment #6) > (From update of attachment 142344 [details]) > > If this is a pixel test, it should have expected pixel results for at least one platform (as well as text results, even if they're empty). > > Is there a reason why this needs to be a pixel test? Especially in the disabled case, it should be easy enough to test pixels each side of a colour change. I was thinking a pixel test would be better for the interpolated case, where platforms might differ on smoothing algorithm, and it might be difficult to detect in a cross-platform way. If we know that all ports in InterpolationDefault do some kind of smoothing (ie., a non-aligned (smoothed) pixel halfway between source pixel centers at has some non-zero difference from one of the source pixels), then a script test would be easy to write. I just wasn't sure that all ports had that property, since I think you could get away with no image smoothing in any case and still be spec-compliant.
Comment on attachment 142344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142344&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2279 > + drawingContext()->setImageInterpolationQuality(m_previousImageInterpolationQuality); Actually, to keep things simpler, I think you can just set the interpolation back to DefaultInterpolationQuality (defined in HTMLCanvasElement.h), rather than saving the previous quality. This is the only other value that the image interpolation can be right now for canvas.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 142344 [details] [details]) > > > If this is a pixel test, it should have expected pixel results for at least one platform (as well as text results, even if they're empty). > > > > Is there a reason why this needs to be a pixel test? Especially in the disabled case, it should be easy enough to test pixels each side of a colour change. > > I was thinking a pixel test would be better for the interpolated case, where platforms might differ on smoothing algorithm, and it might be difficult to detect in a cross-platform way. > > If we know that all ports in InterpolationDefault do some kind of smoothing (ie., a non-aligned (smoothed) pixel halfway between source pixel centers at has some non-zero difference from one of the source pixels), then a script test would be easy to write. I just wasn't sure that all ports had that property, since I think you could get away with no image smoothing in any case and still be spec-compliant. That's true, but since the functionality we're adding is turning off smoothing, I figured it would be ok to assume whatever ports are doing now is not a regression for them. You're right though - we should probably test this in the case of toggling it multiple times within a single test (although there we could simply record the value). Anyway, I'm not against a pixel test, I was just seeing if we could avoid it :)
Created attachment 142554 [details] Patch
Comment on attachment 142554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142554&action=review > Source/WebCore/ChangeLog:9 > + Tests: fast/canvas/canvas-check-imageSmoothingEnabled-getter-webkit-bug-82804.html > + fast/canvas/canvas-check-imageSmoothingEnabled-works-webkit-bug-82804.html We don't put the webkit bug ID in test names. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2282 > + if (enabled) > + drawingContext()->setImageInterpolationQuality(InterpolationDefault); > + else > + drawingContext()->setImageInterpolationQuality(InterpolationNone); Actually, this should be DefaultInterpolationQuality (#defined in HTMLCanvasElement.h), not InterpolationDefault -- it's defined differently on different ports. It could also be a bit shorter as: drawingContext()->setImageInterpolationQuality(enabled ? DefaultInterpolationQuality : InterpolationNone); > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:339 > + Nit: don't need this extra vertical whitespace. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:341 > + Or this.
Created attachment 142573 [details] Patch
Comment on attachment 142573 [details] Patch Attachment 142573 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12723449 New failing tests: platform/chromium/virtual/gpu/fast/canvas/canvas-check-imageSmoothingEnabled-works.html
Created attachment 142592 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #13) > (From update of attachment 142573 [details]) > Attachment 142573 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12723449 > > New failing tests: > platform/chromium/virtual/gpu/fast/canvas/canvas-check-imageSmoothingEnabled-works.html Ah yes, you'll need GPU results for cr-linux as well. That said, perhaps we should try to do a script test instead of a pixel test after all. If any platforms don't do some kind of interpolation by default, we'll find out when the bots run and we can go back to a pixel test instead.
Created attachment 142735 [details] Patch
Comment on attachment 142735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142735&action=review > LayoutTests/ChangeLog:11 > + * platform/chromium-linux/fast/canvas/canvas-imageSmoothingEnabled-expected.txt: Added. > + * platform/chromium-linux/platform/chromium/virtual/gpu/fast/canvas/canvas-imageSmoothingEnabled-expected.txt: Added. Since this test should have the same results on all platforms, they should be placed in the same directory as the test (fast/canvas). > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:6 > +/** > + * @fileoverview Description of this file. > + * @author keyar@chromium.com (Keyar Hood) > + */ We don't put these kinds of headers in source (or test) files. > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:10 > +debug("Test default value is true."); Grammar nit: "Test that the default value is true." > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:13 > +debug("Test correct value is returned after a set."); Grammar nit: "Test that the correct value is returned after a set." > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:47 > +debug("Test image is smoothed by default. We check by making sure the pixel just to the left of the middle line is not completely black."); Grammar nit: Same. > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:55 > +debug("Test nearest neighbour is used when imageSmoothingEnabled is set to false. We check this by making sure the pixel just to the left of the middle line is completely black."); Grammar nit: same. > LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:63 > +debug("Test image is smoothed when imageSmoothingEnabled is set to true."); Grammar hit: same.
Created attachment 142738 [details] Patch
Comment on attachment 142738 [details] Patch Looks good (assuming the bots are happy with it). r=me
Comment on attachment 142738 [details] Patch Attachment 142738 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12724548 New failing tests: platform/chromium/virtual/gpu/fast/canvas/canvas-imageSmoothingEnabled.html fast/canvas/canvas-imageSmoothingEnabled.html
Created attachment 142757 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 142758 [details] Patch
Comment on attachment 142758 [details] Patch Looks good. r=me
Comment on attachment 142758 [details] Patch Clearing flags on attachment: 142758 Committed r117635: <http://trac.webkit.org/changeset/117635>
All reviewed patches have been landed. Closing bug.