Simple PKM image decoder.

Simple PKM image decoder.

This decoder takes a PKM file as produced by the android SDK tool 'etc1tool'. It then decodes the file using the same software used to create the ETC1 encoding from the AOSP.

Hal: Image Decoder
Eric: gyp includes

[robertphillips, 2014-05-22 14:41:34]

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode1
gm/etc1bitmap.cpp:1: /*
2014

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode15
gm/etc1bitmap.cpp:15: /**
Update comment.

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode22
gm/etc1bitmap.cpp:22: protected:
add SK_OVERRIDE after ever overridden method.

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode51
gm/etc1bitmap.cpp:51: 
Use DEF_GM macro

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
File src/images/SkImageDecoder_pkm.cpp (right):

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:1: /*
Please use new header (not Android one).

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:32: 
Put this in the class?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:55: width, height)
Move the ") {" to the prior line?
Can it all fit on one 100 col line?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:59: 
one 100 col line?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:81: // Decode ETC1
space after if?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:88: uint8_t *dst = reinterpret_cast<uint8_t
*>(bm->getPixels());
++i ?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:97: return true;
rm "//onDecode" ? Not really standard.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:99: 
Why are the "///*" lines different length. I think they should be 80.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:104: static bool is_pkm(SkStreamRewindable*
stream) {
are ... ?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:105: // Check to see if the first four bytes
are
Do we really need an AutoMalloc for 6 bytes? Can we just put it on the stack?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:107: unsigned char* buf = (unsigned
char*)autoMal.get();
space after if ?

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:111: 
return etc1_pkm_is_valid(buf); ?

https://codereview.chromium.org/292663011/diff/1/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/292663011/diff/1/tests/ImageDecodingTest.cpp#...
tests/ImageDecodingTest.cpp:54: // decoders do not, so skip them as well.
Shouldn't this go in the opaque block (at the end)?

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
File third_party/externals/etc1/etc1.cpp (right):

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
third_party/externals/etc1/etc1.cpp:14: 
Add fork comment?

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
File third_party/externals/etc1/etc1.h (right):

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
third_party/externals/etc1/etc1.h:13: // limitations under the License.
Add fork comment ?

[krajcevski, 2014-05-22 15:32:17]

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode1
gm/etc1bitmap.cpp:1: /*
On 2014/05/22 14:41:35, robertphillips wrote:
> 2014

Done.

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode15
gm/etc1bitmap.cpp:15: /**
On 2014/05/22 14:41:35, robertphillips wrote:
> Update comment.

Done.

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode22
gm/etc1bitmap.cpp:22: protected:
On 2014/05/22 14:41:35, robertphillips wrote:
> add SK_OVERRIDE after ever overridden method.

Done.

https://codereview.chromium.org/292663011/diff/1/gm/etc1bitmap.cpp#newcode51
gm/etc1bitmap.cpp:51: 
On 2014/05/22 14:41:35, robertphillips wrote:
> Use DEF_GM macro

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
File src/images/SkImageDecoder_pkm.cpp (right):

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:1: /*
On 2014/05/22 14:41:35, robertphillips wrote:
> Please use new header (not Android one).

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:32: 
On 2014/05/22 14:41:35, robertphillips wrote:
> Put this in the class?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:55: width, height)
On 2014/05/22 14:41:35, robertphillips wrote:
> Move the ") {" to the prior line?
> Can it all fit on one 100 col line?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:59: 
On 2014/05/22 14:41:35, robertphillips wrote:
> one 100 col line?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:81: // Decode ETC1
On 2014/05/22 14:41:35, robertphillips wrote:
> space after if?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:88: uint8_t *dst = reinterpret_cast<uint8_t
*>(bm->getPixels());
On 2014/05/22 14:41:35, robertphillips wrote:
> ++i ?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:97: return true;
On 2014/05/22 14:41:35, robertphillips wrote:
> rm "//onDecode" ? Not really standard.

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:99: 
On 2014/05/22 14:41:35, robertphillips wrote:
> Why are the "///*" lines different length. I think they should be 80.

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:104: static bool is_pkm(SkStreamRewindable*
stream) {
On 2014/05/22 14:41:35, robertphillips wrote:
> are ... ?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:105: // Check to see if the first four bytes
are
On 2014/05/22 14:41:35, robertphillips wrote:
> Do we really need an AutoMalloc for 6 bytes? Can we just put it on the stack?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:107: unsigned char* buf = (unsigned
char*)autoMal.get();
On 2014/05/22 14:41:35, robertphillips wrote:
> space after if ?

Done.

https://codereview.chromium.org/292663011/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:111: 
On 2014/05/22 14:41:35, robertphillips wrote:
> return etc1_pkm_is_valid(buf); ?

Done.

https://codereview.chromium.org/292663011/diff/1/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/292663011/diff/1/tests/ImageDecodingTest.cpp#...
tests/ImageDecodingTest.cpp:54: // decoders do not, so skip them as well.
On 2014/05/22 14:41:35, robertphillips wrote:
> Shouldn't this go in the opaque block (at the end)?

Done.

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
File third_party/externals/etc1/etc1.cpp (right):

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
third_party/externals/etc1/etc1.cpp:14: 
On 2014/05/22 14:41:35, robertphillips wrote:
> Add fork comment?

Done.

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
File third_party/externals/etc1/etc1.h (right):

https://codereview.chromium.org/292663011/diff/1/third_party/externals/etc1/e...
third_party/externals/etc1/etc1.h:13: // limitations under the License.
On 2014/05/22 14:41:35, robertphillips wrote:
> Add fork comment ?

Done.

[hal.canary, 2014-05-22 15:44:37]

https://codereview.chromium.org/292663011/diff/30001/third_party/externals/et...
File third_party/externals/etc1/etc1.h (right):

https://codereview.chromium.org/292663011/diff/30001/third_party/externals/et...
third_party/externals/etc1/etc1.h:1: // Copyright 2009 Google Inc.
Should be third_party/etc1 if it's not managed using DEPS

[krajcevski, 2014-05-22 16:14:35]

https://codereview.chromium.org/292663011/diff/30001/third_party/externals/et...
File third_party/externals/etc1/etc1.h (right):

https://codereview.chromium.org/292663011/diff/30001/third_party/externals/et...
third_party/externals/etc1/etc1.h:1: // Copyright 2009 Google Inc.
On 2014/05/22 15:44:37, Hal Canary wrote:
> Should be third_party/etc1 if it's not managed using DEPS

Done.

[hal.canary, 2014-05-22 16:23:23]

Image Decoder LGTM.

[robertphillips, 2014-05-22 17:25:12]

General stuff - lgtm.

[borenet, 2014-05-22 17:26:19]

GYP LGTM

https://codereview.chromium.org/292663011/diff/50001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/292663011/diff/50001/gyp/images.gyp#newcode12
gyp/images.gyp:12: 'etc1.gyp:*',
Not a big deal, but you can specify the target_name here.

[reed1, 2014-05-22 17:46:23]

lgtm

https://codereview.chromium.org/292663011/diff/70001/src/images/SkImageDecode...
File src/images/SkImageDecoder_pkm.cpp (right):

https://codereview.chromium.org/292663011/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_pkm.cpp:34: {
nit: function(params) {

[krajcevski, 2014-05-22 17:52:56]

https://codereview.chromium.org/292663011/diff/50001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/292663011/diff/50001/gyp/images.gyp#newcode12
gyp/images.gyp:12: 'etc1.gyp:*',
On 2014/05/22 17:26:19, borenet wrote:
> Not a big deal, but you can specify the target_name here.

Done.

https://codereview.chromium.org/292663011/diff/70001/src/images/SkImageDecode...
File src/images/SkImageDecoder_pkm.cpp (right):

https://codereview.chromium.org/292663011/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_pkm.cpp:34: {
On 2014/05/22 17:46:24, reed1 wrote:
> nit: function(params) {

Done.

[krajcevski, 2014-05-22 17:53:00]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-05-22 17:53:32]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/krajcevski@google.com/292663011/80001

[krajcevski, 2014-05-22 18:08:59]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-05-22 18:09:38]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/krajcevski@google.com/292663011/100001

[robertphillips, 2014-05-22 18:41:12]

committed manually as r14852 since code review seemed to be having trouble with
the addition of a binary file

[robertphillips, 2014-05-22 21:29:43]

The pkm finally made it in in r14858.