Support decoding opaque to *premul

Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

Change the filling code (i.e. for incomplete images) to base its color on
the source alpha type. Prior to adding the support to decode opaque to
any, it was fine to use either source or dest (which would have yielded
the same result). If the client requests non-opaque, we do not want this
to switch the fill value from black to transparent. This also allows
simplifying the signatures for getFillValue and onGetFillValue.

In CodexTest, expect the same result when decoding opaque to *premul,
and compare to the opaque version.

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

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

[scroggo, 2016-01-29 13:29:11]

Description was changed from

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

In CodexTest, expect the same result when decoding to *premul, and
compare to the opaque version if we expect kSuccess (as opposed to
kIncomplete).

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

to

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

In CodexTest, expect the same result when decoding to *premul, and
compare to the opaque version if we expect kSuccess (as opposed to
kIncomplete).

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

[scroggo, 2016-01-29 13:33:20]

Description was changed from

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

In CodexTest, expect the same result when decoding to *premul, and
compare to the opaque version if we expect kSuccess (as opposed to
kIncomplete).

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

to

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

In CodexTest, expect the same result when decoding to *premul, and
compare to the opaque version if we expect kSuccess as opposed to
kIncomplete. An incomplete image fills with a different color based on
the alpha type, so the non-opaque versions look different.

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

[scroggo, 2016-01-29 14:11:45]

Description was changed from

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

In CodexTest, expect the same result when decoding to *premul, and
compare to the opaque version if we expect kSuccess as opposed to
kIncomplete. An incomplete image fills with a different color based on
the alpha type, so the non-opaque versions look different.

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

to

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

Change the filling code (i.e. for incomplete images) to base its color on
the source alpha type. Prior to adding the support to decode opaque to
any, it was fine to use either source or dest (which would have yielded
the same result). If the client requests non-opaque, we do not want this
to switch the fill value from black to transparent. This also allows
simplifying the signatures for getFillValue and onGetFillValue.

In CodexTest, expect the same result when decoding opaque to *premul,
and compare to the opaque version.

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

[scroggo, 2016-01-29 14:15:17]

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

[scroggo, 2016-01-29 14:15:17]

Something that came up in tests is that if we decode an opaque image to premul,
we initially changed the fill value from black to transparent (which only makes
a difference for incomplete images).

So I changed the code to depend on the native alpha type, rather than the
requested. Alternatively, we could decide that we want to offer the client a way
to fill with transparent by requesting premul/unpremul. In that case, patch set
3 is the way to go.

[msarett, 2016-01-29 14:23:05]

On 2016/01/29 14:15:17, scroggo wrote:
> Something that came up in tests is that if we decode an opaque image to
premul,
> we initially changed the fill value from black to transparent (which only
makes
> a difference for incomplete images).
> 
> So I changed the code to depend on the native alpha type, rather than the
> requested. Alternatively, we could decide that we want to offer the client a
way
> to fill with transparent by requesting premul/unpremul. In that case, patch
set
> 3 is the way to go.

