Updates to skimage tool to use it for testing.

Updates to skimage tool to use it for testing.

skimage_main.cpp:
More changes in the interest of testing our decoders.

force_all_opaque before writing PNG files.

Test reencoding the image to its original type (if possible), and
then test redecoding it (to make sure the encoding was successful).
Add an option to turn off this behavior.

Merge decodeFileAndWrite with decodeFile.

SkImageDecoder:
Add kUnknown_Type to SkImageEncoder::Types.

Add a static function to get the Format of an SkStream.

In getFormatName(), remove an incorrect assert.

When calling the flavor of DecodeStream that returns the Format,
check the stream if the decoder returns kUnknown_Format.

BUG=https://code.google.com/p/skia/issues/detail?id=1241

Committed: https://code.google.com/p/skia/source/detail?r=8862

[scroggo, 2013-04-18 22:02:32]

More changes to test image decoding/encoding.

Ben, I tried to set up SkImageEncoder_WIC to encode to GIF, but it's failing. I
saw that you wrote this file to begin with, so I was wondering if you could help
out. It appears we want to set the format to GUID_WICPixelFormat32bppBGRA, but
end up with a different format.

[scroggo, 2013-04-19 20:19:28]

Adding epoger for expertise on comparing image files.

[epoger, 2013-04-19 21:35:16]

