Update Subset benches to support interlacing and fix bugs

Update Subset benches to support interlacing and fix bugs

Instead of decoding one line at a time, if the ScanlineOrder is kNone,
decode all of the lines in one pass, and then copy the subset into the
output. This will allow us to more realistically test subset decodes
for interlaced png. It also makes running them not take forever.

Do *not* support other modes (besides kTopDown), since they are not
used by the big three we need to replace BitmapRegionDecoder
implementation (skbug.com/4428).

Fix a bug in SubsetTranslateBench and SubsetZoomBench:
When we decode another subset, we need to reset the scanline decode
first. This bug appears to have been present since the introduction of
these tests in crrev.com/1160953002

BUG=skia:4205
BUG=skia:3418
Committed: https://skia.googlesource.com/skia/+/ab12c2715886e7f04c415028a7db49fb0b1e67f3

[scroggo, 2015-10-06 20:36:03]

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

[scroggo, 2015-10-06 20:36:04]

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
File bench/subset/SubsetSingleBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
bench/subset/SubsetSingleBench.cpp:81: SkAutoTDeleteArray<uint8_t> row(new
uint8_t[info.minRowBytes()]);
This got moved because it's not needed for kNone.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
bench/subset/SubsetSingleBench.cpp:82: for (uint32_t y = 0; y < fSubsetHeight;
y++) {
This code should be unchanged; it's just now inside the switch statement.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:80: codec->startScanlineDecode(info,
nullptr, colors, &colorCount);
Here's the bug (unless I'm missing something?): without calling startScanlines
again, we're just attempting to read off the end of the image, which will return
some failure case (which we never read).

On that note, perhaps we should read the Results and assert them. (I also wonder
if we should make it so kNonrendering tests in nanobench still get a canvas, at
least when you use the flag -w to write the output images, so we can verify the
results somehow.)

