Fix CodecSubset benches seg faults for kIndex8

Fix CodecSubset benches seg faults for kIndex8

All of the CodecSubset benches fail when the color type is
kIndex8.  We need to pass a color table to allocPixels()
when we want to decode to kIndex8 or it will throw a failure.


BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7f6283bdf8926f72b886389588e3e2d3bc0ea066

[msarett, 2015-06-30 16:01:18]

Thanks for catching this Emmalee!

This should fix it.  Patch Set 1 is also a potential fix, but I think Patch Set
2 is a better way to fix it.

[msarett, 2015-06-30 16:02:11]

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

[msarett, 2015-06-30 16:02:11]

Adding reviewers

[scroggo, 2015-06-30 16:40:19]

> Fix subset decoding bench bug for Index8 decodes

nit: can you update the description to be more specific about the bug you are
fixing?

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

https://codereview.chromium.org/1213983003/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:58: int colorCount = 0;
This got left out of patch set 2, but according to
https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp#ne...,
we need to initialize it?

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
File bench/subset/SubsetSingleBench.cpp (right):

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
bench/subset/SubsetSingleBench.cpp:73: if (kIndex_8_SkColorType == fColorType) {
I agree that this is better (I think in patch set 1, we would leak the color
table in the case where it is not kIndex_8).

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
bench/subset/SubsetSingleBench.cpp:75: SkNEW_ARGS(SkColorTable, (colors,
colorCount)));
According to the documentation for tryAllocPixels (which allocPixels calls),

If the ImageInfo requires a colortable, then ColorTable must be non-null, and
will be ref'd.

Digging into the constructor of SkMallocPixelRef (which this eventually calls),
it is in fact ref'd. So this will lead to a memory leak.

You'll want to put your colortable into an SkAutoTUnref before passing it to
this function.

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

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:71: if (kIndex_8_SkColorType ==
fColorType) {
This code is almost exactly the same. Can we write a common function? (Is there
already a file for common functions for bench decodes?)

[msarett, 2015-06-30 18:39:11]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-06-30 18:40:28]

Sharing code in SubsetBenchPriv

No longer leaking SkColorTable

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

https://codereview.chromium.org/1213983003/diff/1/bench/subset/SubsetTranslat...
bench/subset/SubsetTranslateBench.cpp:58: int colorCount = 0;
On 2015/06/30 16:40:19, scroggo wrote:
> This got left out of patch set 2, but according to
>
https://codereview.chromium.org/1160953002/diff/140001/bench/nanobench.cpp#ne...,
> we need to initialize it?

I switched away from this approach because I thought it was confusing and also
probably worse performance (although this is untested and I doubt it really
makes a difference).  But clearly, it's confusing.

My thinking was that colorCount will only be set to a non-zero value on a
kIndex8 decode.  So then when we initialize the color table below:
1) If it is kIndex8 a color table will be allocated normally.
2) If colorCount is still 0, the constructor for SkColorTable essentially does
nothing.

So this is a really confusing way to get rid of an if statement (and always
construct an SkColorTable, even if it is empty).  Probably not a good approach.

You're right, in the normal approach we don't need to initialize colorCount.

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
File bench/subset/SubsetSingleBench.cpp (right):

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
bench/subset/SubsetSingleBench.cpp:73: if (kIndex_8_SkColorType == fColorType) {
On 2015/06/30 16:40:19, scroggo wrote:
> I agree that this is better (I think in patch set 1, we would leak the color
> table in the case where it is not kIndex_8).

Acknowledged.

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetSing...
bench/subset/SubsetSingleBench.cpp:75: SkNEW_ARGS(SkColorTable, (colors,
colorCount)));
On 2015/06/30 16:40:19, scroggo wrote:
> According to the documentation for tryAllocPixels (which allocPixels calls),
> 
> If the ImageInfo requires a colortable, then ColorTable must be non-null, and
> will be ref'd.
> 
> Digging into the constructor of SkMallocPixelRef (which this eventually
calls),
> it is in fact ref'd. So this will lead to a memory leak.
> 
> You'll want to put your colortable into an SkAutoTUnref before passing it to
> this function.

You're right.  Sorry should have put a little more thought into this.

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

