Initial KTX file decoder

Initial KTX file decoder

Committed: https://skia.googlesource.com/skia/+/99ffe24200d8940ceba20f6fbf8c460f994d3cd1

[robertphillips, 2014-06-02 19:56:02]

Will have to look at this more later

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode28
gm/etc1bitmap.cpp:28: SkString str = SkString("etc1bitmap_");
this->fileExtension ?

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode38
gm/etc1bitmap.cpp:38: 
SK_OVERRIDE ?

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode42
gm/etc1bitmap.cpp:42: INHERITED::gResourcePath.c_str(), "mandrill_128.");
this->fileExtension ?

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode64
gm/etc1bitmap.cpp:64: 
// This class specialized ETC1BitmapGM to pkm files. ?

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode77
gm/etc1bitmap.cpp:77: 
// This class specialized ETC1BitmapGM to ktx files. ?

[krajcevski, 2014-06-02 19:57:47]

+ halcanary

[krajcevski, 2014-06-02 20:01:57]

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode28
gm/etc1bitmap.cpp:28: SkString str = SkString("etc1bitmap_");
On 2014/06/02 19:56:03, robertphillips wrote:
> this->fileExtension ?

Done.

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode38
gm/etc1bitmap.cpp:38: 
On 2014/06/02 19:56:03, robertphillips wrote:
> SK_OVERRIDE ?

Done.

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode42
gm/etc1bitmap.cpp:42: INHERITED::gResourcePath.c_str(), "mandrill_128.");
On 2014/06/02 19:56:03, robertphillips wrote:
> this->fileExtension ?

Done.

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode64
gm/etc1bitmap.cpp:64: 
On 2014/06/02 19:56:03, robertphillips wrote:
> // This class specialized ETC1BitmapGM to pkm files. ?

Done.

https://codereview.chromium.org/302333002/diff/50001/gm/etc1bitmap.cpp#newcode77
gm/etc1bitmap.cpp:77: 
On 2014/06/02 19:56:03, robertphillips wrote:
> // This class specialized ETC1BitmapGM to ktx files. ?

Done.