A few concerns... and you might want to consider asking bungeman to take a look.

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
include/images/SkImageDecoder.h:44: formats, returns the format of the data that
was most recently decoded successfully. Can
This time-dependent, stateful behavior (return format of most recently decoded
image) seems like it could bite us later.  I would rather the behavior be more
predictable.

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:21: const char *SkImageDecoder::sFormatName[] = {
Rather than using an array to hold this lookup table, I think a switch statement
is better, for two reasons:

1. If we forget to list any of the Format enum values in the switch statement,
the compiler will issue a warning.

2. It will be easier to make sure they are listed in the same order.

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:63: SkASSERT(SK_ARRAY_COUNT(sFormatName) ==
kLastKnownFormat + 1);
It scares me that the previous error wasn't caught by a unit test.  Are there
some unit tests that can be beefed up?

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp
File tools/picture_utils.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp#n...
tools/picture_utils.cpp:24: if (SkBitmap::kARGB_8888_Config == bitmap.config())
{
Do non-8888 configs even allow for alpha channel variations?  I thought, at this
point, we only supported 8888 and 565...

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp#n...
tools/picture_utils.cpp:28: bmPtr = &bitmap8888;
In this case, are this method's modifications even preserved?  It seems to me
that it is modifying bitmap8888, which only lives on the stack.

https://codereview.chromium.org/14363003/diff/15001/tools/skimage_main.cpp
File tools/skimage_main.cpp (left):

https://codereview.chromium.org/14363003/diff/15001/tools/skimage_main.cpp#ol...
tools/skimage_main.cpp:151: print_strings("Invalid files", invalids);
I have to give the previous author style points for this variable name.

[scroggo, 2013-04-19 22:12:01]

I haven't addressed all your concerns, but wanted to address the easy ones. Will
handle the other ones next week.

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
include/images/SkImageDecoder.h:44: formats, returns the format of the data that
was most recently decoded successfully. Can
On 2013/04/19 21:35:16, epoger wrote:
> This time-dependent, stateful behavior (return format of most recently decoded
> image) seems like it could bite us later.  I would rather the behavior be more
> predictable.

That seems reasonable to me. I used this behavior to figure out the Format of an
SkStream. I'll look for another way.

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:21: const char *SkImageDecoder::sFormatName[] = {
On 2013/04/19 21:35:16, epoger wrote:
> Rather than using an array to hold this lookup table, I think a switch
statement
> is better, for two reasons:
> 
> 1. If we forget to list any of the Format enum values in the switch statement,
> the compiler will issue a warning.
> 
> 2. It will be easier to make sure they are listed in the same order.

Done.

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:63: SkASSERT(SK_ARRAY_COUNT(sFormatName) ==
kLastKnownFormat + 1);
On 2013/04/19 21:35:16, epoger wrote:
> It scares me that the previous error wasn't caught by a unit test.  Are there
> some unit tests that can be beefed up?

Lucky for us, no one was calling it...

Using the switch statement, this assert has been removed.

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp
File tools/picture_utils.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp#n...
tools/picture_utils.cpp:24: if (SkBitmap::kARGB_8888_Config == bitmap.config())
{
On 2013/04/19 21:35:16, epoger wrote:
> Do non-8888 configs even allow for alpha channel variations?  I thought, at
this
> point, we only supported 8888 and 565...

I think kIndex8 does. And we can get kIndex8 from a decoder.

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp#n...
tools/picture_utils.cpp:28: bmPtr = &bitmap8888;
On 2013/04/19 21:35:16, epoger wrote:
> In this case, are this method's modifications even preserved?  It seems to me
> that it is modifying bitmap8888, which only lives on the stack.

D'oh! No the modifications are not :(

https://codereview.chromium.org/14363003/diff/15001/tools/skimage_main.cpp
File tools/skimage_main.cpp (left):

https://codereview.chromium.org/14363003/diff/15001/tools/skimage_main.cpp#ol...
tools/skimage_main.cpp:151: print_strings("Invalid files", invalids);
On 2013/04/19 21:35:16, epoger wrote:
> I have to give the previous author style points for this variable name.

Haha, that would be me. Thanks ;)

[epoger, 2013-04-20 21:10:07]

Changes look good so far.  Please consider adding unit tests, in addition to the
remaining work.

[djsollen, 2013-04-22 12:57:41]

https://codereview.chromium.org/14363003/diff/22001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14363003/diff/22001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:62: continue;
why do you need the unknown type in your gFormats list?  It seems like will
always get the same behavior without it as it is what you return by default.

Not having it there also allows you to remove if statements like the one above.

[scroggo, 2013-04-22 22:10:15]

epoger@ said
> Please consider adding unit tests,

It is actually my intention that skimage itself be a kind of unit test.

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/15001/include/images/SkImageDec...
include/images/SkImageDecoder.h:44: formats, returns the format of the data that
was most recently decoded successfully. Can
On 2013/04/19 22:12:01, scroggo wrote:
> On 2013/04/19 21:35:16, epoger wrote:
> > This time-dependent, stateful behavior (return format of most recently
decoded
> > image) seems like it could bite us later.  I would rather the behavior be
more
> > predictable.
> 
> That seems reasonable to me. I used this behavior to figure out the Format of
an
> SkStream. I'll look for another way.

Removed. Instead of this behavior, I've retained the old behavior, and added
getFormat(SkStream*)

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp
File tools/picture_utils.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/tools/picture_utils.cpp#n...
tools/picture_utils.cpp:28: bmPtr = &bitmap8888;
On 2013/04/19 22:12:01, scroggo wrote:
> On 2013/04/19 21:35:16, epoger wrote:
> > In this case, are this method's modifications even preserved?  It seems to
me
> > that it is modifying bitmap8888, which only lives on the stack.
> 
> D'oh! No the modifications are not :(

Rather than modify this function further, so it will do what I want it to do, I
do my own forcing to opaque...

https://codereview.chromium.org/14363003/diff/22001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14363003/diff/22001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:62: continue;
On 2013/04/22 12:57:41, djsollen wrote:
> why do you need the unknown type in your gFormats list?  It seems like will
> always get the same behavior without it as it is what you return by default.
> 
> Not having it there also allows you to remove if statements like the one
above.

Done.

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:40: kMultiple_Formats,
Does this seem like the right place for kMultiple_Formats?

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:55: const char* getFormatName() const;
Derek, does Android call this? (I assume I have access now so I can check?)

I'm tempted to deprecate it in favor of a function which takes an
SkImageDecoder::Format instead.

[epoger, 2013-04-23 16:03:40]

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/14363003/diff/15001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:63: SkASSERT(SK_ARRAY_COUNT(sFormatName) ==
kLastKnownFormat + 1);
On 2013/04/19 22:12:01, scroggo wrote:
> On 2013/04/19 21:35:16, epoger wrote:
> > It scares me that the previous error wasn't caught by a unit test.  Are
there
> > some unit tests that can be beefed up?
> 
> Lucky for us, no one was calling it...
> 
> Using the switch statement, this assert has been removed.

Regarding unit tests: Regardless of whether code is run as part of the Skia
library, or used as a tool to test the Skia library, I think it's good (in the
absence of counter-argument) to make sure it's tested.  Broken tools can cause
all sorts of problems.

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:40: kMultiple_Formats,
On 2013/04/22 22:10:16, scroggo wrote:
> Does this seem like the right place for kMultiple_Formats?

This seems problematic to me.  Maybe, instead, there could be a bool
decodesFormat(Format f) method that tells the caller whether a particular format
is supported?  The caller can loop through and try several if he wants to...

Otherwise, return an array of formats that are supported.

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:49: kUnknown_Format.
Please indicate what state the SkStream will be left in (fully rewound? unknown
state?)

https://codereview.chromium.org/14363003/diff/59001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:57: Format format = this->onGetFormat(stream);
It bugs me that SkImageDecoder::getFormat(SkStream* stream) purports to tell me
which format a stream is, but really all it does is tell me whether the stream
matches the SkImageDecoder subclass I instantiated.

e.g., if I call jpegImageDecoder.getFormat(gifStream), it will return
kUnknown_Format, NOT kGIF_Format .

So, I think it would be more honest to call this method something like
bool isSupportedFormat(SkStream* stream) ,
and if we want to find out which format a stream is encoded in, we could create
a factory class that loops over the various implementations and, depending on
which one returns true for isSupportedFormat(), returns the appropriate Format
enum value.

https://codereview.chromium.org/14363003/diff/59001/src/ports/SkImageDecoder_...
File src/ports/SkImageDecoder_CG.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/src/ports/SkImageDecoder_...
src/ports/SkImageDecoder_CG.cpp:91: return UTType_to_Format(name);
Oh.  I guess this is why the SkImageDecoder subclasses can't just return a
boolean indicating whether they support the format or not... this "meta-decoder"
can support several.

Although, practically speaking, why would the caller care WHICH format the
CG_Decoder finds in the stream?  I think all the caller cares about is whether
CG_Decoder can decode an image from the stream or not, right?

https://codereview.chromium.org/14363003/diff/59001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:97: SkBitmap* bmPtr;
or, rather than creating this additional SkBitmap*, you could just modify the
existing "bm" pointer and then rest assured that it points at an 8888 bitmap for
the rest of this method...

// Make sure bm points at an 8888 bitmap
SkBitmap bitmapCopy;
if (SkBitmap::kARGB_8888_Config != bm->config()) {
  if (bm->copyTo(&bitmapCopy, SkBitmap::kARGB_8888_Config)) {
    bm = &bitmapCopy;
  } else {
    return false;
  }
}

I don't know which is better, but I just had to get that out of my system.

[djsollen, 2013-04-23 16:40:45]

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:55: const char* getFormatName() const;
On 2013/04/22 22:10:16, scroggo wrote:
> Derek, does Android call this? (I assume I have access now so I can check?)
> 
> I'm tempted to deprecate it in favor of a function which takes an
> SkImageDecoder::Format instead.

It is only used in a debug statement in the BitmapRegionDecoder.cpp

[scroggo, 2013-04-23 20:55:18]

https://codereview.chromium.org/14363003/diff/59001/src/ports/SkImageDecoder_...
File src/ports/SkImageDecoder_CG.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/src/ports/SkImageDecoder_...
src/ports/SkImageDecoder_CG.cpp:91: return UTType_to_Format(name);
On 2013/04/23 16:03:41, epoger wrote:
> Oh.  I guess this is why the SkImageDecoder subclasses can't just return a
> boolean indicating whether they support the format or not... this
"meta-decoder"
> can support several.
> 
> Although, practically speaking, why would the caller care WHICH format the
> CG_Decoder finds in the stream?  I think all the caller cares about is whether
> CG_Decoder can decode an image from the stream or not, right?

In general, you are correct. The typical client will not care about the Format
of the data. In skimage, I actually do care about the Format, because I want to
test the encoder to verify that we can encode back into that Format. (Ideally
I'd like to then compare that image(re-encoded) to the original, but I don't
think that will work without doing a fuzzy compare.) Do you think that's a good
strategy for skimage? If so, I need to get the data somehow.

[scroggo, 2013-04-24 18:00:04]

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:40: kMultiple_Formats,
On 2013/04/23 16:03:41, epoger wrote:
> On 2013/04/22 22:10:16, scroggo wrote:
> > Does this seem like the right place for kMultiple_Formats?
> 
> This seems problematic to me.  Maybe, instead, there could be a bool
> decodesFormat(Format f) method that tells the caller whether a particular
format
> is supported?  The caller can loop through and try several if he wants to...
> 
> Otherwise, return an array of formats that are supported.

That seems reasonable to me. For now though, I'll leave getFormat the way it is
and look into deprecation in a further CL.

https://codereview.chromium.org/14363003/diff/59001/include/images/SkImageDec...
include/images/SkImageDecoder.h:49: kUnknown_Format.
On 2013/04/23 16:03:41, epoger wrote:
> Please indicate what state the SkStream will be left in (fully rewound?
unknown
> state?)

Done.

https://codereview.chromium.org/14363003/diff/59001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:57: Format format = this->onGetFormat(stream);
On 2013/04/23 16:03:41, epoger wrote:
> It bugs me that SkImageDecoder::getFormat(SkStream* stream) purports to tell
me
> which format a stream is, but really all it does is tell me whether the stream
> matches the SkImageDecoder subclass I instantiated.
> 
> e.g., if I call jpegImageDecoder.getFormat(gifStream), it will return
> kUnknown_Format, NOT kGIF_Format .
> 
> So, I think it would be more honest to call this method something like
> bool isSupportedFormat(SkStream* stream) ,
> and if we want to find out which format a stream is encoded in, we could
create
> a factory class that loops over the various implementations and, depending on
> which one returns true for isSupportedFormat(), returns the appropriate Format
> enum value.

I've replaced this function with a static function. Each decoder now registers a
function to check to see if it knows the Format. They will be called
consecutively, similar to how Factory works.

https://codereview.chromium.org/14363003/diff/59001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14363003/diff/59001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:97: SkBitmap* bmPtr;
On 2013/04/23 16:03:41, epoger wrote:
> or, rather than creating this additional SkBitmap*, you could just modify the
> existing "bm" pointer and then rest assured that it points at an 8888 bitmap
for
> the rest of this method...
> 
> // Make sure bm points at an 8888 bitmap
> SkBitmap bitmapCopy;
> if (SkBitmap::kARGB_8888_Config != bm->config()) {
>   if (bm->copyTo(&bitmapCopy, SkBitmap::kARGB_8888_Config)) {
>     bm = &bitmapCopy;
>   } else {
>     return false;
>   }
> }
> 
> I don't know which is better, but I just had to get that out of my system.

Done.

[djsollen, 2013-04-24 18:26:51]

as long as elliot feels his comments are addressed this looks ok to me.

[epoger, 2013-04-25 14:38:43]

LGTM!  Two optional suggestions, fine with me to commit with or without.

https://codereview.chromium.org/14363003/diff/79001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/79001/include/images/SkImageDec...
include/images/SkImageDecoder.h:50: static Format GetFormat(SkStream*);
optional: rename this method, because it is confusingly close to (but not
exactly the same as) getFormat.  Maybe GetStreamFormat() ?

https://codereview.chromium.org/14363003/diff/79001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14363003/diff/79001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:151: format = guess_format_from_suffix(dot);
Since there are two possible ways skimage can figure out the format (content or
filename), maybe it would be good to print a debug message stating what format
it determined and how?

[scroggo, 2013-04-25 17:31:05]

https://codereview.chromium.org/14363003/diff/79001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/14363003/diff/79001/include/images/SkImageDec...
include/images/SkImageDecoder.h:50: static Format GetFormat(SkStream*);
On 2013/04/25 14:38:43, epoger wrote:
> optional: rename this method, because it is confusingly close to (but not
> exactly the same as) getFormat.  Maybe GetStreamFormat() ?

Done.

[scroggo, 2013-04-25 17:34:01]

Committed patchset #16 manually as r8862 (presubmit successful).