Finish supporting decoding opaque to non-opaque

Finish supporting decoding opaque to non-opaque

When decoding to 565 or Gray, allow the client to incorrectly ask for premul.

When checking whether it's possible to decode to 565, return whether the
source is opaque.

In DM, allow decoding to 565 or Gray, even if the client also asked for premul.

This fixes a bug introduced in crrev.com/1999593003 when we stopped ever
requesting Opaque, resulting in us not testing 565 or Gray.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003

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

[scroggo_chromium, 2016-05-20 18:48:32]

Description was changed from

==========
Support decoding wbmp to non-opaque

Just as we made the other codecs support asking for unpremul or
premul when the image is opaque, wbmp should behave the same.

Also fix another bug where we never tested the generator to Gray
because we required Opaque. (This got broken in crrev.com/1999593003
when we stopped ever requesting Opaque.)

BUG=skia:4616
==========

to

==========
Support decoding wbmp to non-opaque

Just as we made the other codecs support asking for unpremul or
premul when the image is opaque, wbmp should behave the same.

Also fix another bug where we never tested the generator to Gray
because we required Opaque. (This got broken in crrev.com/1999593003
when we stopped ever requesting Opaque.)

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

[scroggo, 2016-05-20 18:51:23]

Description was changed from

==========
Support decoding wbmp to non-opaque

Just as we made the other codecs support asking for unpremul or
premul when the image is opaque, wbmp should behave the same.

Also fix another bug where we never tested the generator to Gray
because we required Opaque. (This got broken in crrev.com/1999593003
when we stopped ever requesting Opaque.)

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

to

==========
Support decoding wbmp to non-opaque

Just as we made the other codecs support asking for unpremul or
premul when the image is opaque, wbmp should behave the same.

Also fix another bug where we never tested the generator to Gray
because we required Opaque. (This got broken in crrev.com/1999593003
when we stopped ever requesting Opaque.) Now that we will only decode
an Opaque image to Unpremul, we no longer need the code that returns a
Nonfatal error for decoding to the wrong alpha type, since we will only
attempt one.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

[scroggo, 2016-05-20 18:52:42]

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

[msarett, 2016-05-20 18:58:01]

I think you've hit on something bigger than this...

For all of our codecs, I believe that we only support 565/Gray if the alphaType
is opaque.  I remember pushing for this behavior.  565/Gray with any other alpha
type doesn't make much sense to me.

By deleting our opaque->opaque tests, we've probably deleted all of our tests to
565 and to Gray.  Uggh.

[scroggo, 2016-05-20 18:59:13]

On 2016/05/20 18:58:01, msarett wrote:
> I think you've hit on something bigger than this...
> 
> For all of our codecs, I believe that we only support 565/Gray if the
alphaType
> is opaque.  I remember pushing for this behavior.  565/Gray with any other
alpha
> type doesn't make much sense to me.
> 
> By deleting our opaque->opaque tests, we've probably deleted all of our tests
to
> 565 and to Gray.  Uggh.

[scroggo, 2016-05-20 19:29:15]

On 2016/05/20 18:58:01, msarett wrote:
> I think you've hit on something bigger than this...
> 
> For all of our codecs, I believe that we only support 565/Gray if the
alphaType
> is opaque.  I remember pushing for this behavior.  565/Gray with any other
alpha
> type doesn't make much sense to me.
> 
> By deleting our opaque->opaque tests, we've probably deleted all of our tests
to
> 565 and to Gray.  Uggh.

(Sorry, I spammed the click button and sent this too quickly.)

Shoot. You are partially right. You did push for that decision, but we
ultimately reversed it. Look at the bug in the description: skbug.com/4616. Now
we support asking for premul/unpremul even if the true type is opaque. Or at
least, we're supposed to. Still more work to be done here....

[msarett, 2016-05-20 19:37:41]

