Fix colorType/alphaType checks in SkCodec

Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

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

[msarett, 2016-02-11 18:08:11]

Description was changed from

==========
Expect creating the swizzler to always succeed

BUG=skia:4203
==========

to

==========
Expect creating the swizzler to always succeed

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

[msarett, 2016-02-11 18:10:36]

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

[scroggo, 2016-02-11 18:39:53]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-02-11 18:39:53]

lgtm

[commit-bot: I haz the power, 2016-02-11 18:40:00]

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

[commit-bot: I haz the power, 2016-02-11 18:40:52]

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

[commit-bot: I haz the power, 2016-02-11 18:40:52]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[msarett, 2016-02-11 19:13:45]

PTAL
The trybots caught a non-trivial error with wbmp.

https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.cpp
File src/codec/SkWbmpCodec.cpp (right):

https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.c...
src/codec/SkWbmpCodec.cpp:37: case kGray_8_SkColorType:
Should kGray_8 also require that the alphaType is opaque?  If so, we should
change the version in SkCodecPriv.h as well.

[msarett, 2016-02-12 16:46:26]

Added code to consistently require that kGray8 decodes be opaque.

Also removed redundant code from SkCodec.cpp.

[msarett, 2016-02-12 16:47:28]

Description was changed from

==========
Expect creating the swizzler to always succeed

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

to

==========
Require that decodes to kGray8 be opaque

Also, assert that creating the swizzler always succeeds

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

[scroggo, 2016-02-12 17:13:55]

-lgtm at patch set 3

https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.cpp
File src/codec/SkWbmpCodec.cpp (right):

https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.c...
src/codec/SkWbmpCodec.cpp:37: case kGray_8_SkColorType:
On 2016/02/11 19:13:45, msarett wrote:
> Should kGray_8 also require that the alphaType is opaque?  If so, we should
> change the version in SkCodecPriv.h as well.