I don't feel strongly about this, but I'll lean toward to the latest solution
(that won't change anything in Gold).  At some point we might want to allow the
client to set their own fill value.

[msarett, 2016-01-29 14:50:38]

I still have the same concerns I mentioned in skbug.com/4616, but if the rest of
Skia behaves like this, we ought to as well.

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:185: if (kOpaque_SkAlphaType ==
dstInfo.alphaType()) {
I think dstInfo needs to be changed to this->getInfo().alphaType() here.

I think there's a bug against me somewhere to make BMP decodes not depend on the
requested alpha type...  Though I can't find it, so maybe it's not filed.

Of course, if that bug exists, this seems to fix it easily enough.  I want to
suggest we add a test, but I'm not sure this is worth adding another mode to
DMSrcSink?  What do you think?

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

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:84: // If the source is opaque, we can support any.
Can we print a "Warning: This is a bad idea..." or something that like that
here?

[scroggo, 2016-01-29 17:19:54]

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:185: if (kOpaque_SkAlphaType ==
dstInfo.alphaType()) {
On 2016/01/29 14:50:38, msarett wrote:
> I think dstInfo needs to be changed to this->getInfo().alphaType() here.

Good catch!
> 
> I think there's a bug against me somewhere to make BMP decodes not depend on
the
> requested alpha type...  Though I can't find it, so maybe it's not filed.
> 
> Of course, if that bug exists, this seems to fix it easily enough.  I want to
> suggest we add a test, but I'm not sure this is worth adding another mode to
> DMSrcSink?  What do you think?

Alternatively, if you have a BMP image in resources (or one that we can check
into resources) we can rely on the test in CodexTest to make sure it matches.

Adding the test to DMSrcSink doesn't seem so bad - if it looks the same, as we
intend, it will not make any extra work for triaging in Gold, and it shouldn't
make the tests take significantly longer (and that's on the infrastructure team
anyway ;).

Patch set 5 fixes this, and 6 adds opaque to premul testing to DM.

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

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:84: // If the source is opaque, we can support any.
On 2016/01/29 14:50:38, msarett wrote:
> Can we print a "Warning: This is a bad idea..." or something that like that
> here?

You'll invoke Cary's ire for adding print statements to DM (since we test this
to make sure it works). We could instead add an SkError (a more powerful feature
for errors which never caught on), which would allow us to silence it in the
test.

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp#newcode264
dm/DM.cpp:264: folder.append("_opaque");
I think you have in the past considered such an extension and decided against it
(maybe that was in nanobench). This is a little wordy, especially if we only
have _premul and not _opaque. I could write some more code that would only do it
when needed, but it would be a little messier.

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp#newcode386
dm/DM.cpp:386: codec->getInfo().alphaType(), 1.0f);
This will be ignored, besides appending the suffix.

[msarett, 2016-01-29 19:17:05]

LGTM

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

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:84: // If the source is opaque, we can support any.
On 2016/01/29 17:19:54, scroggo wrote:
> On 2016/01/29 14:50:38, msarett wrote:
> > Can we print a "Warning: This is a bad idea..." or something that like that
> > here?
> 
> You'll invoke Cary's ire for adding print statements to DM (since we test this
> to make sure it works). We could instead add an SkError (a more powerful
feature
> for errors which never caught on), which would allow us to silence it in the
> test.

What about SkCodecPrintf?  I believe that's silent in DM, but turned on in
Android?

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp#newcode264
dm/DM.cpp:264: folder.append("_opaque");
On 2016/01/29 17:19:54, scroggo wrote:
> I think you have in the past considered such an extension and decided against
it
> (maybe that was in nanobench). This is a little wordy, especially if we only
> have _premul and not _opaque. I could write some more code that would only do
it
> when needed, but it would be a little messier.

I think this is good.  IMO it provides useful information when used in this
context.

https://codereview.chromium.org/1641273003/diff/100001/dm/DM.cpp#newcode386
dm/DM.cpp:386: codec->getInfo().alphaType(), 1.0f);
On 2016/01/29 17:19:54, scroggo wrote:
> This will be ignored, besides appending the suffix.

Acknowledged.

[scroggo, 2016-01-29 19:31:22]

Mike, can you review the API change (this is a protected method, so the public
API is unchanged).

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

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:84: // If the source is opaque, we can support any.
On 2016/01/29 19:17:05, msarett wrote:
> On 2016/01/29 17:19:54, scroggo wrote:
> > On 2016/01/29 14:50:38, msarett wrote:
> > > Can we print a "Warning: This is a bad idea..." or something that like
that
> > > here?
> > 
> > You'll invoke Cary's ire for adding print statements to DM (since we test
this
> > to make sure it works). We could instead add an SkError (a more powerful
> feature
> > for errors which never caught on), which would allow us to silence it in the
> > test.
> 
> What about SkCodecPrintf?  I believe that's silent in DM, but turned on in
> Android?

Done.

[reed1, 2016-01-29 19:49:12]

lgtm

[scroggo, 2016-01-29 19:50:58]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-01-29 19:50:59]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/1641273003/#ps120001
(title: "Add warning, and suffix for unpremul")

[commit-bot: I haz the power, 2016-01-29 19:51:03]

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

[scroggo, 2016-01-29 19:51:12]

