SkPNGImageEncoder encodes all SkColorTypes

SkPNGImageEncoder encodes all SkColorTypes

Committed: https://skia.googlesource.com/skia/+/dfd6c6e3bb8c951fcd2bf85995629f1c8ef76590

[hal.canary, 2015-12-07 00:07:37]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 00:07:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506663002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506663002/1

[hal.canary, 2015-12-07 00:08:38]

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

[hal.canary, 2015-12-07 00:08:39]

PTAL

[commit-bot: I haz the power, 2015-12-07 00:21:31]

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

[commit-bot: I haz the power, 2015-12-07 00:21:32]

Dry run: This issue passed the CQ dry run.

[scroggo, 2015-12-07 17:05:45]

> SkPNGImageEncoder encodes all SkColorTypes

nit: I think this is a worthy change on its own, but maybe include motivation
for it in the description? Or a bug, if there is one?

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libpng.cpp:834: bool useCopy = false;
I don't think this variable is necessary. Instead you could say something like:

SkBitmap bmStorage;
const SkBitmap* bitmap;

switch (ct) {
  // All your cases:
    bitmap = &originalBitmap;
    break;
  default:
    copyTo();
    bitmap = &bitmapStorage;
    break;
}

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libpng.cpp:841: default:
So basically, what we are now supporting are:

- The other swizzle for N32 (the ironic thing here is that we might copy to the
opposite of what libpng wants - maybe add a FIXME?)
- Gray (again, this probably deserves a FIXME - we could actually encoded it to
gray, rather than copying)
- Alpha8 (not really sure what to do here, but maybe copying to N32 is as good
as anything? Android uses ALPHA_8 to mean gray, since gray did not exist when
they wanted it; maybe it would be interesting to abuse gray here? not sure...)

[hal.canary, 2015-12-07 21:34:38]

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libpng.cpp:834: bool useCopy = false;
On 2015/12/07 at 17:05:45, scroggo wrote:
> I don't think this variable is necessary. Instead you could say something
like:
> 
> SkBitmap bmStorage;
> const SkBitmap* bitmap;
> 
> switch (ct) {
>   // All your cases:
>     bitmap = &originalBitmap;
>     break;
>   default:
>     copyTo();
>     bitmap = &bitmapStorage;
>     break;
> }

done.

https://codereview.chromium.org/1506663002/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libpng.cpp:841: default:
On 2015/12/07 at 17:05:45, scroggo wrote:
> So basically, what we are now supporting are:
> 
> - The other swizzle for N32 (the ironic thing here is that we might copy to
the opposite of what libpng wants - maybe add a FIXME?)
> - Gray (again, this probably deserves a FIXME - we could actually encoded it
to gray, rather than copying)
> - Alpha8 (not really sure what to do here, but maybe copying to N32 is as good
as anything? Android uses ALPHA_8 to mean gray, since gray did not exist when
they wanted it; maybe it would be interesting to abuse gray here? not sure...)

I added TODOs and assigned them to you.

[scroggo, 2015-12-07 21:44:24]

lgtm

[hal.canary, 2015-12-07 21:47:21]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-12-07 21:47:26]

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

[commit-bot: I haz the power, 2015-12-07 22:07:34]

Description was changed from

==========
SkPNGImageEncoder encodes all SkColorTypes
==========

to

==========
SkPNGImageEncoder encodes all SkColorTypes

Committed:
https://skia.googlesource.com/skia/+/dfd6c6e3bb8c951fcd2bf85995629f1c8ef76590
==========

[commit-bot: I haz the power, 2015-12-07 22:07:34]

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