Not sure - this means that they have an inconsistent target...

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (left):

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp#o...
src/codec/SkCodec.cpp:171: if (!SkColorTypeValidateAlphaType(info.colorType(),
info.alphaType(), &canonical)
What is this redundant with? I started to remove this recently, but convinced
myself we still wanted it.

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

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:139: case kGray_8_SkColorType:
So if the Source is N32 and has no alpha, we will support converting to gray? Is
that true for all formats? For JPEG I understand that we can (although shouldn't
we be careful in case clients request ALPHA_8 meaning to get ALPHA_8 only if
it's gray?), but I think this is not true for all formats?

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

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.c...
src/codec/SkWbmpCodec.cpp:40: return kOpaque_SkAlphaType == alphaType;
Here you're checking the dst instead of the src, unlike in SkCodecPriv.

It's also weird that someone would request you to decode to 565 premul, but
maybe it's fine?

Similar for Gray...?

[scroggo, 2016-02-12 17:14:03]

On 2016/02/12 17:13:55, scroggo wrote:
> -lgtm at patch set 3
> 
>
https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.cpp
> File src/codec/SkWbmpCodec.cpp (right):
> 
>
https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.c...
> src/codec/SkWbmpCodec.cpp:37: case kGray_8_SkColorType:
> On 2016/02/11 19:13:45, msarett wrote:
> > Should kGray_8 also require that the alphaType is opaque?  If so, we should
> > change the version in SkCodecPriv.h as well.
> 
> Not sure - this means that they have an inconsistent target...
> 
> https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp
> File src/codec/SkCodec.cpp (left):
> 
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp#o...
> src/codec/SkCodec.cpp:171: if (!SkColorTypeValidateAlphaType(info.colorType(),
> info.alphaType(), &canonical)
> What is this redundant with? I started to remove this recently, but convinced
> myself we still wanted it.
> 
> https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h
> File src/codec/SkCodecPriv.h (right):
> 
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h...
> src/codec/SkCodecPriv.h:139: case kGray_8_SkColorType:
> So if the Source is N32 and has no alpha, we will support converting to gray?
Is
> that true for all formats? For JPEG I understand that we can (although
shouldn't
> we be careful in case clients request ALPHA_8 meaning to get ALPHA_8 only if
> it's gray?), but I think this is not true for all formats?
> 
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.cpp
> File src/codec/SkWbmpCodec.cpp (right):
> 
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.c...
> src/codec/SkWbmpCodec.cpp:40: return kOpaque_SkAlphaType == alphaType;
> Here you're checking the dst instead of the src, unlike in SkCodecPriv.
> 
> It's also weird that someone would request you to decode to 565 premul, but
> maybe it's fine?
> 
> Similar for Gray...?

not lgtm

[scroggo, 2016-02-12 17:14:23]

On 2016/02/12 17:14:03, scroggo wrote:
> On 2016/02/12 17:13:55, scroggo wrote:
> > -lgtm at patch set 3
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.cpp
> > File src/codec/SkWbmpCodec.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/20001/src/codec/SkWbmpCodec.c...
> > src/codec/SkWbmpCodec.cpp:37: case kGray_8_SkColorType:
> > On 2016/02/11 19:13:45, msarett wrote:
> > > Should kGray_8 also require that the alphaType is opaque?  If so, we
should
> > > change the version in SkCodecPriv.h as well.
> > 
> > Not sure - this means that they have an inconsistent target...
> > 
> > https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp
> > File src/codec/SkCodec.cpp (left):
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp#o...
> > src/codec/SkCodec.cpp:171: if
(!SkColorTypeValidateAlphaType(info.colorType(),
> > info.alphaType(), &canonical)
> > What is this redundant with? I started to remove this recently, but
convinced
> > myself we still wanted it.
> > 
> >
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h
> > File src/codec/SkCodecPriv.h (right):
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h...
> > src/codec/SkCodecPriv.h:139: case kGray_8_SkColorType:
> > So if the Source is N32 and has no alpha, we will support converting to
gray?
> Is
> > that true for all formats? For JPEG I understand that we can (although
> shouldn't
> > we be careful in case clients request ALPHA_8 meaning to get ALPHA_8 only if
> > it's gray?), but I think this is not true for all formats?
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.cpp
> > File src/codec/SkWbmpCodec.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.c...
> > src/codec/SkWbmpCodec.cpp:40: return kOpaque_SkAlphaType == alphaType;
> > Here you're checking the dst instead of the src, unlike in SkCodecPriv.
> > 
> > It's also weird that someone would request you to decode to 565 premul, but
> > maybe it's fine?
> > 
> > Similar for Gray...?
> 
> not lgtm

(I thought "-" would have the same effect as "not"...)

[msarett, 2016-02-12 17:41:31]

Changing the commit message as well

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (left):

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp#o...
src/codec/SkCodec.cpp:171: if (!SkColorTypeValidateAlphaType(info.colorType(),
info.alphaType(), &canonical)
On 2016/02/12 17:13:55, scroggo wrote:
> What is this redundant with? I started to remove this recently, but convinced
> myself we still wanted it.

This just checks that the alphaType matches the colorType.

So we return kInvalidConversion if k565 or kGray is not matched with kOpaque.  I
think that if conversion_possible handles this check, then we don't need it
here.

If we want to handle it here, startScanlineDecode() is missing this check.

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

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:139: case kGray_8_SkColorType:
On 2016/02/12 17:13:55, scroggo wrote:
> So if the Source is N32 and has no alpha, we will support converting to gray?
Is
> that true for all formats? For JPEG I understand that we can (although
shouldn't
> we be careful in case clients request ALPHA_8 meaning to get ALPHA_8 only if
> it's gray?), but I think this is not true for all formats?

Oops this is not what I meant to do.  Just want to require that gray decodes are
opaque.

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

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.c...
src/codec/SkWbmpCodec.cpp:40: return kOpaque_SkAlphaType == alphaType;
On 2016/02/12 17:13:55, scroggo wrote:
> Here you're checking the dst instead of the src, unlike in SkCodecPriv.
> 
> It's also weird that someone would request you to decode to 565 premul, but
> maybe it's fine?
> 
> Similar for Gray...?

I think it makes sense to disable it.  Let me know if you feel otherwise.  Right
now we disable it for getPixels(), but Gray/Premul can sneak through on scanline
decodes...

[msarett, 2016-02-12 17:42:56]

Description was changed from

==========
Require that decodes to kGray8 be opaque

Also, assert that creating the swizzler always succeeds

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

to

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

[scroggo, 2016-02-12 17:59:00]

Maybe we should move SkColorTypeValidateAlphaType into conversion_possible?

lgtm

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (left):

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkCodec.cpp#o...
src/codec/SkCodec.cpp:171: if (!SkColorTypeValidateAlphaType(info.colorType(),
info.alphaType(), &canonical)
On 2016/02/12 17:41:31, msarett wrote:
> On 2016/02/12 17:13:55, scroggo wrote:
> > What is this redundant with? I started to remove this recently, but
convinced
> > myself we still wanted it.
> 
> This just checks that the alphaType matches the colorType.
> 
> So we return kInvalidConversion if k565 or kGray is not matched with kOpaque. 
I
> think that if conversion_possible handles this check, then we don't need it
> here.
> 
> If we want to handle it here, startScanlineDecode() is missing this check.

Ah, got it. So you're just moving this into a common place where it's called by
startScanlineDecode.

It might be nice to leave it here (and include it for scanline decoding), so
when this gets updated codec gets updated for free. I don't feel strongly about
that though.

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

https://codereview.chromium.org/1695473002/diff/40001/src/codec/SkWbmpCodec.c...
src/codec/SkWbmpCodec.cpp:40: return kOpaque_SkAlphaType == alphaType;
On 2016/02/12 17:41:31, msarett wrote:
> On 2016/02/12 17:13:55, scroggo wrote:
> > Here you're checking the dst instead of the src, unlike in SkCodecPriv.
> > 
> > It's also weird that someone would request you to decode to 565 premul, but
> > maybe it's fine?
> > 
> > Similar for Gray...?
> 
> I think it makes sense to disable it.  Let me know if you feel otherwise. 
Right
> now we disable it for getPixels(), but Gray/Premul can sneak through on
scanline
> decodes...

Ah, I see now. I would lean towards calling the validate method for scanline
decodes. (Should we have tests for this?)

[msarett, 2016-02-12 18:22:02]

I would lean toward doing the checking in conversion_possible (vs doing it in
getPixels() and getScanlines()).

I kind of have the opposite opinion on SkColorTypeValidateAlpha.  Every time I
see it, I forget what it does, and then I need to read through it to try to
figure out which of the checks that it performs are relevant for us. 
Additionally, it actually makes me nervous that it may change (and change the
behavior of SkCodec).

[msarett, 2016-02-12 18:22:18]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-12 18:22:20]

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

[commit-bot: I haz the power, 2016-02-12 18:27:25]

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

[commit-bot: I haz the power, 2016-02-12 18:27:26]

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...)

[msarett, 2016-02-12 20:40:02]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-12 20:40:02]

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/1695473002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-02-12 20:40:07]

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

[commit-bot: I haz the power, 2016-02-12 21:07:17]

Description was changed from

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

to

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

[commit-bot: I haz the power, 2016-02-12 21:07:18]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/c7578b6cdd03b61f076ffc7956efd952d6c301c0

[msarett, 2016-02-12 22:59:51]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1694023002/ by msarett@google.com.

The reason for reverting is: Really bad images in Gold..

[msarett, 2016-02-15 17:30:26]

Description was changed from

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

to

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

[msarett, 2016-02-15 17:57:06]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-15 17:57:06]

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/1695473002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-15 17:57:10]

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

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

Description was changed from

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

to

==========
Fix colorType/alphaType checks in SkCodec

Make getPixels() and startScanlineDecode() behave
consistently.

Require that kGray8 decodes are opaque.

Assert that creating the swizzler succeeds.

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

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

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

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

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/b30d6984e6dfc185bf1eebf927343563057a7bb3