https://codereview.chromium.org/1213983003/diff/20001/bench/subset/SubsetTran...
bench/subset/SubsetTranslateBench.cpp:71: if (kIndex_8_SkColorType ==
fColorType) {
On 2015/06/30 16:40:19, scroggo wrote:
> This code is almost exactly the same. Can we write a common function? (Is
there
> already a file for common functions for bench decodes?)

Yes there is, good idea.

[scroggo, 2015-06-30 18:48:51]

> All of the CodecSubset benches fail when the color type is
> kIndex8.  We need to pass a color table to allocPixels()
> when we want to decode to kIndex8 or it will throw a
> failure.

Nit: Can you add the subject line + blank line to the description? (Typically we
use a one line short description followed by a blank line and then a longer
description.)

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

https://codereview.chromium.org/1213983003/diff/60001/bench/subset/SubsetBenc...
bench/subset/SubsetBenchPriv.h:35: static inline void alloc_pixels(SkBitmap
bitmap, SkColorType colorType, int width, int height,
SkBitmap is a parameter passed by value, so this will not affect the bitmap you
pass to this function. You should pass a pointer to an SkBitmap.

[msarett, 2015-06-30 19:06:38]

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

https://codereview.chromium.org/1213983003/diff/60001/bench/subset/SubsetBenc...
bench/subset/SubsetBenchPriv.h:35: static inline void alloc_pixels(SkBitmap
bitmap, SkColorType colorType, int width, int height,
On 2015/06/30 18:48:50, scroggo wrote:
> SkBitmap is a parameter passed by value, so this will not affect the bitmap
you
> pass to this function. You should pass a pointer to an SkBitmap.

Of course thanks!  Not sure how I didn't catch that.

[scroggo, 2015-06-30 19:12:00]

https://codereview.chromium.org/1213983003/diff/80001/bench/subset/SubsetBenc...
File bench/subset/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1213983003/diff/80001/bench/subset/SubsetBenc...
bench/subset/SubsetBenchPriv.h:37: if (kIndex_8_SkColorType == colorType) {
Does colorType need to be a separate parameter? It seems like you could get it
from info? (If not, it seems like we're doing the wrong thing, since info needs
to be Index8 to use the colortable.)

[msarett, 2015-06-30 19:36:00]

https://codereview.chromium.org/1213983003/diff/80001/bench/subset/SubsetBenc...
File bench/subset/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1213983003/diff/80001/bench/subset/SubsetBenc...
bench/subset/SubsetBenchPriv.h:37: if (kIndex_8_SkColorType == colorType) {
On 2015/06/30 19:12:00, scroggo wrote:
> Does colorType need to be a separate parameter? It seems like you could get it
> from info? (If not, it seems like we're doing the wrong thing, since info
needs
> to be Index8 to use the colortable.)

Agreed!

[scroggo, 2015-06-30 19:37:55]

lgtm

https://codereview.chromium.org/1213983003/diff/100001/bench/subset/SubsetBen...
File bench/subset/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1213983003/diff/100001/bench/subset/SubsetBen...
bench/subset/SubsetBenchPriv.h:35: static inline void alloc_pixels(SkBitmap*
bitmap, const SkImageInfo& info, SkPMColor* colors,
Maybe add a comment explaining what this does/why it is needed?

[msarett, 2015-06-30 20:21:30]

https://codereview.chromium.org/1213983003/diff/100001/bench/subset/SubsetBen...
File bench/subset/SubsetBenchPriv.h (right):

https://codereview.chromium.org/1213983003/diff/100001/bench/subset/SubsetBen...
bench/subset/SubsetBenchPriv.h:35: static inline void alloc_pixels(SkBitmap*
bitmap, const SkImageInfo& info, SkPMColor* colors,
On 2015/06/30 19:37:55, scroggo wrote:
> Maybe add a comment explaining what this does/why it is needed?

Done.

[msarett, 2015-06-30 20:21:56]

The CQ bit was checked by msarett@google.com

[msarett, 2015-06-30 20:21:56]

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/1213983003/#ps110001
(title: " ")

[commit-bot: I haz the power, 2015-06-30 20:22:19]

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

[commit-bot: I haz the power, 2015-06-30 20:29:40]

Committed patchset #6 (id:110001) as
https://skia.googlesource.com/skia/+/7f6283bdf8926f72b886389588e3e2d3bc0ea066