Disable dithering in libjpeg-turbo for 565 decodes

Disable dithering in libjpeg-turbo for 565 decodes

libjpeg-turbo turns on dithering by default.  When we
decode to RGBA, it uses Floyd-Steinberg dithering -
consistent with the libjpeg6b gold standard,

When we decode to 565, it uses ordered dithering.  Ordered
dithering for 565 is not part of the 6b standard (libjpeg6b
doesn't even support 565) and is causing us a number of
issues:
(1) Ordered dithering + nearest neighbor sampling is
causing checkerboard visual artifacts in some outputs.
(2) The ordered dither function in libjpeg-turbo actually
behaves differently depending on the alignment of the
memory that it decodes into.  This means that two image
decodes that should be identical may look different just
because they decode into different memory blocks.  This
was causing some diffs on Gold with the scanline_subset
test that were causing me some confusion.
(3) Maybe not the best evidence, but visually I can't tell
a difference with and without dithering (except when
nearest neighbor scaling causes the checkerboard artifact).
(4) Turning off dithering should be a more significant
performance improvement than you might expect.
libjpeg-turbo has SIMD color conversions to 565, but when
dithering is on, it defaults to scalar code.

This CL should make every jpeg decode to 565 on Gold look
slightly different.  Yay!

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8ff6ca6e73644b0bec65c9dfeaa5ca01110d948c

[msarett, 2015-09-18 14:16:18]

msarett@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-09-18 15:42:41]

First off, I want to say that this is a perfect CL description. It describes
exactly what you're changing and why (along with any doubts).

> Disable dithering in libjpeg-turbo for 565 decodes
> 
> libjpeg-turbo turns on dithering by default.  When we
> decode to RGBA, it uses Floyd-Steinberg dithering -
> consistent with the libjpeg6b gold standard,
> 
> When we decode to 565, it uses ordered dithering.  Ordered
> dithering for 565 is not part of the 6b standard (libjpeg6b
> doesn't even support 565) and is causing us a number of
> issues:
> (1) Ordered dithering + nearest neighbor sampling is
> causing checkerboard visual artifacts in some outputs.
> (2) The ordered dither function in libjpeg-turbo actually
> behaves differently depending on the alignment of the
> memory that it decodes into.  This means that two image
> decodes that should be identical may look different just
> because they decode into different memory blocks.  This
> was causing some diffs on Gold with the scanline_subset
> test that were causing me some confusion.

That's strange. Worth filing a bug against libjpeg-turbo?

> (3) Maybe not the best evidence, but visually I can't tell
> a difference with and without dithering.

Except when there is strange checkerboarding :P

But seriously, I think it is good evidence. (The question is whether it's
enough!) I generally haven't noticed a difference, and I don't know that anyone
will, hence the general decision to not provide an option to dither in SkCodec.
From looking in our code base, it's not clear that dithering is used very often.
My favorite comment in is in NinePatchDrawable.java states that the default is
true because it helps a lot and it is cheap, but the variable is set to false.
(The CL that changed that, ag/318102, gives no indication as to why it changed.
I'm guessing the dithering didn't end up looking right after the image was
stretched.) Otherwise it's typically set to false, even when the config is set
to 8888, when the variable is ignored, hinting that the author of that code
doesn't know what the variable does. I found one instance of code (again,
internally - who knows what external developers are doing?) where they seem to
have deliberately used dithering + 565 in AcelerometerPlayActivity.java
(although it's a sample...).


> (4) Turning off dithering should be a more significant
> performance improvement than you might expect.
> libjpeg-turbo has SIMD color conversions to 565, but when
> dithering is on, it defaults to scalar code.

That is excellent motivation for turning it off, especially since the quality
difference appears to be negligible.

> 
> This CL should make every jpeg decode to 565 on Gold look
> slightly different.  Yay!

lol

I just have one concern about state. Once that is addressed I think this is
ready.

https://codereview.chromium.org/1349563007/diff/1/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1349563007/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:251: // best to do this in SkSwizzler and also move
convert_CMYK_to_RGBA into SkSwizzler.
That's an interesting idea!

https://codereview.chromium.org/1349563007/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:254: fDecoderMgr->dinfo()->dither_mode = JDITHER_NONE;
Does this get reset to the appropriate value if someone decodes to 565 and then
decodes to 8888? (Or does it matter? Does this mean it will no longer use FS?)

(You could write a test for this which decodes to 8888 and creates a checksum,
decodes to 565, decodes to 8888 again and compares the checksum.)

[msarett, 2015-09-18 18:15:08]

“First off, I want to say that this is a perfect CL description. It describes
exactly what you're changing and why (along with any doubts).”

Thanks :)

“That's strange. Worth filing a bug against libjpeg-turbo?”

Probably is, at least to make them aware.  I will do this.

“They seem to have deliberately used dithering + 565 in
AcelerometerPlayActivity.java”

Hmm yeah that’s interesting.  I don’t think it’s enough to convince us to keep
the dithering option, but it’s worth being aware of.

https://codereview.chromium.org/1349563007/diff/1/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1349563007/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:254: fDecoderMgr->dinfo()->dither_mode = JDITHER_NONE;
On 2015/09/18 15:42:41, scroggo wrote:
> Does this get reset to the appropriate value if someone decodes to 565 and
then
> decodes to 8888? (Or does it matter? Does this mean it will no longer use FS?)
> 
> (You could write a test for this which decodes to 8888 and creates a checksum,
> decodes to 565, decodes to 8888 again and compares the checksum.)

I think so.  Adding a test to confirm.

[scroggo, 2015-09-18 18:35:30]

lgtm

https://codereview.chromium.org/1349563007/diff/20001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1349563007/diff/20001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:83: bool supports565 = true) {
Do all the callers just use the default value of true? Maybe remove this
variable?

(This came up because I wanted to make sure this test wasn't passing due to luck
- if the decode to 565 never happened, the redecode would not be interesting.)

[msarett, 2015-09-18 18:59:26]

https://codereview.chromium.org/1349563007/diff/20001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1349563007/diff/20001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:83: bool supports565 = true) {
On 2015/09/18 18:35:30, scroggo wrote:
> Do all the callers just use the default value of true? Maybe remove this
> variable?
> 
> (This came up because I wanted to make sure this test wasn't passing due to
luck
> - if the decode to 565 never happened, the redecode would not be interesting.)

We can almost remove this.

CMYK.jpg still needs to support 565.  I will remove this when I make that fix.

[msarett, 2015-09-18 18:59:31]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-09-18 18:59:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1349563007/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1349563007/20001

[commit-bot: I haz the power, 2015-09-18 19:06:07]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/8ff6ca6e73644b0bec65c9dfeaa5ca01110d948c