change SkColorTable to be immutable

change SkColorTable to be immutable

BUG=
R=scroggo@google.com

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

[scroggo, 2013-10-01 17:30:38]

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

https://codereview.chromium.org/25353002/diff/1/include/core/SkColorTable.h#n...
include/core/SkColorTable.h:89: uint8_t     fPad;
fPad is never used.

https://codereview.chromium.org/25353002/diff/1/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/1/src/core/SkColorTable.cpp#new...
src/core/SkColorTable.cpp:17: #if 0
If we're going to leave this code around, can we add a comment about why we
might want to revive it in the future?

https://codereview.chromium.org/25353002/diff/1/src/core/SkColorTable.cpp#new...
src/core/SkColorTable.cpp:111: fAlphaType = SkToU8(buffer.readUInt());
If we don't update the PICTURE_VERSION, this will result in the wrong value
being read. That might be okay though...

[scroggo, 2013-10-01 17:38:28]

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

https://codereview.chromium.org/25353002/diff/1/include/core/SkColorTable.h#n...
include/core/SkColorTable.h:63: void unlockColors();
Callers need to be updated in src/images/SkImageDecoder_libpng.cpp and
SkImageDecoder_libgif.cpp

There are also some callers in Android that will need to be updated.

[scroggo, 2013-10-02 21:16:26]

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

https://codereview.chromium.org/25353002/diff/1/include/core/SkColorTable.h#o...
include/core/SkColorTable.h:43: void    setFlags(unsigned flags);
We still have callers of setFlags and getFlags in images/

[scroggo, 2013-10-02 22:19:15]

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