Taking it a step further, maybe we ultimately don't need these tests - instead,
we should be using some version of SkBitmapRegionDecoderInterface.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:92: for (uint32_t y = 0; y <
currSubsetHeight; y++) {
Again, I think this loop is unchanged.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetZoomBenc...
File bench/subset/SubsetZoomBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetZoomBenc...
bench/subset/SubsetZoomBench.cpp:89: for (int y = 0; y < subsetHeight; y++) {
loop should be unchanged

[msarett, 2015-10-06 21:58:52]

lgtm

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
File bench/subset/SubsetSingleBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
bench/subset/SubsetSingleBench.cpp:81: SkAutoTDeleteArray<uint8_t> row(new
uint8_t[info.minRowBytes()]);
On 2015/10/06 20:36:04, scroggo wrote:
> This got moved because it's not needed for kNone.

Acknowledged.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetSingleBe...
bench/subset/SubsetSingleBench.cpp:82: for (uint32_t y = 0; y < fSubsetHeight;
y++) {
On 2015/10/06 20:36:04, scroggo wrote:
> This code should be unchanged; it's just now inside the switch statement.

Acknowledged.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:80: codec->startScanlineDecode(info,
nullptr, colors, &colorCount);
On 2015/10/06 20:36:04, scroggo wrote:
> Here's the bug (unless I'm missing something?): without calling startScanlines
> again, we're just attempting to read off the end of the image, which will
return
> some failure case (which we never read).
> 
> On that note, perhaps we should read the Results and assert them. (I also
wonder
> if we should make it so kNonrendering tests in nanobench still get a canvas,
at
> least when you use the flag -w to write the output images, so we can verify
the
> results somehow.)
> 
> Taking it a step further, maybe we ultimately don't need these tests -
instead,
> we should be using some version of SkBitmapRegionDecoderInterface.

Ugh this is an embarrassing bug.

Theoretically, we test correctness in dm, so we can test performance here
without worrying about checking correctness.  But I guess the code got
complicated enough that I made a mistake.  I'm not sure if that is a bad
oversight on my part or an indication that we need more ways of error checking
here.

Certainly asserting the results as a first step can't hurt.

I do think that the SkBitmapRegionDecoderInterface tests are more important at
this point...these will be tests we look at when replacing BRD.  But I think
these still have value, especially now that we need to redo them to make valid
conclusions.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:92: for (uint32_t y = 0; y <
currSubsetHeight; y++) {
On 2015/10/06 20:36:04, scroggo wrote:
> Again, I think this loop is unchanged.

Acknowledged.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetZoomBenc...
File bench/subset/SubsetZoomBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetZoomBenc...
bench/subset/SubsetZoomBench.cpp:89: for (int y = 0; y < subsetHeight; y++) {
On 2015/10/06 20:36:04, scroggo wrote:
> loop should be unchanged

Acknowledged.

[scroggo, 2015-10-07 19:50:58]

Please take a look at patch set 3. I've prevented us from running the
SubsetBenches on any files that do not make sense, and added in the asserts.

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:80: codec->startScanlineDecode(info,
nullptr, colors, &colorCount);
On 2015/10/06 21:58:52, msarett wrote:
> On 2015/10/06 20:36:04, scroggo wrote:
> > Here's the bug (unless I'm missing something?): without calling
startScanlines
> > again, we're just attempting to read off the end of the image, which will
> return
> > some failure case (which we never read).
> > 
> > On that note, perhaps we should read the Results and assert them. (I also
> wonder
> > if we should make it so kNonrendering tests in nanobench still get a canvas,
> at
> > least when you use the flag -w to write the output images, so we can verify
> the
> > results somehow.)
> > 
> > Taking it a step further, maybe we ultimately don't need these tests -
> instead,
> > we should be using some version of SkBitmapRegionDecoderInterface.
> 
> Ugh this is an embarrassing bug.
> 
> Theoretically, we test correctness in dm, so we can test performance here
> without worrying about checking correctness.  But I guess the code got
> complicated enough that I made a mistake.  I'm not sure if that is a bad
> oversight on my part or an indication that we need more ways of error checking
> here.
> 
> Certainly asserting the results as a first step can't hurt.

Done.

> 
> I do think that the SkBitmapRegionDecoderInterface tests are more important at
> this point...these will be tests we look at when replacing BRD.  But I think
> these still have value, especially now that we need to redo them to make valid
> conclusions.

That is probably ultimately true, although we cannot look at those until we
settle on how to do subsets in SkScaledCodec (and of course, the hard part is
sampling combined with subsets).

In the meantime, I think this gives us something interesting to look at. (Also,
if we find the straightforward code here is better, but not so in SkScaledCodec,
we can know that we've probably done something wrong in SkScaledCodec...)

[msarett, 2015-10-07 20:06:08]

lgtm

Wish we'd had those asserts a while ago...

https://codereview.chromium.org/1387233002/diff/40001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1387233002/diff/40001/bench/nanobench.cpp#new...
bench/nanobench.cpp:521: return !useCodec && (path.endsWith("webp") ||
path.endsWith("WEBP"));
It occurs to me that we've done very little to benchmark webp subset decodes. 
I'm guessing that we probably won't see any surprises since our implementation
is so similar to SkImageDecoder?

[scroggo, 2015-10-07 20:09:10]

https://codereview.chromium.org/1387233002/diff/40001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1387233002/diff/40001/bench/nanobench.cpp#new...
bench/nanobench.cpp:521: return !useCodec && (path.endsWith("webp") ||
path.endsWith("WEBP"));
On 2015/10/07 20:06:08, msarett wrote:
> It occurs to me that we've done very little to benchmark webp subset decodes. 
> I'm guessing that we probably won't see any surprises since our implementation
> is so similar to SkImageDecoder?

Yes. I do think it's worth benchmarking in general, but it shouldn't hold up
switching to SkCodec.

[scroggo, 2015-10-07 20:11:35]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-10-07 20:11:36]

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/1387233002/#ps60001
(title: "Rebase/remove dependent patch set")

[commit-bot: I haz the power, 2015-10-07 20:11:48]

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

[commit-bot: I haz the power, 2015-10-07 20:14:20]

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

[commit-bot: I haz the power, 2015-10-07 20:14:21]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[scroggo, 2015-10-08 18:29:08]

Please take another look. The windows bot found an unitialized read, which is
fixed in the latest patch set.

https://codereview.chromium.org/1387233002/diff/60001/bench/subset/SubsetTran...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/60001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:74: alloc_pixels(&bitmap, subsetInfo,
colors, colorCount);
The bots caught me - "colorCount" is now unitialized. Previously it was
initialized by the call to startScanlineDecode, but I've moved that down below
due to the fact that it needs to be called in each iteration of the loop. So now
I need to initialize it first...

Fixed in the next patch set.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:69: int colorCount = 256;
I'm cheating a little bit here. Setting this value here allows alloc_pixels to
read from it (and expect a deterministic value), but that won't necessarily be
the value set by startScanlineDecode (on the other hand, I think we almost
always defensively set it to 256 - perhaps the only time we do not is in wbmp,
since the indices in the data, being stored in one bit per pixel, cannot be out
of range, so this might be correct in most cases).

Alternatively, I could have continued to call startScanlineDecode before calling
alloc_pixels. This can be done in one of two ways:

1. Make an extra call to startScanlineDecode
2. Do not reuse the bitmap

(1) probably does more work (it means we need to rewind and reread the stream,
and potentially recreate some internal structures), and probably does not
represent the use case.

(2) goes against the original intent, which was to reuse the same bitmap. I
wonder if it would be more realistic to use a pool of bitmaps (since the Android
client wants to have multiple tiles which can be drawn to the screen in one
frame), but that seems like too much work for us to try to reproduce their exact
use case.

This seemed like the best compromise to me.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:91: codec->startScanlineDecode(info,
nullptr, get_colors(&bitmap), &colorCount);
Note that I'm using the colors from the bitmap's color table, which were copied
from colors. If I just used colors directly, bitmap's colors would be whatever
they were randomly initialized to. I think this only really matters if we decide
to draw the output and look at it.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
File bench/subset/SubsetZoomBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
bench/subset/SubsetZoomBench.cpp:87: alloc_pixels(&bitmap, subsetInfo, colors,
colorCount);
Since we do a new allocation each time, we can always do this after
startScanlineDecode, so colorCount (as well as colors) is always initialized.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
bench/subset/SubsetZoomBench.cpp:109: alloc_pixels(&stripeBm, stripeInfo,
colors, colorCount);
Here and in SubsetTranslateBench, we're doing some extra work to create a bitmap
plus its colortable (in index8). We could instead just do some kind of malloc.
Then we'd have to write more code to do all the calculations that bitmap does
for us. I went with the simpler option, since this test had errors in the
past...

[msarett, 2015-10-08 18:52:42]

lgtm

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:69: int colorCount = 256;
On 2015/10/08 18:29:08, scroggo wrote:
> I'm cheating a little bit here. Setting this value here allows alloc_pixels to
> read from it (and expect a deterministic value), but that won't necessarily be
> the value set by startScanlineDecode (on the other hand, I think we almost
> always defensively set it to 256 - perhaps the only time we do not is in wbmp,
> since the indices in the data, being stored in one bit per pixel, cannot be
out
> of range, so this might be correct in most cases).
> 

Yes I think it is almost always 256.  Rare bmps may also sometimes have a
smaller color table when they use less than 8-bit indices.

> Alternatively, I could have continued to call startScanlineDecode before
calling
> alloc_pixels. This can be done in one of two ways:
> 
> 1. Make an extra call to startScanlineDecode
> 2. Do not reuse the bitmap
> 
> (1) probably does more work (it means we need to rewind and reread the stream,
> and potentially recreate some internal structures), and probably does not
> represent the use case.
> 
> (2) goes against the original intent, which was to reuse the same bitmap. I
> wonder if it would be more realistic to use a pool of bitmaps (since the
Android
> client wants to have multiple tiles which can be drawn to the screen in one
> frame), but that seems like too much work for us to try to reproduce their
exact
> use case.
> 
> This seemed like the best compromise to me.

I agree that this is the best compromise.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:91: codec->startScanlineDecode(info,
nullptr, get_colors(&bitmap), &colorCount);
On 2015/10/08 18:29:08, scroggo wrote:
> Note that I'm using the colors from the bitmap's color table, which were
copied
> from colors. If I just used colors directly, bitmap's colors would be whatever
> they were randomly initialized to. I think this only really matters if we
decide
> to draw the output and look at it.

Acknowledged.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
File bench/subset/SubsetZoomBench.cpp (right):

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
bench/subset/SubsetZoomBench.cpp:87: alloc_pixels(&bitmap, subsetInfo, colors,
colorCount);
On 2015/10/08 18:29:08, scroggo wrote:
> Since we do a new allocation each time, we can always do this after
> startScanlineDecode, so colorCount (as well as colors) is always initialized.

Acknowledged.

https://codereview.chromium.org/1387233002/diff/80001/bench/subset/SubsetZoom...
bench/subset/SubsetZoomBench.cpp:109: alloc_pixels(&stripeBm, stripeInfo,
colors, colorCount);
On 2015/10/08 18:29:08, scroggo wrote:
> Here and in SubsetTranslateBench, we're doing some extra work to create a
bitmap
> plus its colortable (in index8). We could instead just do some kind of malloc.
> Then we'd have to write more code to do all the calculations that bitmap does
> for us. I went with the simpler option, since this test had errors in the
> past...

This is probably the right decision.

In general, I don't like SkBitmap and SkColorTable and use them as little as
possible.  I think we are stuck with Bitmaps anyway for the Android API so we
might as well benchmark them.

[scroggo, 2015-10-08 18:53:58]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-10-08 18:54:04]

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

[commit-bot: I haz the power, 2015-10-08 19:02:46]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/ab12c2715886e7f04c415028a7db49fb0b1e67f3