Implement InterpolationMedium to filter animated images

Implement InterpolationMedium to filter animated images

Since r168759 we use bilinear filtering for animated images. There
are complaints about the drop in image quality. However we don't want to
spend a lot of CPU cycles doing high quality animations for upscaling.
This change is a compromise for animated images: we do a cheaper
mipmapping for downscaling and bilinear for upscaling. There is a
noticable quality improvement.

This change implements InterpolationMedium for GraphicsContext. It used
to be equivalent to InterpolationLow and is now a higher quality option.
All GraphicsContext::drawSomething() methods have |useLowQualityScale|
removed. This gives a much cleaner code.

I also discovered/fixed a couple small issues with interpolation levels:
* It is apparent now that InterpolationDefault means InterpolationHigh
  on desktop and InterpolationLow on Android.
* Default for canvas elements used to InterpolationMedium but it
  really meant InterpolationLow.
* There is a bug for rendering images in canvas that InterpolationNone
  should be used but it has been InterpolationLow all along. I didn't
  fix in this change but added a FIXME.

The new interpolation level does not affect non-animated images. There
is no need to rebaseline any pixel test. I added a test GIF file that
loops two identical frames. This is used to test the image quality.

BUG=353251
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169585

[Stephen Chennney, 2014-03-18 12:56:31]

Any chance this could be a ref test?

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
File Source/core/rendering/ImageQualityController.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:68: // For images that are
potentially animated we paint them at low quality.
This comment doesn't seem to match the code. Is the problem the comment or the
semantics of InterpolationMedium.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.cpp (left):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.cpp:153: case
InterpolationDefault:
I think you'll get build errors due to enum not in the switch.

[Stephen White, 2014-03-18 17:26:43]

https://codereview.chromium.org/201513003/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:306: void
HTMLCanvasElement::paint(GraphicsContext* context, const LayoutRect& r,
InterpolationQuality interpolationQuality)
I believe this is only used when compositing the canvas into the page, not while
drawing elements into the canvas (and only in software rasterization, IIRC, not
for accelerated rendering).