https://codereview.chromium.org/25353002/diff/1/include/core/SkColorTable.h#n...
include/core/SkColorTable.h:29: SkColorTable(const SkPMColor colors[], int
count,
Comment that this makes a deep copy? (Or possibly make it optional?)

https://codereview.chromium.org/25353002/diff/1/include/core/SkColorTable.h#n...
include/core/SkColorTable.h:55: const SkPMColor* lockColors() {
We'll need to update the image decoders' use of color tables to create them with
the new constructor rather than editing after creation.

[reed1, 2013-10-07 21:09:04]

now with codec goodness. ptal

[scroggo, 2013-10-07 21:58:42]

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

https://codereview.chromium.org/25353002/diff/18001/include/core/SkColorTable...
include/core/SkColorTable.h:89: uint8_t     fPad;
unused.

https://codereview.chromium.org/25353002/diff/18001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:18: SkColorTable::SkColorTable(int count)
Why not remove this entirely?

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:228: SkPMColor colorPtr[256]; // storage
for worse-case
worst*

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:661: SkPMColor colorStorage[256];    //
worse-case storage
worst*

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:674: bool reallyHasAlpha = numTrans > 0;
This is going to change the behavior. Previously, if numTrans was greater than
0, but none of the supposed transparent colors were transparent, this would get
set to false. Now it will remain true.

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:704: SkAlphaType alphaType = reallyHasAlpha
? kPremul_SkAlphaType : kOpaque_SkAlphaType;
You need to check getRequireUnpremultipliedColors to determine the right alpha
type.

[reed1, 2013-10-09 12:19:54]

ptal

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

https://codereview.chromium.org/25353002/diff/18001/include/core/SkColorTable...
include/core/SkColorTable.h:89: uint8_t     fPad;
On 2013/10/07 21:58:42, scroggo wrote:
> unused.

Done.

https://codereview.chromium.org/25353002/diff/18001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:18: SkColorTable::SkColorTable(int count)
On 2013/10/07 21:58:42, scroggo wrote:
> Why not remove this entirely?

Done.

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:228: SkPMColor colorPtr[256]; // storage
for worse-case
On 2013/10/07 21:58:42, scroggo wrote:
> worst*

Done.

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:661: SkPMColor colorStorage[256];    //
worse-case storage
On 2013/10/07 21:58:42, scroggo wrote:
> worst*

Done.

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:674: bool reallyHasAlpha = numTrans > 0;
On 2013/10/07 21:58:42, scroggo wrote:
> This is going to change the behavior. Previously, if numTrans was greater than
> 0, but none of the supposed transparent colors were transparent, this would
get
> set to false. Now it will remain true.

Fixed

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:704: SkAlphaType alphaType = reallyHasAlpha
? kPremul_SkAlphaType : kOpaque_SkAlphaType;
On 2013/10/07 21:58:42, scroggo wrote:
> You need to check getRequireUnpremultipliedColors to determine the right alpha
> type.

Fixed

[reed1, 2013-10-09 15:01:38]

derek, I think leon might be out today.

[djsollen, 2013-10-09 15:42:18]

I'm late to the party so some of these questions may have already been answered.

Also do we really need to keep the lock/unlock mechanism around?  It is just a
debug bookkeeping block and I'm not sure how useful it really is.

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleDitherBi...
File samplecode/SampleDitherBitmap.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleDitherBi...
samplecode/SampleDitherBitmap.cpp:104: #if 0
what about this test changes due to this?  Can  we just remove this code instead
of #if 0?

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleTinyBitm...
File samplecode/SampleTinyBitmap.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleTinyBitm...
samplecode/SampleTinyBitmap.cpp:61: #if 0
same?

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:62: void SkColorTable::unlockColors() {
make this function inlined as well

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:253: SkPMColor colorPtr[256]; // storage
for worst-case
we should assert that colorCount is not greater than 256.

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:254: SkAlphaType alphaType =
kOpaque_SkAlphaType;
is it safe to assume always opaque without checking the reallyHasAlpha variable?

[reed1, 2013-10-09 15:51:25]

ptal

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleDitherBi...
File samplecode/SampleDitherBitmap.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleDitherBi...
samplecode/SampleDitherBitmap.cpp:104: #if 0
On 2013/10/09 15:42:18, djsollen wrote:
> what about this test changes due to this?  Can  we just remove this code
instead
> of #if 0?

Frankly, I think the sample (not even a gm) is uninteresting. It could be
rewritten to make a deep-copy of the colortable in order to change its
opaqueness. I think its ok to leave as a TODO (or remove).

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleTinyBitm...
File samplecode/SampleTinyBitmap.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/samplecode/SampleTinyBitm...
samplecode/SampleTinyBitmap.cpp:61: #if 0
On 2013/10/09 15:42:18, djsollen wrote:
> same?

same as other sample (not even gm) comment

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:62: void SkColorTable::unlockColors() {
On 2013/10/09 15:42:18, djsollen wrote:
> make this function inlined as well

Actually, I think we could go further and remove lock/unlock entirely (at least
for the 32bit colors). However, I think that can be a follow-on CL..?

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:253: SkPMColor colorPtr[256]; // storage
for worst-case
On 2013/10/09 15:42:18, djsollen wrote:
> we should assert that colorCount is not greater than 256.

Done.

https://codereview.chromium.org/25353002/diff/42001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libgif.cpp:254: SkAlphaType alphaType =
kOpaque_SkAlphaType;
On 2013/10/09 15:42:18, djsollen wrote:
> is it safe to assume always opaque without checking the reallyHasAlpha
variable?

That is detected on line 266

[scroggo, 2013-10-09 16:05:23]

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:674: bool reallyHasAlpha = numTrans > 0;
On 2013/10/09 12:19:54, reed1 wrote:
> On 2013/10/07 21:58:42, scroggo wrote:
> > This is going to change the behavior. Previously, if numTrans was greater
than
> > 0, but none of the supposed transparent colors were transparent, this would
> get
> > set to false. Now it will remain true.
> 
> Fixed

Still not fixed. Counterexample:

- numTrans > 0
- none of the "transparent" colors actually have an alpha < FF

The old code would have set reallyHasAlpha to false, but the new code would set
reallyHasAlpha to true. In order to retain the behavior, reallyHasAlpha should
be initialized to false.

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:62: void SkColorTable::unlockColors() {
On 2013/10/09 15:51:25, reed1 wrote:
> On 2013/10/09 15:42:18, djsollen wrote:
> > make this function inlined as well
> 
> Actually, I think we could go further and remove lock/unlock entirely (at
least
> for the 32bit colors). However, I think that can be a follow-on CL..?

Won't this function go away completely in release mode?

[reed1, 2013-10-09 16:16:36]

ptal

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25353002/diff/18001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:674: bool reallyHasAlpha = numTrans > 0;
On 2013/10/09 16:05:23, scroggo wrote:
> On 2013/10/09 12:19:54, reed1 wrote:
> > On 2013/10/07 21:58:42, scroggo wrote:
> > > This is going to change the behavior. Previously, if numTrans was greater
> than
> > > 0, but none of the supposed transparent colors were transparent, this
would
> > get
> > > set to false. Now it will remain true.
> > 
> > Fixed
> 
> Still not fixed. Counterexample:
> 
> - numTrans > 0
> - none of the "transparent" colors actually have an alpha < FF
> 
> The old code would have set reallyHasAlpha to false, but the new code would
set
> reallyHasAlpha to true. In order to retain the behavior, reallyHasAlpha should
> be initialized to false.

Done.

[reed1, 2013-10-09 16:17:38]

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/25353002/diff/42001/src/core/SkColorTable.cpp...
src/core/SkColorTable.cpp:62: void SkColorTable::unlockColors() {
On 2013/10/09 16:05:23, scroggo wrote:
> On 2013/10/09 15:51:25, reed1 wrote:
> > On 2013/10/09 15:42:18, djsollen wrote:
> > > make this function inlined as well
> > 
> > Actually, I think we could go further and remove lock/unlock entirely (at
> least
> > for the 32bit colors). However, I think that can be a follow-on CL..?
> 
> Won't this function go away completely in release mode?

Agreed. Just didn't want to change all the callers to not call it as part of
this CL, but I think we can do that soon.

[djsollen, 2013-10-09 16:19:50]

in general this 1gtm, but I'll defer to leon as this seems to have more
implications on the image decoders than anywhere else.

[scroggo, 2013-10-09 16:23:29]

On 2013/10/09 16:19:50, djsollen wrote:
> in general this 1gtm, but I'll defer to leon as this seems to have more
> implications on the image decoders than anywhere else.

I hope that was a deliberate '1' so the code review system wouldn't consider it
actually approved - I like it :)

LGTM

[reed1, 2013-10-09 16:34:56]

Committed patchset #10 manually as r11676 (presubmit successful).