RESOLVED FIXED 82804
Support imageSmoothingEnabled
https://bugs.webkit.org/show_bug.cgi?id=82804
Summary Support imageSmoothingEnabled
Dean Jackson
Reported 2012-03-30 16:10:02 PDT
Attachments
Patch (7.84 KB, patch)
2012-05-16 14:10 PDT, Keyar Hood
no flags
Patch (13.59 KB, patch)
2012-05-17 14:17 PDT, Keyar Hood
no flags
Patch (13.09 KB, patch)
2012-05-17 15:48 PDT, Keyar Hood
no flags
Archive of layout-test-results from ec2-cr-linux-03 (547.75 KB, application/zip)
2012-05-17 17:23 PDT, WebKit Review Bot
no flags
Patch (11.68 KB, patch)
2012-05-18 10:42 PDT, Keyar Hood
no flags
Patch (9.72 KB, patch)
2012-05-18 10:54 PDT, Keyar Hood
no flags
Archive of layout-test-results from ec2-cr-linux-01 (532.34 KB, application/zip)
2012-05-18 11:57 PDT, WebKit Review Bot
no flags
Patch (9.77 KB, patch)
2012-05-18 11:59 PDT, Keyar Hood
no flags
Radar WebKit Bug Importer
Comment 1 2012-03-30 16:10:38 PDT
Jeff Timanus
Comment 2 2012-05-09 15:20:08 PDT
(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?
Dean Jackson
Comment 3 2012-05-09 16:12:20 PDT
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 :(
Keyar Hood
Comment 4 2012-05-16 14:10:33 PDT
Stephen White
Comment 5 2012-05-16 14:41:32 PDT
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).
Dean Jackson
Comment 6 2012-05-16 16:00:45 PDT
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.
Stephen White
Comment 7 2012-05-17 07:01:38 PDT
(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.
Stephen White
Comment 8 2012-05-17 07:06:00 PDT
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.
Dean Jackson
Comment 9 2012-05-17 11:31:33 PDT
(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 :)
Keyar Hood
Comment 10 2012-05-17 14:17:32 PDT
Stephen White
Comment 11 2012-05-17 14:23:48 PDT
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.
Keyar Hood
Comment 12 2012-05-17 15:48:49 PDT
WebKit Review Bot
Comment 13 2012-05-17 17:23:36 PDT
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
WebKit Review Bot
Comment 14 2012-05-17 17:23:40 PDT
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
Stephen White
Comment 15 2012-05-18 06:54:16 PDT
(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.
Keyar Hood
Comment 16 2012-05-18 10:42:05 PDT
Stephen White
Comment 17 2012-05-18 10:48:21 PDT
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.
Keyar Hood
Comment 18 2012-05-18 10:54:08 PDT
Stephen White
Comment 19 2012-05-18 10:55:47 PDT
Comment on attachment 142738 [details] Patch Looks good (assuming the bots are happy with it). r=me
WebKit Review Bot
Comment 20 2012-05-18 11:57:38 PDT
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
WebKit Review Bot
Comment 21 2012-05-18 11:57:54 PDT
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
Keyar Hood
Comment 22 2012-05-18 11:59:41 PDT
Stephen White
Comment 23 2012-05-18 12:59:24 PDT
Comment on attachment 142758 [details] Patch Looks good. r=me
WebKit Review Bot
Comment 24 2012-05-18 14:45:38 PDT
Comment on attachment 142758 [details] Patch Clearing flags on attachment: 142758 Committed r117635: <http://trac.webkit.org/changeset/117635>
WebKit Review Bot
Comment 25 2012-05-18 14:45:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.