Support more color types for ICOs

Support more color types for ICOs

BUG=skia:4620
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1694513002

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

[msarett, 2016-02-11 16:41:51]

Description was changed from

==========
Support more color types for ICOs

BUG=skia:4620
==========

to

==========
Support more color types for ICOs

BUG=skia:4620
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-02-11 16:50:15]

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

[msarett, 2016-02-11 16:50:15]

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

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpCodec.cpp#ne...
src/codec/SkBmpCodec.cpp:478: bool isOpaque = kOpaque_SkAlphaType == alphaType;
Here we need to remember if the embedded BMP is actually opaque.  Since we will
always suggest kUnpremul for BMP-in-ICOs.

Another approach would be handle this in SkIcoCodec similar to how we did
before:

if (embeddedCodec is a bmp) {
    recommendedInfo = recommendedInfo.makeAlphaType(kUnpremul);
}

This would be simpler than the current code (no modifications to SkBmpCodec).

The drawback would be that we would print warnings on Ico decodes.
Ex: "BMP said it was opaque but is being decoded as if it is kUnpremul/Premul"

[scroggo, 2016-02-11 18:05:13]

lgtm

Should we have any tests for this? Or this will be covered by DM/Gold?

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

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpCodec.cpp#ne...
src/codec/SkBmpCodec.cpp:478: bool isOpaque = kOpaque_SkAlphaType == alphaType;
On 2016/02/11 16:50:15, msarett wrote:
> Here we need to remember if the embedded BMP is actually opaque.  Since we
will
> always suggest kUnpremul for BMP-in-ICOs.
> 
> Another approach would be handle this in SkIcoCodec similar to how we did
> before:
> 
> if (embeddedCodec is a bmp) {
>     recommendedInfo = recommendedInfo.makeAlphaType(kUnpremul);
> }
> 
> This would be simpler than the current code (no modifications to SkBmpCodec).
> 
> The drawback would be that we would print warnings on Ico decodes.
> Ex: "BMP said it was opaque but is being decoded as if it is kUnpremul/Premul"

This seems simple enough to me.

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpStandardCodec.h
File src/codec/SkBmpStandardCodec.h (right):

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpStandardCode...
src/codec/SkBmpStandardCodec.h:38: * @param inIco    indicates if the bmp is
embedded in an ico file
nit: inIco here, and isIco down below. I don't have a strong preference, but
they should match

[msarett, 2016-02-11 18:15:47]

This should be covered by DM/Gold.  All of my PNG-in-ICOs seem to be RGBA, so I
don't think we'll see any new/untriaged images.

But seeing no changes in Gold should confirm that this works.

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

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpCodec.cpp#ne...
src/codec/SkBmpCodec.cpp:478: bool isOpaque = kOpaque_SkAlphaType == alphaType;
On 2016/02/11 18:05:13, scroggo wrote:
> On 2016/02/11 16:50:15, msarett wrote:
> > Here we need to remember if the embedded BMP is actually opaque.  Since we
> will
> > always suggest kUnpremul for BMP-in-ICOs.
> > 
> > Another approach would be handle this in SkIcoCodec similar to how we did
> > before:
> > 
> > if (embeddedCodec is a bmp) {
> >     recommendedInfo = recommendedInfo.makeAlphaType(kUnpremul);
> > }
> > 
> > This would be simpler than the current code (no modifications to
SkBmpCodec).
> > 
> > The drawback would be that we would print warnings on Ico decodes.
> > Ex: "BMP said it was opaque but is being decoded as if it is
kUnpremul/Premul"
> 
> This seems simple enough to me.

Acknowledged.

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpStandardCodec.h
File src/codec/SkBmpStandardCodec.h (right):

https://codereview.chromium.org/1694513002/diff/1/src/codec/SkBmpStandardCode...
src/codec/SkBmpStandardCodec.h:38: * @param inIco    indicates if the bmp is
embedded in an ico file
On 2016/02/11 18:05:13, scroggo wrote:
> nit: inIco here, and isIco down below. I don't have a strong preference, but
> they should match

Using inIco.

[msarett, 2016-02-11 18:25:56]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-11 18:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1694513002/#ps20001
(title: "inIco")

[commit-bot: I haz the power, 2016-02-11 18:26:06]

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

[commit-bot: I haz the power, 2016-02-11 18:49:33]

Description was changed from

==========
Support more color types for ICOs

BUG=skia:4620
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Support more color types for ICOs

BUG=skia:4620
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

[commit-bot: I haz the power, 2016-02-11 18:49:34]

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