Ensure that SkColorTable->fCount is set properly after decodes

Ensure that SkColorTable->fCount is set properly after decodes

We now have some blits that will process the color table.

If we erroneously report that the size of the color table is 256,
we will do extra work and annoy MSAN.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1982753002

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

[msarett, 2016-05-16 19:07:55]

Description was changed from

==========
Ensure that SkColorTable->fCount is set properly after decodes

We now have some blits that will process the color table.

If we erroneously report that the size of the color table is 256,
we will do extra work and annoy MSAN.

BUG=skia:
==========

to

==========
Ensure that SkColorTable->fCount is set properly after decodes

We now have some blits that will process the color table.

If we erroneously report that the size of the color table is 256,
we will do extra work and annoy MSAN.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-05-16 19:10:27]

msarett@google.com changed reviewers:
+ herb@google.com, reed@google.com, scroggo@google.com

[msarett, 2016-05-16 19:10:27]

Once this is settled, I'll need to make a similar change in BitmapFactory.cpp in
Android.

[scroggo, 2016-05-16 19:28:15]

lgtm

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode357
dm/DMSrcSink.cpp:357: void draw_to_canvas(SkCanvas* canvas, const SkImageInfo&
info, void* pixels, size_t rowBytes,
static?

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode398
dm/DMSrcSink.cpp:398: size_t rowBytes = size.width() * bpp;
nit: These variables could be const?

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode399
dm/DMSrcSink.cpp:399: SkAutoMalloc pixels(size.height() * rowBytes);
I think you could instead call decodeInfo.getSafeSize(decodeInfo.minRowBytes())
to determine the size of your storage. Not sure if that's better.

[herb_g, 2016-05-16 20:38:23]

lgtm

[reed1, 2016-05-17 13:48:38]

lgtm

https://codereview.chromium.org/1982753002/diff/1/include/core/SkColorTable.h
File include/core/SkColorTable.h (right):

https://codereview.chromium.org/1982753002/diff/1/include/core/SkColorTable.h...
include/core/SkColorTable.h:75: if (count < 0 || count > fCount) {
I wonder if we can add a SkASSERT(this->unique()); so we don't ever try to
change the ctable if there is more than one owner...

[msarett, 2016-05-17 15:29:35]

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode357
dm/DMSrcSink.cpp:357: void draw_to_canvas(SkCanvas* canvas, const SkImageInfo&
info, void* pixels, size_t rowBytes,
On 2016/05/16 19:28:15, scroggo wrote:
> static?

Thanks!  Also fixing this elsewhere...

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode398
dm/DMSrcSink.cpp:398: size_t rowBytes = size.width() * bpp;
On 2016/05/16 19:28:15, scroggo wrote:
> nit: These variables could be const?

Done.

https://codereview.chromium.org/1982753002/diff/1/dm/DMSrcSink.cpp#newcode399
dm/DMSrcSink.cpp:399: SkAutoMalloc pixels(size.height() * rowBytes);
On 2016/05/16 19:28:15, scroggo wrote:
> I think you could instead call
decodeInfo.getSafeSize(decodeInfo.minRowBytes())
> to determine the size of your storage. Not sure if that's better.

sgtm

https://codereview.chromium.org/1982753002/diff/1/include/core/SkColorTable.h
File include/core/SkColorTable.h (right):

https://codereview.chromium.org/1982753002/diff/1/include/core/SkColorTable.h...
include/core/SkColorTable.h:75: if (count < 0 || count > fCount) {
On 2016/05/17 13:48:38, reed1 wrote:
> I wonder if we can add a SkASSERT(this->unique()); so we don't ever try to
> change the ctable if there is more than one owner...

I tried.  I released the local ref early in SkBitmapRegionCodec, so that this
would work there.  But it breaks somewhere for image generator unfortunately.

[msarett, 2016-05-17 15:29:45]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 15:29:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982753002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982753002/20001

[commit-bot: I haz the power, 2016-05-17 15:49:57]

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

[commit-bot: I haz the power, 2016-05-17 15:49:58]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-05-17 15:51:09]

The CQ bit was checked by msarett@google.com

[msarett, 2016-05-17 15:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from herb@google.com,
scroggo@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/1982753002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-05-17 15:51:17]

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

[commit-bot: I haz the power, 2016-05-17 15:52:14]

Description was changed from

==========
Ensure that SkColorTable->fCount is set properly after decodes

We now have some blits that will process the color table.

If we erroneously report that the size of the color table is 256,
we will do extra work and annoy MSAN.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Ensure that SkColorTable->fCount is set properly after decodes

We now have some blits that will process the color table.

If we erroneously report that the size of the color table is 256,
we will do extra work and annoy MSAN.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/b1be46b1d938c21eafa7ded5560bcc10b71cd471
==========

[commit-bot: I haz the power, 2016-05-17 15:52:15]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/b1be46b1d938c21eafa7ded5560bcc10b71cd471

[msarett, 2016-05-17 19:54:58]

On 2016/05/16 19:10:27, msarett wrote:
> Once this is settled, I'll need to make a similar change in BitmapFactory.cpp
in
> Android.

Started thinking about how to fix this in BitmapFactory.cpp in Android.

We are very much committed to using bitmap.tryAllocPixels(Allocator,
SkColorTable), which means that we need to have an SkColorTable before we start
the decode.  And we can't really fix the SkColorTable after the decode because
BitmapFactory can't access dangerous_overwrite().

I think we'll need to use Leon's suggestion and change the SkCodec::getPixels()
API to take an SkPixmap (which has an SkColorTable).  Then, the codec be able to
modify the color table as it decodes.