The CQ bit was unchecked by scroggo@google.com

[scroggo, 2016-01-29 19:56:37]

Patchset #8 (id:140001) has been deleted

[scroggo, 2016-01-29 19:57:09]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-01-29 19:57:09]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1641273003/#ps160001
(title: "Rewrite warning; add warning to SkJpegCodec")

[commit-bot: I haz the power, 2016-01-29 19:57:17]

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

[commit-bot: I haz the power, 2016-01-29 20:03:05]

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

[commit-bot: I haz the power, 2016-01-29 20:03:06]

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, 2016-02-03 14:58:45]

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1641273003/diff/60001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:185: if (kOpaque_SkAlphaType ==
dstInfo.alphaType()) {
On 2016/01/29 14:50:38, msarett wrote:
> I think dstInfo needs to be changed to this->getInfo().alphaType() here.
> 
> I think there's a bug against me somewhere to make BMP decodes not depend on
the
> requested alpha type...  Though I can't find it, so maybe it's not filed.
> 
> Of course, if that bug exists, this seems to fix it easily enough.  I want to
> suggest we add a test, but I'm not sure this is worth adding another mode to
> DMSrcSink?  What do you think?

I don't know if there was a bug filed, but the SkMaskSwizzler still had a null
RowProc for 565 premul. It almost seems deceptive to allow a client to combine
565 with premul alphaType, but I guess it's in the spirit of the rest of this
change to support it? SkColorTypeValidateAlphaType returns true for such an
info, but it sets canonical to opaque (so an SkImage or SkBitmap gets the
canonical version).

https://codereview.chromium.org/1641273003/diff/200001/src/codec/SkMaskSwizzl...
File src/codec/SkMaskSwizzler.cpp (right):

https://codereview.chromium.org/1641273003/diff/200001/src/codec/SkMaskSwizzl...
src/codec/SkMaskSwizzler.cpp:253: proc = &swizzle_mask16_to_565;
These changes fix the crash I was seeing on the trybots.

[msarett, 2016-02-03 15:03:07]

lgtm

https://codereview.chromium.org/1641273003/diff/200001/src/codec/SkMaskSwizzl...
File src/codec/SkMaskSwizzler.cpp (right):

https://codereview.chromium.org/1641273003/diff/200001/src/codec/SkMaskSwizzl...
src/codec/SkMaskSwizzler.cpp:253: proc = &swizzle_mask16_to_565;
On 2016/02/03 14:58:45, scroggo wrote:
> These changes fix the crash I was seeing on the trybots.

Acknowledged.

[scroggo, 2016-02-03 16:21:29]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-02-03 16:21:30]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1641273003/#ps200001
(title: "Update SkMaskSwizzler to support opaque to premul")

[commit-bot: I haz the power, 2016-02-03 16:21:42]

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

[commit-bot: I haz the power, 2016-02-03 17:42:46]

Description was changed from

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

Change the filling code (i.e. for incomplete images) to base its color on
the source alpha type. Prior to adding the support to decode opaque to
any, it was fine to use either source or dest (which would have yielded
the same result). If the client requests non-opaque, we do not want this
to switch the fill value from black to transparent. This also allows
simplifying the signatures for getFillValue and onGetFillValue.

In CodexTest, expect the same result when decoding opaque to *premul,
and compare to the opaque version.

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

to

==========
Support decoding opaque to *premul

If a client requests unpremul or premul from an opaque SkCodec,
support it. The opaque image can be treated as any of them, though
it will be less efficient to draw than if the client had used
opaque.

Change the filling code (i.e. for incomplete images) to base its color on
the source alpha type. Prior to adding the support to decode opaque to
any, it was fine to use either source or dest (which would have yielded
the same result). If the client requests non-opaque, we do not want this
to switch the fill value from black to transparent. This also allows
simplifying the signatures for getFillValue and onGetFillValue.

In CodexTest, expect the same result when decoding opaque to *premul,
and compare to the opaque version.

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

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

[commit-bot: I haz the power, 2016-02-03 17:42:47]

Committed patchset #10 (id:200001) as
https://skia.googlesource.com/skia/+/c5560bef14149f4c945a4536988aeba1a16adedc