Bug 82804 – Support imageSmoothingEnabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dean Jackson]

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

[Radar WebKit Bug Importer]

<rdar://problem/11159427>

[Jeff Timanus]

(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]

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]

Created attachment 142344 [details]
Patch

[Stephen White]

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 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]

(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 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]

(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]

Created attachment 142554 [details]
Patch

[Stephen White]

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]

Created attachment 142573 [details]
Patch

[WebKit Review Bot]

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]

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]

(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]

Created attachment 142735 [details]
Patch

[Stephen White]

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 [e-mail] (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]

Created attachment 142738 [details]
Patch

[Stephen White]

Comment on attachment 142738 [details]
Patch

Looks good (assuming the bots are happy with it).  r=me

[WebKit Review Bot]

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]

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]

Created attachment 142758 [details]
Patch

[Stephen White]

Comment on attachment 142758 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 142758 [details]
Patch

Clearing flags on attachment: 142758

Committed r117635: <http://trac.webkit.org/changeset/117635>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.