I'm not sure if that changes what value we pass here.

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
File Source/core/rendering/ImageQualityController.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:66: return InterpolationLow;
We actually might want to consider using Medium here as well, to get mips for
minification on animated HTML elements as well. But we can consider that in a
separate change.

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderHTMLCanvas.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderHTMLCanvas.cpp:76: // FIXME: InterpolationNone
should be used if ImageRenderingOptimizeContrast is set.
Please log a bug for this, if there isn't one already.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1054: if (interpolationQuality !=
InterpolationDefault) {
Shouldn't this check be against imageInterpolationQuality(), not
InterpolationDefault?

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1069: if (interpolationQuality !=
InterpolationDefault) {
Same here.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1091: if (interpolationQuality !=
InterpolationDefault) {
Same here.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1137: if (interpolationQuality !=
InterpolationDefault) {
Same here. (Tangentially, I wonder if these functions could be templated and/or
refactored.)

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1140: image->draw(this, dest, src,
op, blendMode, interpolationQuality);
(Not new to this patch, but it seems odd that we have to both set the
interpolation quality, and pass it to image->draw() as well.)

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.cpp:366: else if (resampling ==
MediumResampling)
I'd suggest to refactor this out into a helper function that converts between
resampling mode + useBicubicFilter and filterlevel, and call that here and
below.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.h (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.h:66: MediumResampling,
Bikeshed: maybe this should be LinearMipmaps or LinearWithMipmaps or something.
The other types in this enum refer to algorithms, while this seems to belong
more to high/medium/low.

[Alpha Left Google, 2014-03-18 22:59:12]

I'm not sure I can make a ref test. Because there's no other images rendered
with medium quality other than animated GIFs.

After some changes I found that having interpolationQuality in each drawBlah()
call in GraphicsContext is problematic. I have removed the argument and now it
only relies on GraphicsContext::setImageInterpolationQuality().

Again I don't see any pixel difference.

https://codereview.chromium.org/201513003/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:306: void
HTMLCanvasElement::paint(GraphicsContext* context, const LayoutRect& r,
InterpolationQuality interpolationQuality)
On 2014/03/18 17:26:43, Stephen White wrote:
> I believe this is only used when compositing the canvas into the page, not
while
> drawing elements into the canvas (and only in software rasterization, IIRC,
not
> for accelerated rendering).
> 
> I'm not sure if that changes what value we pass here.

I don't think it matters functionally. If the argument stays a boolean then I
have to do an ugly convertion from bool -> InterpolationQuality in the
drawBlah() calls. Having the enum as parameter seems cleaner.

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
File Source/core/rendering/ImageQualityController.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:66: return InterpolationLow;
On 2014/03/18 17:26:43, Stephen White wrote:
> We actually might want to consider using Medium here as well, to get mips for
> minification on animated HTML elements as well. But we can consider that in a
> separate change.

That's right. We can update that later.

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:68: // For images that are
potentially animated we paint them at low quality.
On 2014/03/18 12:56:32, Stephen Chenney wrote:
> This comment doesn't seem to match the code. Is the problem the comment or the
> semantics of InterpolationMedium.

The code is correct. I've updated the comments.

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderHTMLCanvas.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderHTMLCanvas.cpp:76: // FIXME: InterpolationNone
should be used if ImageRenderingOptimizeContrast is set.
On 2014/03/18 17:26:43, Stephen White wrote:
> Please log a bug for this, if there isn't one already.

Done.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1054: if (interpolationQuality !=
InterpolationDefault) {
On 2014/03/18 17:26:43, Stephen White wrote:
> Shouldn't this check be against imageInterpolationQuality(), not
> InterpolationDefault?

Done.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1069: if (interpolationQuality !=
InterpolationDefault) {
On 2014/03/18 17:26:43, Stephen White wrote:
> Same here.

Done.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1091: if (interpolationQuality !=
InterpolationDefault) {
On 2014/03/18 17:26:43, Stephen White wrote:
> Same here.

Done.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1137: if (interpolationQuality !=
InterpolationDefault) {
On 2014/03/18 17:26:43, Stephen White wrote:
> Same here. (Tangentially, I wonder if these functions could be templated
and/or
> refactored.)

Done. Yeah I'll leave that refactoring as a separate change.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1140: image->draw(this, dest, src,
op, blendMode, interpolationQuality);
On 2014/03/18 17:26:43, Stephen White wrote:
> (Not new to this patch, but it seems odd that we have to both set the
> interpolation quality, and pass it to image->draw() as well.)

Indeed. :(

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.cpp (left):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.cpp:153: case
InterpolationDefault:
On 2014/03/18 12:56:32, Stephen Chenney wrote:
> I think you'll get build errors due to enum not in the switch.

Actually this is correct since InterpolationDefault equals InterpolationLow or
InterpolationHigh. The code now is actually cleaner.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.cpp (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.cpp:366: else if (resampling ==
MediumResampling)
On 2014/03/18 17:26:43, Stephen White wrote:
> I'd suggest to refactor this out into a helper function that converts between
> resampling mode + useBicubicFilter and filterlevel, and call that here and
> below.

Done.

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/NativeImageSkia.h (right):

https://codereview.chromium.org/201513003/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/NativeImageSkia.h:66: MediumResampling,
On 2014/03/18 17:26:43, Stephen White wrote:
> Bikeshed: maybe this should be LinearMipmaps or LinearWithMipmaps or
something.
> The other types in this enum refer to algorithms, while this seems to belong
> more to high/medium/low.

Done.

[Stephen White, 2014-03-19 13:57:56]

Looks good (assuming the bots are happy), but I'll leave for schenney@ to take a
look.

[Stephen Chennney, 2014-03-19 15:23:41]

All good. LGTM.

[Alpha Left Google, 2014-03-19 19:42:01]

On 2014/03/19 15:23:41, Stephen Chenney wrote:
> All good. LGTM.

I'll submit this change without the new layout test such that it can run through
the bots. And then I'll land the test separately.

[Alpha Left Google, 2014-03-19 19:52:18]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-03-19 19:52:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/201513003/40001

[commit-bot: I haz the power, 2014-03-19 20:39:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 20:39:14]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Alpha Left Google, 2014-03-19 20:55:14]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-03-19 20:55:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/201513003/40001

[commit-bot: I haz the power, 2014-03-19 21:37:15]

Change committed as 169585