Support more swizzles to 565 in SkCodec

Support more swizzles to 565 in SkCodec

Add more swizzling functions for swizzling to 565. Much of this
code was revived from crrev.com/1055743003 (for BMP). Also added
swizzling functions for WBMP.

Consolidate the static function conversion_possible.

In SkCodec::getPixels, check that the alphatype corresponds to the
colorType. This prevents requesting 565 + non-opaque.

In SkIcoCodec, report that the image is unpremul (instead of
whatever the largest embedded codec thinks), but modify the
requested info to have the alpha type expected/required by the
embedded codec.

Add tests for decoding to 565.

BUG=skia:3257
BUG=skia:3683
Committed: https://skia.googlesource.com/skia/+/cc2feb161f756c4035a407296567654d86bc7be7

[scroggo_chromium, 2015-08-10 17:26:16]

scroggo@chromium.org changed reviewers:
+ emmaleer@google.com, msarett@google.com

[scroggo_chromium, 2015-08-10 17:26:16]

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:63: static bool conversion_possible(const SkImageInfo&
dst, const SkImageInfo& src) {
This function is a simplified version of the method that showed up in other
files.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:77: case kRGB_565_SkColorType:
This part is different - I've added support for 565, if the source is opaque.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:80: return dst.colorType() == src.colorType();
Some versions of conversion_possible checked specifically for index8. That is
covered by this check.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:302: // FIXME: I believe this is correct - still need
to test.
Oops. Will remove this in a followup

[msarett, 2015-08-10 17:44:24]

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:63: static bool conversion_possible(const SkImageInfo&
dst, const SkImageInfo& src) {
On 2015/08/10 17:26:16, scroggo_chromium wrote:
> This function is a simplified version of the method that showed up in other
> files.

Nice!  This is a big improvement.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkMaskSwizzle...
File src/codec/SkMaskSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkMaskSwizzle...
src/codec/SkMaskSwizzler.cpp:66: static SkSwizzler::ResultAlpha
swizzle_mask16_to_565(
Would you mind adding a TODO (msarett) here to improve this routine?  IMO, one
of the few reasonable uses of bmp mask mode is to an encode an image as 565.  I
think we ought to be able to recognize that the image is encoded as 565 and
swizzle directly to 565 rather than promote to 888 and then move back down to
565.

This is something that I neglected in the past that I think should be done.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:302: // FIXME: I believe this is correct - still need
to test.
On 2015/08/10 17:26:16, scroggo_chromium wrote:
> Oops. Will remove this in a followup

Acknowledged.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:473: switch (info.colorType()) {
I believe you need to add a 565 case to this switch.

[scroggo, 2015-08-10 17:50:45]

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

[scroggo, 2015-08-10 17:50:46]

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkMaskSwizzle...
File src/codec/SkMaskSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkMaskSwizzle...
src/codec/SkMaskSwizzler.cpp:66: static SkSwizzler::ResultAlpha
swizzle_mask16_to_565(
On 2015/08/10 17:44:24, msarett wrote:
> Would you mind adding a TODO (msarett) here to improve this routine?  IMO, one
> of the few reasonable uses of bmp mask mode is to an encode an image as 565. 
I
> think we ought to be able to recognize that the image is encoded as 565 and
> swizzle directly to 565 rather than promote to 888 and then move back down to
> 565.
> 
> This is something that I neglected in the past that I think should be done.

Done.

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:473: switch (info.colorType()) {
On 2015/08/10 17:44:24, msarett wrote:
> I believe you need to add a 565 case to this switch.

Yep. I added the proc swizzle_small_index_to_565 but never call it.

Fixed.

[msarett, 2015-08-11 12:35:42]

lgtm

[scroggo_chromium, 2015-08-12 14:15:47]

The CQ bit was checked by scroggo@chromium.org

[commit-bot: I haz the power, 2015-08-12 14:15:56]

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

[commit-bot: I haz the power, 2015-08-12 14:19:59]

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

[commit-bot: I haz the power, 2015-08-12 14:20:00]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[scroggo, 2015-08-13 14:45:05]

I had to fix an ICO problem (out of bounds write when decoding to 565), so
please take another look.

Sorry, I snuck a rebase in one of my uploads :(

The interesting changes since patch set 4 are:
- some FIXMEs (SkBmpCodec.cpp, SkCodec_libico.cpp
- SkBmpStandardCodec::onGetPixels asserts that the output is
  N32, when applying the alpha mask, since it treats the dst as
  SkPMColor
- SkCodec::getPixels now checks that the requested info's alpha
  and color types are consistent
- SkIcoCodec now checks valid_alpha and modifies the alpha it
  passes to its embedded codec, which may expect a different
  alpha
- SkSwizzler.cpp passes the new function pointer properly

[msarett, 2015-08-13 14:56:31]

lgtm

I'll spend some time attacking those FIXMEs.

[scroggo, 2015-08-13 18:09:43]

More changes, to add tests and support wbmp to 565

https://codereview.chromium.org/1277213002/diff/120001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/120001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:118: static SkSwizzler::ResultAlpha swizzle_bit_to_565(
This method is the same as the other swizzle_bit methods, except it has a
uint16_t* and sets it to RGB_BLACK or RGB_WHITE. It will need to be updated to
match the changes Emmalee has made to the other swizzle_bit methods.

https://codereview.chromium.org/1277213002/diff/120001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1277213002/diff/120001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:82: bool supports565 = true) {
This feels like a bit of a hack. I had assumed that we could always decode to
565, but it turns out we cannot for CMYK. It seems like we could use the
swizzler to get 565, once Emmalee's change goes in that uses the swizzler in
JPEG for sampling.

[msarett, 2015-08-13 18:16:18]

https://codereview.chromium.org/1277213002/diff/120001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1277213002/diff/120001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:82: bool supports565 = true) {
On 2015/08/13 18:09:43, scroggo wrote:
> This feels like a bit of a hack. I had assumed that we could always decode to
> 565, but it turns out we cannot for CMYK. It seems like we could use the
> swizzler to get 565, once Emmalee's change goes in that uses the swizzler in
> JPEG for sampling.

Agreed, however I feel fine about this.  I'm not really thrilled with how we
support CMYK to RGBA either.  Improving CMYK->RGB formats is a TODO for me. 
Feel free to add a TODO (msarett) here to fix this.

[scroggo_chromium, 2015-08-14 15:17:23]

Please take one final look at this CL before I land. After a rebase, only a few
changes were needed.

https://codereview.chromium.org/1277213002/diff/160001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/160001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:130: dst[0] = ((currByte >> (7 - bitIndex)) & 1) ?
RGB565_WHITE : RGB565_BLACK;
I think this function looks just like swizzle_bit_to_n32, except for the color
macros and type of dst. Let me know if I missed something.

[msarett, 2015-08-14 15:21:06]

lgtm

https://codereview.chromium.org/1277213002/diff/160001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1277213002/diff/160001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:130: dst[0] = ((currByte >> (7 - bitIndex)) & 1) ?
RGB565_WHITE : RGB565_BLACK;
On 2015/08/14 15:17:23, scroggo_chromium wrote:
> I think this function looks just like swizzle_bit_to_n32, except for the color
> macros and type of dst. Let me know if I missed something.

Looks good.

[scroggo, 2015-08-14 15:25:48]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-08-14 15:25:54]

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

[commit-bot: I haz the power, 2015-08-14 15:32:49]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/cc2feb161f756c4035a407296567654d86bc7be7