Test region decoding in skimage, plus fixes.

Test region decoding in skimage, plus fixes.

Add tests in skimage to perform region decoding. Write out a
PNG of the region as well as a bitmap obtained with extractSubset
for comparison.

Rename decodeRegion to decodeSubset, so it will not be confused
with SkRegion. (Leave a function called decodeRegion which calls
decodeSubset.)

Clean up some comments.

Use png_set_interlaced_pass instead of modifying pass directly.

Make some changes to region decoding to fix problems I discovered
during testing:

Only call getAddr within a valid range.
Check for a NULL fInputStream.
Return a boolean for whether cropBitmap succeeded.
In cropBitmap, do not attempt to draw to a bitmap to an Index8
bitmap, which crashes. Use extractSubset instead.
Remove an assert.

R=djsollen@google.com

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

[scroggo, 2013-05-02 22:08:43]

Joseph, it looks like you wrote some of the decodeRegion code, so I wanted to
run my changes by you to make sure it still looks good. I ran into some crashes
trying to call decodeRegion on a PNG that gets decoded into kIndex8_Config
SkBitmap.

Florin, I think I found the call to replace the direct access to png_ptr->pass.

[djsollen, 2013-05-03 13:50:29]

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

https://codereview.chromium.org/14567011/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:227: //SkASSERT(dst->width() >= w && dst->height()
>= h);
seems like this should be an SkDEBUGF statement or removed entirely.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:121: static SkIRect generate_random_rect(int32_t maxX,
int32_t maxY) {
can you document that this function never returns a degenerate rect, but may
return an empty rect.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:166: // compared against earlier results.
do you have any ideas on how to make this consistent? It seems at a high level
you could reset your random number generator for each image and get comparable
results.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:181: // Also use extractSubset from the original for
visual comparison.
does a pixel compare at this stage fail?

[scroggo, 2013-05-03 17:09:24]

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

https://codereview.chromium.org/14567011/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:227: //SkASSERT(dst->width() >= w && dst->height()
>= h);
On 2013/05/03 13:50:29, djsollen wrote:
> seems like this should be an SkDEBUGF statement or removed entirely.

Agreed. I have turned it into an SkDEBUGF statement.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:121: static SkIRect generate_random_rect(int32_t maxX,
int32_t maxY) {
On 2013/05/03 13:50:29, djsollen wrote:
> can you document that this function never returns a degenerate rect, but may
> return an empty rect.

Actually, it's probably better to not return an empty rect. I've modified the
code to reflect that.

In a separate CL I'll add a unit test that tests for empty, to make sure it is
handled gracefully.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:166: // compared against earlier results.
On 2013/05/03 13:50:29, djsollen wrote:
> do you have any ideas on how to make this consistent? It seems at a high level
> you could reset your random number generator for each image and get comparable
> results.

Yeah, that was what I was thinking, although I hoped I might be able to come up
with some solution that might be more representative of actual use cases.
Consistently random is probably good enough though.

https://codereview.chromium.org/14567011/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:181: // Also use extractSubset from the original for
visual comparison.
On 2013/05/03 13:50:29, djsollen wrote:
> does a pixel compare at this stage fail?

Yes. I do not yet know whether that is due to bugs or the nature of subset
decoding.

[djsollen, 2013-05-03 19:58:12]

lgtm

https://codereview.chromium.org/14567011/diff/6001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14567011/diff/6001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:198: // compared against earlier results.
not sure this comment is completely valid now that comparisons are possible. It
may be true that there are more representative cases we should test though.

[scroggo, 2013-05-03 20:10:24]

https://codereview.chromium.org/14567011/diff/6001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14567011/diff/6001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:198: // compared against earlier results.
On 2013/05/03 19:58:12, djsollen wrote:
> not sure this comment is completely valid now that comparisons are possible.
It
> may be true that there are more representative cases we should test though.

Done.

[scroggo, 2013-05-03 20:14:33]

Committed patchset #3 manually as r8996 (presubmit successful).