Subset decoding benchmarks

Subset decoding benchmarks

It was my goal to create benchmarks that could measure all
of the use cases that we have identified.  I think single
subsets, translating, and scaling are the important ones.

It might be a good idea to discuss the document in greater
detail as well.  I just wanted to share this to aid the
discussion.
https://docs.google.com/a/google.com/document/d/1OxW96GDMAlw6dnzNXmiNX-F9oDBBlGXzSsgd0DMIkbI/edit?usp=sharing

BUG=skia:

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

[msarett, 2015-05-28 18:16:06]

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

[msarett, 2015-05-28 18:57:13]

Patchset #1 (id:1) has been deleted

[scroggo, 2015-05-29 15:36:03]

Oh, shoot, I thought I had sent this last night...

I have some comments inline, but overall I wonder if it would be cleaner to use
different classes for different Modes/SkCodec vs. SkImageDecoder.

Also, I assume there are changes to nanobench and gyp files that need to be
made?

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.cpp
File bench/CodecSubsetBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:72: switch(fColorType) {
What about kIndex8?

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:106: switch(fSubsetMode) {
This giant switch statement might argue for using a different subclass for each
mode - it looks like we don't share much code amongst the modes.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:133: decoder->buildTileIndex(fStream->duplicate(),
&width, &height);
Do we need to safely early exit in case buildTileIndex fails?

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:149: uint32_t subsetWidth = info.width() / fDivisor;
These variables could all be const?

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:190: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(maxSubsetWidth, maxSubsetHeight,
When using SkImageDecoder, you don't need to allocate any pixels - the decoder
will do that for you.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:250: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(fSubsetWidth, fSubsetHeight,
Again, this is not necessary.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:287: // Note that if we subseted and scaled in a
single step, we could use the
subsetted? (Technically not a verb, but it seems like it should have two 't's if
we add an 'ed'.)

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:324: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(subsetWidth, subsetHeight,
Again, not needed. There are some cases where a bitmap is passed in already
allocated (which actually means it may make sense to test that), but they
wouldn't be allocating a new pixelref each time (I think?)

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.h
File bench/CodecSubsetBench.h (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.h:25: // Must specify: subsetWidth, subsetHeight,
offsetLeft, offsetTop
It might be clearer to have different constructors for different modes (or even
a factory), rather than having parameters which are only meaningful in the right
mode.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.h:36: // Choose subsets to mimic a user zooming out on a
photo
nit: zooming in* ?

[msarett, 2015-06-01 14:03:25]

I created a separate subclass for each benchmark.

It looks like bench.gypi has been changed to automatically
compile any file in the bench directory so I don't think
there are any gyp changes that are necessary.

So far, I've decided not to integrate these benchmarks into
nanobench.  It's a lot more useful for me right now to be
able to specify specific benchmarks parameters for specific
images.  We may want to replace the calls to
DecodingSubsetBench in nanobench with calls to these later
on.  I am not including any of my DEF_BENCH tests because
the images are not checked into resources.  Again, once
my results have stabilized, we may want to choose an image
or two to check in and test on.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.cpp
File bench/CodecSubsetBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:72: switch(fColorType) {
On 2015/05/29 15:36:02, scroggo wrote:
> What about kIndex8?

Agreed!

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:106: switch(fSubsetMode) {
On 2015/05/29 15:36:02, scroggo wrote:
> This giant switch statement might argue for using a different subclass for
each
> mode - it looks like we don't share much code amongst the modes.

Agreed.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:133: decoder->buildTileIndex(fStream->duplicate(),
&width, &height);
On 2015/05/29 15:36:02, scroggo wrote:
> Do we need to safely early exit in case buildTileIndex fails?

Let's check that the decode succeeds before creating the benchmark.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:149: uint32_t subsetWidth = info.width() / fDivisor;
On 2015/05/29 15:36:02, scroggo wrote:
> These variables could all be const?

Done.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:190: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(maxSubsetWidth, maxSubsetHeight,
On 2015/05/29 15:36:02, scroggo wrote:
> When using SkImageDecoder, you don't need to allocate any pixels - the decoder
> will do that for you.

The reason I allocate here is so that SkImageDecoder will reuse the same bitmap
for all of the subsets decoded below.  This is what SkCodec does above.  I think
that allows for a more fair comparison.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:250: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(fSubsetWidth, fSubsetHeight,
On 2015/05/29 15:36:02, scroggo wrote:
> Again, this is not necessary.

Acknowledged.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:287: // Note that if we subseted and scaled in a
single step, we could use the
On 2015/05/29 15:36:02, scroggo wrote:
> subsetted? (Technically not a verb, but it seems like it should have two 't's
if
> we add an 'ed'.)

Done.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:324: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(subsetWidth, subsetHeight,
On 2015/05/29 15:36:03, scroggo wrote:
> Again, not needed. There are some cases where a bitmap is passed in already
> allocated (which actually means it may make sense to test that), but they
> wouldn't be allocating a new pixelref each time (I think?)

Agreed that this allocation is unnecessary here.  We use a different bitmap for
each subset anyway.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.h
File bench/CodecSubsetBench.h (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.h:25: // Must specify: subsetWidth, subsetHeight,
offsetLeft, offsetTop
On 2015/05/29 15:36:03, scroggo wrote:
> It might be clearer to have different constructors for different modes (or
even
> a factory), rather than having parameters which are only meaningful in the
right
> mode.

I am splitting the benchmark into separate subclasses.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.h:36: // Choose subsets to mimic a user zooming out on a
photo
On 2015/05/29 15:36:03, scroggo wrote:
> nit: zooming in* ?

It mimics zooming out.  The subsets get bigger each time, meaning that more of
the image is being displayed (and it has be scaled down in a subsequent step).

Of course zooming in may be a more likely use case, and the performance results
should be identical to those if we started with big subsets and got smaller.

I'm changing the comment to indicate that it measures both.

[scroggo, 2015-06-01 17:25:15]

On 2015/06/01 14:03:25, msarett wrote:
> I created a separate subclass for each benchmark.

Thanks! Do you think it makes sense to split up SkCodec vs SkImageDecoder as
well? I can see an advantage to keeping them together, since you can easily see
the difference between the two in one file.

> 
> It looks like bench.gypi has been changed to automatically
> compile any file in the bench directory so I don't think
> there are any gyp changes that are necessary.

Oh, nice!

> 
> So far, I've decided not to integrate these benchmarks into
> nanobench.  It's a lot more useful for me right now to be
> able to specify specific benchmarks parameters for specific
> images.  We may want to replace the calls to
> DecodingSubsetBench in nanobench with calls to these later
> on.

They don't have to be on by default, but it will be a lot more convenient for
Emmalee or I to run them if we don't have to duplicate the work you did to make
nanobench run them first.

I realize one issue is picking the right subsets to decode, which depends on the
size of the image. I can think of a couple ways to address this:

- decode the size of the image, then generate some reasonable subsets (I suppose
you have some process in your mind for picking subsets - can we convert that to
code?)
- allow a command line flag for picking subsets

> I am not including any of my DEF_BENCH tests because
> the images are not checked into resources.  Again, once
> my results have stabilized, we may want to choose an image
> or two to check in and test on.

Rather than checking in image files, I would prefer to add them to Google
Storage (where our other test images are).

I do not think we should use DEF_BENCH - just like DecodingSubsetBench, I think
it makes more sense to be able to specify images on the command line.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.cpp
File bench/CodecSubsetBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:190: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(maxSubsetWidth, maxSubsetHeight,
On 2015/06/01 14:03:24, msarett wrote:
> On 2015/05/29 15:36:02, scroggo wrote:
> > When using SkImageDecoder, you don't need to allocate any pixels - the
decoder
> > will do that for you.
> 
> The reason I allocate here is so that SkImageDecoder will reuse the same
bitmap
> for all of the subsets decoded below.  This is what SkCodec does above.  I
think
> that allows for a more fair comparison.

Got it. We talked about this in person and you convinced me this makes sense.
Can you add a comment, so I don't forget that it makes sense?

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h
File bench/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:17: static bool valid_subset_bench(SkMemoryStream*
stream, SkColorType colorType, bool useCodec) {
Does this need to be an SkMemoryStream? In general, a parameter type should be
as general as it can be. Maybe it should be an SkData, and this function can
create the SkMemoryStream. (Or even call NewFromData, in the case of SkCodec.)

It looks like all the callers are going to start with an SkString path. Maybe
you should just take a path and consolidate the code for reading it and creating
the stream?

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:19: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromStream(stream->duplicate()));
Not sure whether we need to duplicate the stream here. I suppose it would be a
problem if the stream did not duplicate, but an SkMemoryStream should never
fail.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:21: return false;
Can you add a debug statement? This seems like a bug.

If you make this function take an SkString of the path, as I suggested above,
you can make all the error conditions print the file name, so we know which file
failed.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:28: return false;
Maybe add a debug statement here, as well?

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:31: const uint32_t subsetWidth = info.width() / 2;
It seems somewhat weird that we decode an arbitrary subset here. Is the thinking
that if we can successfully decode the last subset, we ought to be able to
decode them all?

I was imagining that valid_subset_bench would catch where we cannot even begin -
i.e. getScanlineDecoder returns NULL or buildTileIndex fails (the former can
just happen on some images; the latter *should* always succeed, but it could
fail...)

If we fail partway, maybe that is something we can depend on DM to catch?

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:35: SkDebugf("Could not allocate memory.  Aborting
bench.\n");
Skipping*

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:39: if (SkImageGenerator::kSuccess !=
scanlineDecoder->skipScanlines(subsetHeight)) {
We probably want a debug statement here, too. Although we *do* want to exit
here, since we don't have anything to time, this is an error, so we should not
fail silently.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:47: memcpy(bitmap.getAddr(0, y), row.get(), subsetWidth
* bpp);
This seems unnecessary. We're trying to make sure it works, but this does not
give us any information about whether it succeeded.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:55: if (!decoder->buildTileIndex(stream->duplicate(),
&width, &height)) {
If width and height are not 1, isn't this a bug for it to fail? Probably worth a
debug statement.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
File bench/SubsetDivisorBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.cpp:60: colorName = "Gray8";
Index8*

This whole block should probably go inside a function in SubsetBenchPriv.h, so
it can be shared by all of the benchmarks.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.cpp:99: if
(!bitmap.tryAllocPixels(info.makeWH(maxSubsetWidth, maxSubsetHeight))) {
Inside the benches, maybe we should call bitmap.allocPixels() instead. IIRC,
that version crashes on failure. That way we don't get a fake result which we
can only know to ignore by reading the log output.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBench.h
File bench/SubsetDivisorBench.h (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:20: class SubsetDivisorBench : public Benchmark {
I remember you were concerned about having so many different classes/files.
Maybe we should put all of these in a folder inside bench? (Although maybe hold
off on that so it's easier to see the changes across revisions in the reitveld
code review tool... If we decide we want them in their own folder, you can do
the move as its own patch set, either now or once we're happy with them.)

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:23: static SubsetDivisorBench* Create(SkString* path,
Why does this take a pointer to a path? Why not a const ref?

(Typically we use a pointer one of the following is true:

A. the function will modify the object
B. the pointer can be NULL

Neither appears to be the case here.)

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:41: SkString fName;
style nit: typically we line up the names of member variables. e.g.

SkString    fName;
SkColorType fColorType;

etc.

[msarett, 2015-06-01 20:37:02]

Just as you said, I felt it was useful to keep Codec and ImageDecoder in the
same file in order to easily compare what was being benchmarked (especially
since these benchmarks were created to measure the two implementations against
each other).

You are correct about nanobench - if I go ahead and integrate it now, it will
help us all in the future.  I have now modified nanobench to run the new
benchmarks.  I tried to find a balance between covering interesting cases and
not running too many.  Let me know what you think.

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench.cpp
File bench/CodecSubsetBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/20001/bench/CodecSubsetBench....
bench/CodecSubsetBench.cpp:190: if
(!bitmap.tryAllocPixels(SkImageInfo::Make(maxSubsetWidth, maxSubsetHeight,
On 2015/06/01 17:25:14, scroggo wrote:
> On 2015/06/01 14:03:24, msarett wrote:
> > On 2015/05/29 15:36:02, scroggo wrote:
> > > When using SkImageDecoder, you don't need to allocate any pixels - the
> decoder
> > > will do that for you.
> > 
> > The reason I allocate here is so that SkImageDecoder will reuse the same
> bitmap
> > for all of the subsets decoded below.  This is what SkCodec does above.  I
> think
> > that allows for a more fair comparison.
> 
> Got it. We talked about this in person and you convinced me this makes sense.
> Can you add a comment, so I don't forget that it makes sense?

Done.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h
File bench/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:17: static bool valid_subset_bench(SkMemoryStream*
stream, SkColorType colorType, bool useCodec) {
On 2015/06/01 17:25:14, scroggo wrote:
> Does this need to be an SkMemoryStream? In general, a parameter type should be
> as general as it can be. Maybe it should be an SkData, and this function can
> create the SkMemoryStream. (Or even call NewFromData, in the case of SkCodec.)
> 
> It looks like all the callers are going to start with an SkString path. Maybe
> you should just take a path and consolidate the code for reading it and
creating
> the stream?

Yes, I think this makes a lot more sense.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:19: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromStream(stream->duplicate()));
On 2015/06/01 17:25:14, scroggo wrote:
> Not sure whether we need to duplicate the stream here. I suppose it would be a
> problem if the stream did not duplicate, but an SkMemoryStream should never
> fail. 

Acknowledged.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:21: return false;
On 2015/06/01 17:25:14, scroggo wrote:
> Can you add a debug statement? This seems like a bug.
> 
> If you make this function take an SkString of the path, as I suggested above,
> you can make all the error conditions print the file name, so we know which
file
> failed.

I agree!  Adding informative error messages through this function.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:28: return false;
On 2015/06/01 17:25:14, scroggo wrote:
> Maybe add a debug statement here, as well?

Done.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:31: const uint32_t subsetWidth = info.width() / 2;
On 2015/06/01 17:25:14, scroggo wrote:
> It seems somewhat weird that we decode an arbitrary subset here. Is the
thinking
> that if we can successfully decode the last subset, we ought to be able to
> decode them all?
> 
> I was imagining that valid_subset_bench would catch where we cannot even begin
-
> i.e. getScanlineDecoder returns NULL or buildTileIndex fails (the former can
> just happen on some images; the latter *should* always succeed, but it could
> fail...)
> 
> If we fail partway, maybe that is something we can depend on DM to catch?

Yeah I thought this was awkward too.  I didn't want to duplicate all of the
different benchmark code here, but I thought we might want to check that subsets
actually decode.  As you say, they *should* work if the scanline/buildTileIndex
work, but there isn't a guarantee.  Similarly, I was thinking all subsets
*should* work if one works but there isn't a guarantee for that either.  So
essentially, you're right, decoding an arbitrary subset does not provide us with
anything useful beyond what we already check.

I would agree that depending on dm is a reasonable solution.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:35: SkDebugf("Could not allocate memory.  Aborting
bench.\n");
On 2015/06/01 17:25:14, scroggo wrote:
> Skipping*

Acknowledged.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:39: if (SkImageGenerator::kSuccess !=
scanlineDecoder->skipScanlines(subsetHeight)) {
On 2015/06/01 17:25:14, scroggo wrote:
> We probably want a debug statement here, too. Although we *do* want to exit
> here, since we don't have anything to time, this is an error, so we should not
> fail silently.

Acknowledged.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:47: memcpy(bitmap.getAddr(0, y), row.get(), subsetWidth
* bpp);
On 2015/06/01 17:25:14, scroggo wrote:
> This seems unnecessary. We're trying to make sure it works, but this does not
> give us any information about whether it succeeded.

Acknowledged.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:55: if (!decoder->buildTileIndex(stream->duplicate(),
&width, &height)) {
On 2015/06/01 17:25:14, scroggo wrote:
> If width and height are not 1, isn't this a bug for it to fail? Probably worth
a
> debug statement.

Acknowledged.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
File bench/SubsetDivisorBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.cpp:60: colorName = "Gray8";
On 2015/06/01 17:25:14, scroggo wrote:
> Index8*
> 
> This whole block should probably go inside a function in SubsetBenchPriv.h, so
> it can be shared by all of the benchmarks.

Done.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.cpp:99: if
(!bitmap.tryAllocPixels(info.makeWH(maxSubsetWidth, maxSubsetHeight))) {
On 2015/06/01 17:25:14, scroggo wrote:
> Inside the benches, maybe we should call bitmap.allocPixels() instead. IIRC,
> that version crashes on failure. That way we don't get a fake result which we
> can only know to ignore by reading the log output.

Yes this makes sense.  And it has the additional benefit of getting rid of an
error check inside the benchmark loop.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBench.h
File bench/SubsetDivisorBench.h (right):

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:20: class SubsetDivisorBench : public Benchmark {
On 2015/06/01 17:25:15, scroggo wrote:
> I remember you were concerned about having so many different classes/files.
> Maybe we should put all of these in a folder inside bench? (Although maybe
hold
> off on that so it's easier to see the changes across revisions in the reitveld
> code review tool... If we decide we want them in their own folder, you can do
> the move as its own patch set, either now or once we're happy with them.)

I will move them immediately after the next upload.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:23: static SubsetDivisorBench* Create(SkString* path,
On 2015/06/01 17:25:14, scroggo wrote:
> Why does this take a pointer to a path? Why not a const ref?
> 
> (Typically we use a pointer one of the following is true:
> 
> A. the function will modify the object
> B. the pointer can be NULL
> 
> Neither appears to be the case here.)

Agreed.  There is not a good reason for this to be a pointer.

https://codereview.chromium.org/1160953002/diff/40001/bench/SubsetDivisorBenc...
bench/SubsetDivisorBench.h:41: SkString fName;
On 2015/06/01 17:25:14, scroggo wrote:
> style nit: typically we line up the names of member variables. e.g.
> 
> SkString    fName;
> SkColorType fColorType;
> 
> etc.

Done.

[scroggo, 2015-06-02 13:45:57]

FYI: I reviewed patch set 3, assuming that the code is unchanged in patch set 4.

https://codereview.chromium.org/1160953002/diff/60001/bench/SubsetBenchPriv.h
File bench/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1160953002/diff/60001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:27: return "Gray8";
Index8

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (left):

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#old...
bench/nanobench.cpp:16: #include "DecodingSubsetBench.h"
If this is no longer used, can we remove this class entirely?

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:504: const SkImageInfo info = codec->getInfo();
don't we need to convert to colorType?

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:508: SkDebugf("Could not create scanline decoder for %s. 
Skipping bench.\n", path.c_str());
We might want to also report the colortype, since maybe that is why the scanline
decoder failed.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:514: SkAutoTDelete<SkImageDecoder>
decoder(SkImageDecoder::Factory(stream.detach()));
For legacy reasons, SkImageDecoder::Factory does not take ownership of the
stream, so you do not want to detach here. buildTileIndex, below, *will* take
ownership, so you can call detach there.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:800: int currentSubsetType = fCurrentSubsetType++;
nit: 4 space indent

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:824: fCurrentSubsetType = 0;
Don't we need to reset this when we reach kLast_SubsetType?

[msarett, 2015-06-02 17:14:11]

Patch Set 4 contained a few changes to the include dirs in gyp/bench.gypi, but
otherwise was identical to Patch Set 3.

The latest version is tested more comprehensively and addresses your comments
from Patch Set 3.

https://codereview.chromium.org/1160953002/diff/60001/bench/SubsetBenchPriv.h
File bench/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1160953002/diff/60001/bench/SubsetBenchPriv.h...
bench/SubsetBenchPriv.h:27: return "Gray8";
On 2015/06/02 13:45:56, scroggo wrote:
> Index8

Done.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (left):

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#old...
bench/nanobench.cpp:16: #include "DecodingSubsetBench.h"
On 2015/06/02 13:45:57, scroggo wrote:
> If this is no longer used, can we remove this class entirely?

Done.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:504: const SkImageInfo info = codec->getInfo();
On 2015/06/02 13:45:57, scroggo wrote:
> don't we need to convert to colorType?

Done.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:508: SkDebugf("Could not create scanline decoder for %s. 
Skipping bench.\n", path.c_str());
On 2015/06/02 13:45:57, scroggo wrote:
> We might want to also report the colortype, since maybe that is why the
scanline
> decoder failed.

Done.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:514: SkAutoTDelete<SkImageDecoder>
decoder(SkImageDecoder::Factory(stream.detach()));
On 2015/06/02 13:45:57, scroggo wrote:
> For legacy reasons, SkImageDecoder::Factory does not take ownership of the
> stream, so you do not want to detach here. buildTileIndex, below, *will* take
> ownership, so you can call detach there.

Nice catch, thanks!

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:800: int currentSubsetType = fCurrentSubsetType++;
On 2015/06/02 13:45:57, scroggo wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/1160953002/diff/60001/bench/nanobench.cpp#new...
bench/nanobench.cpp:824: fCurrentSubsetType = 0;
On 2015/06/02 13:45:57, scroggo wrote:
> Don't we need to reset this when we reach kLast_SubsetType?

Yes for sure!  It appears I should have tested this for more than one image.

[scroggo, 2015-06-04 13:50:05]

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:507: SkScanlineDecoder* scanlineDecoder =
codec->getScanlineDecoder(info);
If colortype is Index8, this will always fail, right?

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:826: fCurrentSubsetType = 0;
Is this necessary? Won't we break out of this loop and go to the next line which
sets fCurrentSubsetType to 0?

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
File bench/subset/SubsetDivisorBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
bench/subset/SubsetDivisorBench.cpp:89: SkColorTable* colorTable =
SkNEW_ARGS(SkColorTable, (colors, 0));
Maybe add a comment that it's okay for these colors to be uninitialized, since
we never use them - they're only here to satisfy allocPixels.

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
bench/subset/SubsetDivisorBench.cpp:106: // use case of BitmapRegionDecoder.;
Semicolon not needed here.

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetTra...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetTra...
bench/subset/SubsetTranslateBench.cpp:89: SkColorTable* colorTable =
SkNEW_ARGS(SkColorTable, (colors, 0));
Similar comment explaining why we're using uninitialized colors.

[msarett, 2015-06-05 17:11:21]

Patchset #6 (id:120001) has been deleted

[msarett, 2015-06-08 16:28:33]

Meant to publish this Friday:

This isn't ready to go yet.  It is currently failing subset decodes for webp
images when the color type is kIndex8 or kGray8.

If we request that the webp subset decoder decode to a color type that it
doesn't like (ex: kIndex8, kGray8), it will default to kN32.  Then it will call
cropSubset() in SkImageDecoder.cpp, which will try to create a canvas in order
to complete the subset decode (this fails because canvas does not support
kIndex8 or kGray8).

cropBitmap() does have a special case where it uses SkBitmap->extractSubset()
for color types that the canvas does not support.  It is possible to modify this
case to catch the failures mentioned above.  This stops the code from crashing,
but does not work properly, because extractSubset() will simply overwrite the
dstBitmap color type with whatever the srcBitmap color type is.

It seems to me that it makes the most sense to return false or some kind of
error signal when we try to decode webp subsets to unsupported color types
(rather than silently deciding to use kN32).  But I'm not sure if this might
have other implications.  Let me know what you think.

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:507: SkScanlineDecoder* scanlineDecoder =
codec->getScanlineDecoder(info);
On 2015/06/04 13:50:04, scroggo wrote:
> If colortype is Index8, this will always fail, right?

Umm yeah it returns NULL in Release mode.  But there is also an SkASSERT, so
this crashes in Debug mode.  Probably not what we want.

Looks like I need to provide a color table and use the more complex version of
this function.

https://codereview.chromium.org/1160953002/diff/100001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:826: fCurrentSubsetType = 0;
On 2015/06/04 13:50:04, scroggo wrote:
> Is this necessary? Won't we break out of this loop and go to the next line
which
> sets fCurrentSubsetType to 0?

Yes we will!

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
File bench/subset/SubsetDivisorBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
bench/subset/SubsetDivisorBench.cpp:89: SkColorTable* colorTable =
SkNEW_ARGS(SkColorTable, (colors, 0));
On 2015/06/04 13:50:04, scroggo wrote:
> Maybe add a comment that it's okay for these colors to be uninitialized, since
> we never use them - they're only here to satisfy allocPixels.

Done.

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetDiv...
bench/subset/SubsetDivisorBench.cpp:106: // use case of BitmapRegionDecoder.;
On 2015/06/04 13:50:04, scroggo wrote:
> Semicolon not needed here.

Done.

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetTra...
File bench/subset/SubsetTranslateBench.cpp (right):

https://codereview.chromium.org/1160953002/diff/100001/bench/subset/SubsetTra...
bench/subset/SubsetTranslateBench.cpp:89: SkColorTable* colorTable =
SkNEW_ARGS(SkColorTable, (colors, 0));
On 2015/06/04 13:50:04, scroggo wrote:
> Similar comment explaining why we're using uninitialized colors.

Done.

[msarett, 2015-06-08 16:28:37]

Meant to publish this Friday:

This isn't ready to go yet.  It is currently failing subset decodes for webp
images when the color type is kIndex8 or kGray8.

If we request that the webp subset decoder decode to a color type that it
doesn't like (ex: kIndex8, kGray8), it will default to kN32.  Then it will call
cropSubset() in SkImageDecoder.cpp, which will try to create a canvas in order
to complete the subset decode (this fails because canvas does not support
kIndex8 or kGray8).

cropBitmap() does have a special case where it uses SkBitmap->extractSubset()
for color types that the canvas does not support.  It is possible to modify this
case to catch the failures mentioned above.  This stops the code from crashing,
but does not work properly, because extractSubset() will simply overwrite the
dstBitmap color type with whatever the srcBitmap color type is.

It seems to me that it makes the most sense to return false or some kind of
error signal when we try to decode webp subsets to unsupported color types
(rather than silently deciding to use kN32).  But I'm not sure if this might
have other implications.  Let me know what you think.

[scroggo, 2015-06-08 18:06:43]

On 2015/06/08 16:28:37, msarett wrote:
> Meant to publish this Friday:
> 
> This isn't ready to go yet.  It is currently failing subset decodes for webp
> images when the color type is kIndex8 or kGray8.
> 
> If we request that the webp subset decoder decode to a color type that it
> doesn't like (ex: kIndex8, kGray8), it will default to kN32.  Then it will
call
> cropSubset() in SkImageDecoder.cpp, which will try to create a canvas in order
> to complete the subset decode (this fails because canvas does not support
> kIndex8 or kGray8).
> 
> cropBitmap() does have a special case where it uses SkBitmap->extractSubset()
> for color types that the canvas does not support.  It is possible to modify
this
> case to catch the failures mentioned above. 

I'm surprised it doesn't work for kIndex8 - I think that is why that code (the
call to extractSubset) was written to begin with. It does not surprise me that
it
does not work with kGray8, which did not exist when this code was written.

> This stops the code from crashing,
> but does not work properly, because extractSubset() will simply overwrite the
> dstBitmap color type with whatever the srcBitmap color type is.
> 
> It seems to me that it makes the most sense to return false or some kind of
> error signal when we try to decode webp subsets to unsupported color types
> (rather than silently deciding to use kN32).  But I'm not sure if this might
> have other implications.  Let me know what you think.

Agreed. I assume you mean in the tests? We don't want to change the behavior of
SkImageDecoder itself (at least right now - maybe file a bug about this?), since
we need to make sure BitmapRegionDecoder's behavior does not change.
(Technically,
an Android client (from Java) cannot request kIndex8 or kGray8, so it should not
be a problem to special case those two, but I think it would be better to be
consistent.) But yes, I think it's fine for the benchmark to report some kind of
failure.

FWIW, I don't know that region decoding Index8 bitmaps happens very often in
practice.
I added the code to handle Index8 with https://codereview.chromium.org/14567011,
and
that was only because I wrote tests - no one reported a bug that it was
crashing. I
think the only way to do it (from Java code on Android) would be to use a NULL
BitmapFactory.Options object, or to set its inPreferredConfig field to NULL
(which
defaults to the Java version of N32) *and* to use an image encoded as index8.

I would be content with leaving a TODO for kIndex8/kGray8 for now.

https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:508: int colorCount = 0;
Why did you use a colorCount of 0?

[msarett, 2015-06-08 23:40:48]

This check will avoid crashes for now.  I filed a bug for webp subset decodes to
look into this further.

https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:508: int colorCount = 0;
On 2015/06/08 18:06:42, scroggo wrote:
> Why did you use a colorCount of 0?

SkCodec is required to initialize the colorCount if kIndex8 is used.  I would
agree that it's better to leave it uninitialized here.

[scroggo, 2015-06-09 12:58:14]

lgtm

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:526: if (SkImageDecoder::kWEBP_Format ==
decoder->getFormat() &&
How is this only a WEBP problem? What is different about running on a PNG which
is not indexed, for example?

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:527: (kIndex_8_SkColorType == colorType ||
kGray_8_SkColorType == colorType)) {
Please add a FIXME: See skbug.com/####, and/or an explanation of the problem.

[msarett, 2015-06-09 20:49:54]

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:526: if (SkImageDecoder::kWEBP_Format ==
decoder->getFormat() &&
On 2015/06/09 12:58:13, scroggo wrote:
> How is this only a WEBP problem? What is different about running on a PNG
which
> is not indexed, for example?

You are right.  This occurs for pngs and jpegs as well (and would occur for
other types if we had subset decoders).  I missed it because we only see this
running in Debug mode on Android.

https://codereview.chromium.org/1160953002/diff/160001/bench/nanobench.cpp#ne...
bench/nanobench.cpp:527: (kIndex_8_SkColorType == colorType ||
kGray_8_SkColorType == colorType)) {
On 2015/06/09 12:58:13, scroggo wrote:
> Please add a FIXME: See skbug.com/####, and/or an explanation of the problem.

Done.

[msarett, 2015-06-09 20:50:08]

The CQ bit was checked by msarett@google.com

[msarett, 2015-06-09 20:50:09]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1160953002/#ps180001
(title: "Added FIXME")

[commit-bot: I haz the power, 2015-06-09 20:50:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160953002/180001

[commit-bot: I haz the power, 2015-06-09 20:56:14]

Committed patchset #8 (id:180001) as
https://skia.googlesource.com/skia/+/b23e6aa6767816ac4bc6c191e78ef62d6e765671