Switched to JDCT_ISLOW for Android.

Switched to JDCT_ISLOW for Android.

The ISLOW method does not produce JPEG artifacts, which IFAST
may do. Also, ISLOW is not really much slower than IFAST
(it is actually faster for some images).

BUG=385515

[mage, 2014-06-17 08:18:10]

This is a small patch to get rid of JPEG artifacts on Android.

I decided to split the LOW_QUALITY_IMAGE_NO_JPEG_DITHERING configuration flag
into two flags, since it was in fact doing two things (no dithering + fast dct).
No product currently uses the new LOW_QUALITY_IMAGE_FAST_JPEG_DCT flag though
(since that's the point of this patch).

I was not sure who to pick as a reviewer. Please re-assign to someone more
appropriate if necessary.

[Peter Kasting, 2014-06-17 17:49:05]

LGTM in principle, but please work with noel, kbr, and/or other folks that know
how to do image perf testing to get a more complete picture of the performance
impact of this change.  Your comments on the bug are a good start, but not
enough data for me to feel confident in this.

https://codereview.chromium.org/339143002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/339143002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:83: #if
USE(LOW_QUALITY_IMAGE_FAST_JPEG_DCT)
If nobody uses this, I'd scrap it and add a comment here about why we always
want JDCT_ISLOW.

[Ken Russell (switch to Gerrit), 2014-06-17 21:28:35]

This seems reasonable in order to improve image quality, but people working
specifically on Chrome on Android should review it. Also, please work with noel@
and tonyg@ to run this change through a suite of performance tests, not just
three sample images.

https://codereview.chromium.org/339143002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/339143002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:83: #if
USE(LOW_QUALITY_IMAGE_FAST_JPEG_DCT)
On 2014/06/17 17:49:04, Peter Kasting wrote:
> If nobody uses this, I'd scrap it and add a comment here about why we always
> want JDCT_ISLOW.

Agree with this comment.

[aelias_OOO_until_Jul13, 2014-06-17 21:42:16]

The motivating use case misused JPEG to display a logo, which is the
stereotypical case where PNG should've been used, so I'm not convinced it's
worth taking a performance regression for that.  The claim that ISLOW can
sometimes be faster is intriguing, but it sounds that at least one of your test
cases showed a 40% regression, which is scary.  Given what's been observed so
far, I don't think the evidence supports making this change, although I could be
convinced by more systematic data.

[Peter Kasting, 2014-06-17 21:48:13]

On 2014/06/17 21:42:16, aelias wrote:
> The motivating use case misused JPEG to display a logo, which is the
> stereotypical case where PNG should've been used,

I don't like this kind of argument.  Yes, in principle, you're correct, but in
practice, the JPEG actually performs acceptably well, when it doesn't have
artificially high levels of errors introduced by decoding choices.  The output
with ISLOW looks fine, and this is doubtless what the web author was looking at,
so it's reasonable to have used a JPEG here.

But even if it weren't, this basically says that because of an ideological
stance about which image formats work well for which applications, we're going
to intentionally allow Android to look bad, and to do so in a way desktop
doesn't.  This is much like complaining that people using animated GIFs should
really be using <video>, so we're not going to optimize the memory/CPU use of
animated GIFs.  It may be correct in principle, but it's our duty to make the
web work as well as possible in practice, even when practice is dumb.

I do think the right next step is to get more comprehensive performance numbers
so we have a better idea of what the actual practical perf impact here is.  With
that data in hand, we're better positioned to make choices about tradeoffs.

[no sievers, 2014-06-17 21:54:21]

On 2014/06/17 21:42:16, aelias wrote:
> The motivating use case misused JPEG to display a logo, which is the
> stereotypical case where PNG should've been used, so I'm not convinced it's
> worth taking a performance regression for that.  The claim that ISLOW can
> sometimes be faster is intriguing, but it sounds that at least one of your
test
> cases showed a 40% regression, which is scary.  Given what's been observed so
> far, I don't think the evidence supports making this change, although I could
be
> convinced by more systematic data.

A quick search on the web also suggests that IFAST and ISLOW might benefit very
differently from NEON (or other SIMD) optimizations depending on the exact
implementation. I assume there must be someone who has looked into the
implementation details wrt Android and knows better what the caveats are here.

[Noel Gordon, 2014-06-18 18:26:37]

Our libpeg_turbo has SIMD, and is also used for the Android WebView I believe,
which is not something we'd like to slow down.  JPEG is designed for
photographic images, no argument about that, but there are ways to make text
look better in JPEG.  I'd like to see better perf data.  I added some thoughts
to the bug https://code.google.com/p/chromium/issues/detail?id=385515#c4

[Peter Kasting, 2015-02-20 00:03:06]

This seems to have stalled; commented on the bug.

[Noel Gordon, 2015-03-16 12:45:39]

> This seems to have stalled; commented on the bug.

Saw this mail on return from vacation. One question might be: where is the
LOW_QUALITY_IMAGE_NO_JPEG_DITHERING flag defined? viz., which chrome builds
fiddle that flag?

[Peter Kasting, 2015-03-16 18:15:50]

On 2015/03/16 12:45:39, noel gordon wrote:
> > This seems to have stalled; commented on the bug.
> 
> Saw this mail on return from vacation. One question might be: where is the
> LOW_QUALITY_IMAGE_NO_JPEG_DITHERING flag defined? viz., which chrome builds
> fiddle that flag?

config.h turns it on for OS_ANDROID.  I doubt anyone else turns it on in their
compiler defines.

[Noel Gordon, 2015-03-18 00:31:51]

On 2015/03/16 18:15:50, Peter Kasting wrote:
> config.h turns it on for OS_ANDROID.  I doubt anyone else turns it on in their
> compiler defines.

Right. So one option here is to leave the JPEG code and flag alone, and instead,
unset the flag in config.h for OS_ANDROID.

[Peter Kasting, 2015-03-18 00:33:09]

On 2015/03/18 00:31:51, noel gordon wrote:
> On 2015/03/16 18:15:50, Peter Kasting wrote:
> > config.h turns it on for OS_ANDROID.  I doubt anyone else turns it on in
their
> > compiler defines.
> 
> Right. So one option here is to leave the JPEG code and flag alone, and
instead,
> unset the flag in config.h for OS_ANDROID.

The difference is that that would change the dithering as well.  I don't know
whether we want that.

If we do want that, we should just nuke the flag entirely and all the #ifs here.

[Noel Gordon, 2015-03-18 00:34:27]

> The difference is that that would change the dithering as well.  I don't know
> whether we want that.

Indeed. Let's me ask the bug about that.

[Noel Gordon, 2015-03-25 11:12:24]

Moved to https://codereview.chromium.org/1023193002