On 2016/05/20 19:29:15, scroggo wrote:
> On 2016/05/20 18:58:01, msarett wrote:
> > I think you've hit on something bigger than this...
> > 
> > For all of our codecs, I believe that we only support 565/Gray if the
> alphaType
> > is opaque.  I remember pushing for this behavior.  565/Gray with any other
> alpha
> > type doesn't make much sense to me.
> > 
> > By deleting our opaque->opaque tests, we've probably deleted all of our
tests
> to
> > 565 and to Gray.  Uggh.
> 
> (Sorry, I spammed the click button and sent this too quickly.)
> 
> Shoot. You are partially right. You did push for that decision, but we
> ultimately reversed it. Look at the bug in the description: skbug.com/4616.
Now
> we support asking for premul/unpremul even if the true type is opaque. Or at
> least, we're supposed to. Still more work to be done here....

Yeah I remember that issue.  That's why we now allow N32 decode to be
premul/unpremul/opaque.  I may have caused the current confusion by continuing
to hold out on kGray/k565.  Here are a few interesting sections of code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/d...

[scroggo, 2016-05-20 19:50:17]

Description was changed from

==========
Support decoding wbmp to non-opaque

Just as we made the other codecs support asking for unpremul or
premul when the image is opaque, wbmp should behave the same.

Also fix another bug where we never tested the generator to Gray
because we required Opaque. (This got broken in crrev.com/1999593003
when we stopped ever requesting Opaque.) Now that we will only decode
an Opaque image to Unpremul, we no longer need the code that returns a
Nonfatal error for decoding to the wrong alpha type, since we will only
attempt one.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

to

==========
Finish supporting decoding opaque to non-opaque

When decoding to 565 or Gray, allow the client to incorrectly ask for premul.

When checking whether it's possible to decode to 565, return whether the
source is opaque.

In DM, allow decoding to 565 or Gray, even if the client also asked for premul.

This fixes a bug introduced in crrev.com/1999593003 when we stopped ever
requesting Opaque, resulting in us not testing 565 or Gray.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

[scroggo, 2016-05-20 20:22:04]

Alright, this will revive back our testing to 565, and I added tests that 565 is
supported. We should probably add a gray PNG, but the test covers wbmp and jpeg,
at least.

As an alternative, if we still think it's correct to forbid gray/565 with
non-opaque, we *could* rejigger DM to request the proper alpha type. But I think
allowing it (as in this patch) is the right approach. As a reference point,
SkColorTypeValidateAlphaType corrects 565/gray non-opaque to opaque, and
SkBitmap::setInfo uses it to do the same, rather than failing.

[scroggo, 2016-05-20 20:39:08]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2016-05-20 20:39:22]

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

[commit-bot: I haz the power, 2016-05-20 20:39:23]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-05-21 02:39 UTC

[msarett, 2016-05-20 20:43:58]

On 2016/05/20 20:22:04, scroggo wrote:
> Alright, this will revive back our testing to 565, and I added tests that 565
is
> supported. We should probably add a gray PNG, but the test covers wbmp and
jpeg,
> at least.
> 
> As an alternative, if we still think it's correct to forbid gray/565 with
> non-opaque, we *could* rejigger DM to request the proper alpha type. But I
think
> allowing it (as in this patch) is the right approach. As a reference point,
> SkColorTypeValidateAlphaType corrects 565/gray non-opaque to opaque, and
> SkBitmap::setInfo uses it to do the same, rather than failing.

I agree this is right to be consistent with Skia.  Though I still disagree haha.

Thanks for making this change.  It looks good, except I think we're still not
running 565 and Gray8 in DMSrcSink?

[scroggo, 2016-05-20 20:50:03]

On 2016/05/20 20:43:58, msarett wrote:
> On 2016/05/20 20:22:04, scroggo wrote:
> > Alright, this will revive back our testing to 565, and I added tests that
565
> is
> > supported. We should probably add a gray PNG, but the test covers wbmp and
> jpeg,
> > at least.
> > 
> > As an alternative, if we still think it's correct to forbid gray/565 with
> > non-opaque, we *could* rejigger DM to request the proper alpha type. But I
> think
> > allowing it (as in this patch) is the right approach. As a reference point,
> > SkColorTypeValidateAlphaType corrects 565/gray non-opaque to opaque, and
> > SkBitmap::setInfo uses it to do the same, rather than failing.
> 
> I agree this is right to be consistent with Skia.  Though I still disagree
haha.
> 
> Thanks for making this change.  It looks good, except I think we're still not
> running 565 and Gray8 in DMSrcSink?

