Update CodexTest to test valid_alpha.

Update CodexTest to test valid_alpha.

Add a static function to CodexTest, which consolidates decoding,
comparing to an expected SkCodec::Result, and optionally comparing to a
digest.

Test decoding non-opaque to opaque (fails) and premul to unpremul/vice
versa (succeeds).

BUG=skia:3475
Committed: https://skia.googlesource.com/skia/+/d1bc5746ba9ec86b1ef50aa0d3131adb0e2843f5

[scroggo_chromium, 2015-08-07 23:16:30]

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

[scroggo_chromium, 2015-08-07 23:16:31]

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp#newcode114
tests/CodexTest.cpp:114: if (info.alphaType() == kOpaque_SkAlphaType) {
When I started writing this CL, I expected that an opaque image (e.g. JPEG)
could be decoded to any alpha type, since it is a valid premul/unpremul image.
So the content of this block was to verify that we succeeded, and the digest
matched the original.

I realized that we do not behave that way. Instead, we require the client to
request opaque if the image is opaque. Is that the right way to behave? I can
see the argument that it lets them catch their mistake.

If we stick with the current behavior, we need to add the following to this
block:

test_info(r, codec, info.makeAlphaType(<>), SkCodec::kInvalidConversion,
          NULL);

(Twice, once with kUnpremul and once with kPremul)

If we decide to support decoding opaque to non-opaque, we'll need to apply
https://codereview.chromium.org/1282073004 (DIFFERENT_ISSUE).

(Maybe we already discussed this and came to a decision, but I just forgot...)

[msarett, 2015-08-10 13:51:12]

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp#newcode114
tests/CodexTest.cpp:114: if (info.alphaType() == kOpaque_SkAlphaType) {
On 2015/08/07 23:16:31, scroggo_chromium wrote:
> When I started writing this CL, I expected that an opaque image (e.g. JPEG)
> could be decoded to any alpha type, since it is a valid premul/unpremul image.
> So the content of this block was to verify that we succeeded, and the digest
> matched the original.
> 
> I realized that we do not behave that way. Instead, we require the client to
> request opaque if the image is opaque. Is that the right way to behave? I can
> see the argument that it lets them catch their mistake.
> 
> If we stick with the current behavior, we need to add the following to this
> block:
> 
> test_info(r, codec, info.makeAlphaType(<>), SkCodec::kInvalidConversion,
>           NULL);
> 
> (Twice, once with kUnpremul and once with kPremul)
> 
> If we decide to support decoding opaque to non-opaque, we'll need to apply
> https://codereview.chromium.org/1282073004 (DIFFERENT_ISSUE).
> 
> (Maybe we already discussed this and came to a decision, but I just forgot...)

I would guess that most clients would just use the alpha type that we recommend,
but some may always ask for kPremul/kUnpremul because that is what is convenient
for them:
1)I don't see a real problem with supporting kPremul, other than the fact that
it is a bit misleading about what the image actually is.
2) I'm a little more concerned about kUnpremul because I wouldn't want a client
that does it's own premultiplying to be premultiplying opaque images.

I think it makes the most sense to me to disable conversions to
kPremul/kUnpremul.  That way we force clients to at least be aware that the
image is opaque.  Then, if they still want to treat the output as
kPremul/kUnpremul, they can still do that.

Having said that, I don't really feel strongly about this.  I could be swayed
easily.

[scroggo_chromium, 2015-08-12 14:55:31]

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1277253003/diff/1/tests/CodexTest.cpp#newcode114
tests/CodexTest.cpp:114: if (info.alphaType() == kOpaque_SkAlphaType) {
On 2015/08/10 13:51:12, msarett wrote:
> On 2015/08/07 23:16:31, scroggo_chromium wrote:
> > When I started writing this CL, I expected that an opaque image (e.g. JPEG)
> > could be decoded to any alpha type, since it is a valid premul/unpremul
image.
> > So the content of this block was to verify that we succeeded, and the digest
> > matched the original.
> > 
> > I realized that we do not behave that way. Instead, we require the client to
> > request opaque if the image is opaque. Is that the right way to behave? I
can
> > see the argument that it lets them catch their mistake.
> > 
> > If we stick with the current behavior, we need to add the following to this
> > block:
> > 
> > test_info(r, codec, info.makeAlphaType(<>), SkCodec::kInvalidConversion,
> >           NULL);
> > 
> > (Twice, once with kUnpremul and once with kPremul)
> > 
> > If we decide to support decoding opaque to non-opaque, we'll need to apply
> > https://codereview.chromium.org/1282073004 (DIFFERENT_ISSUE).
> > 
> > (Maybe we already discussed this and came to a decision, but I just
forgot...)
> 
> I would guess that most clients would just use the alpha type that we
recommend,
> but some may always ask for kPremul/kUnpremul because that is what is
convenient
> for them:
> 1)I don't see a real problem with supporting kPremul, other than the fact that
> it is a bit misleading about what the image actually is.
> 2) I'm a little more concerned about kUnpremul because I wouldn't want a
client
> that does it's own premultiplying to be premultiplying opaque images.
> 
> I think it makes the most sense to me to disable conversions to
> kPremul/kUnpremul.  That way we force clients to at least be aware that the
> image is opaque.  Then, if they still want to treat the output as
> kPremul/kUnpremul, they can still do that.
> 
> Having said that, I don't really feel strongly about this.  I could be swayed
> easily.

Sticking with the existing decision sounds good to me. Added the tests I left
out before. This also required updating wbmp to check valid_alpha.

[msarett, 2015-08-12 15:04:45]

lgtm

[scroggo, 2015-08-12 15:21:25]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-08-12 15:21:37]

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

[commit-bot: I haz the power, 2015-08-12 15:31:47]

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