[tfarina, 2014-06-03 03:52:45]

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:19: class SkKTXFile {
could you add a high-level class doc comment? For someone who does not know what
a KTXFile is.

[robertphillips, 2014-06-03 13:27:40]

https://codereview.chromium.org/302333002/diff/70001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/gm/etc1bitmap.cpp#newcode17
gm/etc1bitmap.cpp:17: /**
Update comment ? (either a PKM or KTX file) ?

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

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:23: 
Comment on what KTX is ?

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:53: 
Won't we (in the future) need to support A8 ? Use isRGBA8() here ?

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:108: 
// Uncompressed RGB data ?

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:126: 
// Uncompressed RGBA data ?

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:130: 
RGBA ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp
File src/utils/SkKTXFile.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:11: 
Hmmm - we may need to make this official (in the gyp include paths).

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:18: 
SkEndianSwap32 ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:31: 
I'm not entirely sure this method is working as intended.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:45: // Error of some sort..
Why is "bytesRead > 0" an error?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:59: 
left align '&' ?
We only pass in const values by reference. Any parameters that can be altered
must be passed in via a pointer. This true for both 'buf' and 'bytesLeft'.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:80: bool SkKTXFile::isETC1() const {
this->valid() ?
yoda-fy the '==' check ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:84: bool SkKTXFile::isRGBA8() const {
ditto

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:88: bool SkKTXFile::isRGB8() const {
ditto

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:110: uint32_t magic = this->readInt(buf, bytesLeft);
kKTXEndianessCode ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:131: // additional data types (such as
GL_UNSIGNED_SHORT_5_6_5)
Zero means what in this case?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:135: 
This comment doesn't seem quite right.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:159: if (bytesLeft < fHeader.fBytesOfKeyValueData) {
// file has been truncated somehow ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:208: if (bytesLeft < imgSize) {
// truncated file ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:213: // needs to be taken into account here.
add '{}'s and nesting ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:217: for (int y = 0; y < height; ++y) {
extra space between 'pd' and '(' ?
It looks like we're just taking pointers into 'buf' here. Can this get us into
trouble later (if that memory gets freed)?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:26: {
this->readKTXFile ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:36: SkASSERT(!valid() || mipmap < fPixelData.count());
this->valid ?
missing space before '?' ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:49: private:
// This header captures all the data from the KTX file. ?

Do we really need 'fBytesOfKeyValueData'?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:90: 
KeyValue isn't POD, is it ? Can we actually use SkTDArray here ?

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:100: // bytes if fSwapBytes is set
left justify the '&' ?

[reed1, 2014-06-03 13:40:43]

Can everything KTX be more isolated, as we do for JPEG, PNG?
Does any of this better belong in third_party/?

[hal.canary, 2014-06-03 13:48:00]

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:19: class SkKTXFile {
On 2014/06/03 03:52:46, tfarina wrote:
> could you add a high-level class doc comment? For someone who does not know
what
> a KTXFile is.

// http://www.khronos.org/opengles/sdk/tools/KTX/file_format_spec/

Should be ebough

[krajcevski, 2014-06-03 14:46:27]

>Can everything KTX be more isolated, as we do for JPEG, PNG?
>Does any of this better belong in third_party/?

There exists a libktx provided by Khronos:
http://www.khronos.org/opengles/sdk/tools/KTX/doc/libktx/

Instead of dealing with the associated licensing issues, the format is simple
enough to write a wrapper around ourselves. If there's a better place to put it
let me know. :)

https://codereview.chromium.org/302333002/diff/70001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/gm/etc1bitmap.cpp#newcode17
gm/etc1bitmap.cpp:17: /**
On 2014/06/03 13:27:41, robertphillips wrote:
> Update comment ? (either a PKM or KTX file) ?

Done.

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

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:23: 
On 2014/06/03 13:27:41, robertphillips wrote:
> Comment on what KTX is ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:53: 
On 2014/06/03 13:27:41, robertphillips wrote:
> Won't we (in the future) need to support A8 ? Use isRGBA8() here ?

We will, but I don't think isRGBA8() is the correct check, since we still use an
RGBA config for compressed data (since it decompresses into RGBA).

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:108: 
On 2014/06/03 13:27:41, robertphillips wrote:
> // Uncompressed RGB data ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:126: 
On 2014/06/03 13:27:41, robertphillips wrote:
> // Uncompressed RGBA data ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:130: 
On 2014/06/03 13:27:41, robertphillips wrote:
> RGBA ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp
File src/utils/SkKTXFile.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:18: 
On 2014/06/03 13:27:41, robertphillips wrote:
> SkEndianSwap32 ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:31: 
On 2014/06/03 13:27:41, robertphillips wrote:
> I'm not entirely sure this method is working as intended.

None of the tools that I found produce .ktx files with key/value data. I have
nothing to test it with! That being said, I relooked at the implementation and I
think I fixed a bunch of the bugs associated with it.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:45: // Error of some sort..
On 2014/06/03 13:27:41, robertphillips wrote:
> Why is "bytesRead > 0" an error?

It's not.

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:59: 
On 2014/06/03 13:27:41, robertphillips wrote:
> left align '&' ?
> We only pass in const values by reference. Any parameters that can be altered
> must be passed in via a pointer. This true for both 'buf' and 'bytesLeft'.

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:80: bool SkKTXFile::isETC1() const {
On 2014/06/03 13:27:41, robertphillips wrote:
> this->valid() ?
> yoda-fy the '==' check ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:84: bool SkKTXFile::isRGBA8() const {
On 2014/06/03 13:27:41, robertphillips wrote:
> ditto

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:88: bool SkKTXFile::isRGB8() const {
On 2014/06/03 13:27:41, robertphillips wrote:
> ditto

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:110: uint32_t magic = this->readInt(buf, bytesLeft);
On 2014/06/03 13:27:41, robertphillips wrote:
> kKTXEndianessCode ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:131: // additional data types (such as
GL_UNSIGNED_SHORT_5_6_5)
On 2014/06/03 13:27:41, robertphillips wrote:
> Zero means what in this case?

Done. Fixed the comment.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:135: 
On 2014/06/03 13:27:41, robertphillips wrote:
> This comment doesn't seem quite right.

According to the KTX spec, if the GLType is 0 (compressed) or UNSIGNED_BYTE,
then the type size must be one (since the data is represented on a byte-by-byte
basis).

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:159: if (bytesLeft < fHeader.fBytesOfKeyValueData) {
On 2014/06/03 13:27:41, robertphillips wrote:
> // file has been truncated somehow ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:208: if (bytesLeft < imgSize) {
On 2014/06/03 13:27:41, robertphillips wrote:
> // truncated file ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:213: // needs to be taken into account here.
On 2014/06/03 13:27:41, robertphillips wrote:
> add '{}'s and nesting ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:217: for (int y = 0; y < height; ++y) {
On 2014/06/03 13:27:41, robertphillips wrote:
> extra space between 'pd' and '(' ?
> It looks like we're just taking pointers into 'buf' here. Can this get us into
> trouble later (if that memory gets freed)?

Yes, there is a comment about it above the SkKTXFile constructor. Do you think
we should make copies of this data? It would incur unnecessary overhead when we
need to do things like upload the data as a texture.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:19: class SkKTXFile {
On 2014/06/03 13:48:00, Hal Canary wrote:
> On 2014/06/03 03:52:46, tfarina wrote:
> > could you add a high-level class doc comment? For someone who does not know
> what
> > a KTXFile is.
> 
> // http://www.khronos.org/opengles/sdk/tools/KTX/file_format_spec/
> 
> Should be ebough

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:26: {
On 2014/06/03 13:27:41, robertphillips wrote:
> this->readKTXFile ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:36: SkASSERT(!valid() || mipmap < fPixelData.count());
On 2014/06/03 13:27:41, robertphillips wrote:
> this->valid ?
> missing space before '?' ?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:49: private:
On 2014/06/03 13:27:41, robertphillips wrote:
> // This header captures all the data from the KTX file. ?
> 
> Do we really need 'fBytesOfKeyValueData'?

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:100: // bytes if fSwapBytes is set
On 2014/06/03 13:27:41, robertphillips wrote:
> left justify the '&' ?

Done.

[krajcevski, 2014-06-03 14:58:35]

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp
File src/utils/SkKTXFile.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.cpp#...
src/utils/SkKTXFile.cpp:11: 
On 2014/06/03 13:27:41, robertphillips wrote:
> Hmmm - we may need to make this official (in the gyp include paths).

Done.

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/70001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:90: 
On 2014/06/03 13:27:41, robertphillips wrote:
> KeyValue isn't POD, is it ? Can we actually use SkTDArray here ?

Done.

[robertphillips, 2014-06-03 15:28:01]

lgtm + some nits.

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp
File src/utils/SkKTXFile.cpp (right):

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp...
src/utils/SkKTXFile.cpp:19: 
// magic code used by the KTX format to check for endianess ?
kKTX_... ?

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp...
src/utils/SkKTXFile.cpp:25: 
Much clearer.

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp...
src/utils/SkKTXFile.cpp:56: 
"if (" -- add space ?

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.h#n...
src/utils/SkKTXFile.h:76: Header() { memset(this, 0, sizeof(*this)); }
Is swapBytes actually implemented or used anywhere?

[krajcevski, 2014-06-03 15:41:46]

I've moved the ktx files over to third_party to match the ETC1 files.

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp
File src/utils/SkKTXFile.cpp (right):

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp...
src/utils/SkKTXFile.cpp:19: 
On 2014/06/03 15:28:02, robertphillips wrote:
> // magic code used by the KTX format to check for endianess ?
> kKTX_... ?

Done.

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.cpp...
src/utils/SkKTXFile.cpp:56: 
On 2014/06/03 15:28:02, robertphillips wrote:
> "if (" -- add space ?

Done.

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/110001/src/utils/SkKTXFile.h#n...
src/utils/SkKTXFile.h:76: Header() { memset(this, 0, sizeof(*this)); }
On 2014/06/03 15:28:02, robertphillips wrote:
> Is swapBytes actually implemented or used anywhere?

It's not.. this is left over from before.

Done.

[hal.canary, 2014-06-03 15:41:58]

https://codereview.chromium.org/302333002/diff/70001/include/core/SkImageDeco...
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/302333002/diff/70001/include/core/SkImageDeco...
include/core/SkImageDecoder.h:43: kLastKnownFormat = kWEBP_Format,
kLastKnownFormat = kkKTX_Format

https://codereview.chromium.org/302333002/diff/70001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/src/gpu/SkGr.cpp#newcode139
src/gpu/SkGr.cpp:139: SkData *data = bm.pixelRef()->refEncodedData();
As I mentioned in person, this will leak.  From SkPixelRef.h: "If non-null is
returned, the caller is responsible for calling unref() on the data when it is
finished."

https://codereview.chromium.org/302333002/diff/90001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/90001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:32: // access the pixels.
Alternatively, you could take and ref an SkData here.  Will the normal use case
always have a SkData availible?

[krajcevski, 2014-06-03 18:56:28]

https://codereview.chromium.org/302333002/diff/70001/include/core/SkImageDeco...
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/302333002/diff/70001/include/core/SkImageDeco...
include/core/SkImageDecoder.h:43: kLastKnownFormat = kWEBP_Format,
On 2014/06/03 15:41:58, Hal Canary wrote:
> kLastKnownFormat = kkKTX_Format

Done.

https://codereview.chromium.org/302333002/diff/70001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/302333002/diff/70001/src/gpu/SkGr.cpp#newcode139
src/gpu/SkGr.cpp:139: SkData *data = bm.pixelRef()->refEncodedData();
On 2014/06/03 15:41:58, Hal Canary wrote:
> As I mentioned in person, this will leak.  From SkPixelRef.h: "If non-null is
> returned, the caller is responsible for calling unref() on the data when it is
> finished."

Done.

https://codereview.chromium.org/302333002/diff/90001/src/utils/SkKTXFile.h
File src/utils/SkKTXFile.h (right):

https://codereview.chromium.org/302333002/diff/90001/src/utils/SkKTXFile.h#ne...
src/utils/SkKTXFile.h:32: // access the pixels.
On 2014/06/03 15:41:58, Hal Canary wrote:
> Alternatively, you could take and ref an SkData here.  Will the normal use
case
> always have a SkData availible?

Hrm, this is a good suggestion. It gets rid of my worries that the underlying
data will get freed while this object still lives.

Done.

[hal.canary, 2014-06-03 19:22:48]

lgtm

https://codereview.chromium.org/302333002/diff/150001/src/images/SkImageDecod...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/302333002/diff/150001/src/images/SkImageDecod...
src/images/SkImageDecoder_ktx.cpp:50: SkAutoDataUnref
data(CopyStreamToData(stream));
// TODO: We should implement SkStream::copyToData() so that this is cheap for
memory and file streams.

https://codereview.chromium.org/302333002/diff/150001/third_party/ktx/ktx.h
File third_party/ktx/ktx.h (right):

https://codereview.chromium.org/302333002/diff/150001/third_party/ktx/ktx.h#n...
third_party/ktx/ktx.h:111: bool readKTXFile(const uint8_t *data, size_t
dataLen);
bikeshed:  If this function is only called once, why not move it into the
constructor?

[krajcevski, 2014-06-03 19:46:52]

https://codereview.chromium.org/302333002/diff/150001/src/images/SkImageDecod...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/302333002/diff/150001/src/images/SkImageDecod...
src/images/SkImageDecoder_ktx.cpp:50: SkAutoDataUnref
data(CopyStreamToData(stream));
On 2014/06/03 19:22:48, Hal Canary wrote:
> // TODO: We should implement SkStream::copyToData() so that this is cheap for
> memory and file streams.

Done.

https://codereview.chromium.org/302333002/diff/150001/third_party/ktx/ktx.h
File third_party/ktx/ktx.h (right):

https://codereview.chromium.org/302333002/diff/150001/third_party/ktx/ktx.h#n...
third_party/ktx/ktx.h:111: bool readKTXFile(const uint8_t *data, size_t
dataLen);
On 2014/06/03 19:22:48, Hal Canary wrote:
> bikeshed:  If this function is only called once, why not move it into the
> constructor?

In general, I believe constructors are clearer when they're short (code-wise). I
like the conciseness of having the meat hidden away behind a function that's
more descriptive of what it does. That being said, this function needs a few
comments.

[bsalomon, 2014-06-03 19:47:59]

api lgtm

[krajcevski, 2014-06-03 19:48:15]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-06-03 19:48:32]

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

[commit-bot: I haz the power, 2014-06-03 20:04:48]

Change committed as 99ffe24200d8940ceba20f6fbf8c460f994d3cd1