Where do you see that? Running locally, they both work. Maybe it's a little bit
sneaky, but I changed the callers of get_decode_info to *not* change the alpha
type first. So the method now compares against the src alpha type, and then it
modifies to the dst alpha type (which won't be opaque) afterwards.

[msarett, 2016-05-20 20:52:22]

Ahhh ok I understand now.  I think it would be a little more clear if you remove
those checks for kOpaque (since they are useless now anyway).

lgtm

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:327: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
Don't we need to fix this?

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:344: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
Don't we need to fix this?

[msarett, 2016-05-20 20:53:21]

On 2016/05/20 20:52:22, msarett wrote:
> Ahhh ok I understand now.  I think it would be a little more clear if you
remove
> those checks for kOpaque (since they are useless now anyway).
> 
> lgtm
> 
> https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp
> File dm/DMSrcSink.cpp (right):
> 
>
https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
> dm/DMSrcSink.cpp:327: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
> Don't we need to fix this?
> 
>
https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
> dm/DMSrcSink.cpp:344: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
> Don't we need to fix this?

Sorry I thought my inline comments were published a few messages ago...

[scroggo, 2016-05-20 20:54:29]

The CQ bit was unchecked by scroggo@google.com

[scroggo, 2016-05-20 20:55:03]

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:327: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
On 2016/05/20 20:52:22, msarett wrote:
> Don't we need to fix this?

Nope. We're now comparing against the SkCodec's alphaType, so we do actually
want to return false if it was not opaque.

https://codereview.chromium.org/1996993003/diff/40001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:344: kOpaque_SkAlphaType != decodeInfo->alphaType()) {
On 2016/05/20 20:52:22, msarett wrote:
> Don't we need to fix this?

Similarly, we don't want to try decoding non-opaque to 565.

[scroggo, 2016-05-20 20:55:09]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2016-05-20 20:55:19]

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

[commit-bot: I haz the power, 2016-05-20 20:56:17]

Description was changed from

==========
Finish supporting decoding opaque to non-opaque

When decoding to 565 or Gray, allow the client to incorrectly ask for premul.

When checking whether it's possible to decode to 565, return whether the
source is opaque.

In DM, allow decoding to 565 or Gray, even if the client also asked for premul.

This fixes a bug introduced in crrev.com/1999593003 when we stopped ever
requesting Opaque, resulting in us not testing 565 or Gray.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003
==========

to

==========
Finish supporting decoding opaque to non-opaque

When decoding to 565 or Gray, allow the client to incorrectly ask for premul.

When checking whether it's possible to decode to 565, return whether the
source is opaque.

In DM, allow decoding to 565 or Gray, even if the client also asked for premul.

This fixes a bug introduced in crrev.com/1999593003 when we stopped ever
requesting Opaque, resulting in us not testing 565 or Gray.

BUG=skia:4616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=1996993003

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

[commit-bot: I haz the power, 2016-05-20 20:56:19]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/ba5848953e00685f9a57343aef94372c5803130a

[msarett, 2016-05-20 21:00:12]

On 2016/05/20 20:56:19, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
> https://skia.googlesource.com/skia/+/ba5848953e00685f9a57343aef94372c5803130a

Well we only create kGrayAlways tests when the codec is kGray (which means it
will always also be opaque).

But yeah, you're right for 565.  Maybe there's another potential speed-up for DM
if we can figure out how to not create 565 CodecSrcs for non-opaque images.

[scroggo, 2016-05-20 21:12:09]

On 2016/05/20 21:00:12, msarett wrote:
> On 2016/05/20 20:56:19, commit-bot: I haz the power wrote:
> > Committed patchset #3 (id:40001) as
> >
https://skia.googlesource.com/skia/+/ba5848953e00685f9a57343aef94372c5803130a
> 
> Well we only create kGrayAlways tests when the codec is kGray (which means it
> will always also be opaque).

Fair enough. Removing in https://codereview.chromium.org/2002723002

> 
> But yeah, you're right for 565.  Maybe there's another potential speed-up for
DM
> if we can figure out how to not create 565 CodecSrcs for non-opaque images.

I don't think so. Because we create a Src, and 565 describes the Sink.