WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Attachments
Patch
(7.84 KB, patch)
2012-05-16 14:10 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
Patch
(13.59 KB, patch)
2012-05-17 14:17 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
Patch
(13.09 KB, patch)
2012-05-17 15:48 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(11.68 KB, patch)
2012-05-18 10:42 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
Patch
(9.72 KB, patch)
2012-05-18 10:54 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(9.77 KB, patch)
2012-05-18 11:59 PDT
,
Keyar Hood
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-03-30 16:10:38 PDT
<
rdar://problem/11159427
>
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
Created
attachment 142344
[details]
Patch
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
Created
attachment 142554
[details]
Patch
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
Created
attachment 142573
[details]
Patch
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
Created
attachment 142735
[details]
Patch
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
Created
attachment 142738
[details]
Patch
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
Created
attachment 142758
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug