Initial KTX encoder

Initial KTX encoder

The encoder comes with tests to check that the encoding/decoding
operations between ETC encoded bitmaps and ARGB bitmaps are sane.

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

[robertphillips, 2014-06-05 19:12:22]

lgtm + nits

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:141: bool setRequireUnpremul = false;
Can we just do "SkString premulKey("KTXPremultipliedAlpha");" ?
Can we make it const?

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:175: 
// ??

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:184: 
Can we put onEncode before encodePKM in the file?

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:210: if (etc1_pkm_is_valid(bytes)) {
this-> ?

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp
File tests/KtxTest.cpp (right):

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode42
tests/KtxTest.cpp:42: uint8_t *row = pixels;
i & j are okay but I usually use x & y.

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode111
tests/KtxTest.cpp:111: 
_s -> memStream ? Do we even need _s ?

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp
File third_party/ktx/ktx.cpp (right):

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:84: size_t bytesWrittenPadFour = (bytesWritten + 3) &
~3;
I think we still can't use the "{ 0 }" notation.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:114: 
const SkString& key ?

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:295: 
_key -> cstrKey; _value -> cstrValue ?

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h
File third_party/ktx/ktx.h (right):

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcode56
third_party/ktx/ktx.h:56: // is returned.
const SkString& key ?

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
third_party/ktx/ktx.h:101: size_t size() const { return fDataSz; }
return "const SkString &" ?

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
third_party/ktx/ktx.h:105: private:
Line up the member variable names ?

[krajcevski, 2014-06-05 19:29:45]

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:141: bool setRequireUnpremul = false;
On 2014/06/05 19:12:19, robertphillips wrote:
> Can we just do "SkString premulKey("KTXPremultipliedAlpha");" ?
> Can we make it const?

Done.

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:175: 
On 2014/06/05 19:12:19, robertphillips wrote:
> // ??

Done.

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:184: 
On 2014/06/05 19:12:20, robertphillips wrote:
> Can we put onEncode before encodePKM in the file?

Done.

https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:210: if (etc1_pkm_is_valid(bytes)) {
On 2014/06/05 19:12:20, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp
File tests/KtxTest.cpp (right):

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode42
tests/KtxTest.cpp:42: uint8_t *row = pixels;
On 2014/06/05 19:12:20, robertphillips wrote:
> i & j are okay but I usually use x & y.

Done.

https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode111
tests/KtxTest.cpp:111: 
On 2014/06/05 19:12:20, robertphillips wrote:
> _s -> memStream ? Do we even need _s ?

No, this is my (poor) attempt at trying to manage line length.

Fixed.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp
File third_party/ktx/ktx.cpp (right):

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:84: size_t bytesWrittenPadFour = (bytesWritten + 3) &
~3;
On 2014/06/05 19:12:21, robertphillips wrote:
> I think we still can't use the "{ 0 }" notation.

Done.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:114: 
On 2014/06/05 19:12:20, robertphillips wrote:
> const SkString& key ?

Done.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
third_party/ktx/ktx.cpp:295: 
On 2014/06/05 19:12:20, robertphillips wrote:
> _key -> cstrKey; _value -> cstrValue ?

Done.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h
File third_party/ktx/ktx.h (right):

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcode56
third_party/ktx/ktx.h:56: // is returned.
On 2014/06/05 19:12:21, robertphillips wrote:
> const SkString& key ?

Done.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
third_party/ktx/ktx.h:101: size_t size() const { return fDataSz; }
On 2014/06/05 19:12:21, robertphillips wrote:
> return "const SkString &" ?

Done.

https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
third_party/ktx/ktx.h:105: private:
On 2014/06/05 19:12:21, robertphillips wrote:
> Line up the member variable names ?

Done.

[bsalomon, 2014-06-05 20:36:21]

On 2014/06/05 19:29:45, krajcevski wrote:
>
https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
> File src/images/SkImageDecoder_ktx.cpp (right):
> 
>
https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
> src/images/SkImageDecoder_ktx.cpp:141: bool setRequireUnpremul = false;
> On 2014/06/05 19:12:19, robertphillips wrote:
> > Can we just do "SkString premulKey("KTXPremultipliedAlpha");" ?
> > Can we make it const?
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
> src/images/SkImageDecoder_ktx.cpp:175: 
> On 2014/06/05 19:12:19, robertphillips wrote:
> > // ??
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
> src/images/SkImageDecoder_ktx.cpp:184: 
> On 2014/06/05 19:12:20, robertphillips wrote:
> > Can we put onEncode before encodePKM in the file?
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/src/images/SkImageDecoder_kt...
> src/images/SkImageDecoder_ktx.cpp:210: if (etc1_pkm_is_valid(bytes)) {
> On 2014/06/05 19:12:20, robertphillips wrote:
> > this-> ?
> 
> Done.
> 
> https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp
> File tests/KtxTest.cpp (right):
> 
> https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode42
> tests/KtxTest.cpp:42: uint8_t *row = pixels;
> On 2014/06/05 19:12:20, robertphillips wrote:
> > i & j are okay but I usually use x & y.
> 
> Done.
> 
> https://codereview.chromium.org/312353003/diff/1/tests/KtxTest.cpp#newcode111
> tests/KtxTest.cpp:111: 
> On 2014/06/05 19:12:20, robertphillips wrote:
> > _s -> memStream ? Do we even need _s ?
> 
> No, this is my (poor) attempt at trying to manage line length.
> 
> Fixed.
> 
> https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp
> File third_party/ktx/ktx.cpp (right):
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
> third_party/ktx/ktx.cpp:84: size_t bytesWrittenPadFour = (bytesWritten + 3) &
> ~3;
> On 2014/06/05 19:12:21, robertphillips wrote:
> > I think we still can't use the "{ 0 }" notation.
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
> third_party/ktx/ktx.cpp:114: 
> On 2014/06/05 19:12:20, robertphillips wrote:
> > const SkString& key ?
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.cpp#newc...
> third_party/ktx/ktx.cpp:295: 
> On 2014/06/05 19:12:20, robertphillips wrote:
> > _key -> cstrKey; _value -> cstrValue ?
> 
> Done.
> 
> https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h
> File third_party/ktx/ktx.h (right):
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcode56
> third_party/ktx/ktx.h:56: // is returned.
> On 2014/06/05 19:12:21, robertphillips wrote:
> > const SkString& key ?
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
> third_party/ktx/ktx.h:101: size_t size() const { return fDataSz; }
> On 2014/06/05 19:12:21, robertphillips wrote:
> > return "const SkString &" ?
> 
> Done.
> 
>
https://codereview.chromium.org/312353003/diff/1/third_party/ktx/ktx.h#newcod...
> third_party/ktx/ktx.h:105: private:
> On 2014/06/05 19:12:21, robertphillips wrote:
> > Line up the member variable names ?
> 
> Done.

api lgtm

[krajcevski, 2014-06-05 20:40:01]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-06-05 20:41:04]

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

[commit-bot: I haz the power, 2014-06-06 13:16:32]

Change committed as c250d2e4abdbe8193357696518592af8a0b4555a

[scroggo, 2014-06-09 14:49:45]

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

https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:144:
this->setRequireUnpremultipliedColors(true);
This is intended as a setting for the client. If the client actually wanted
unpremultiplied colors, this will incorrectly give them premultiplied colors.

https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
src/images/SkImageDecoder_ktx.cpp:165:
this->setRequireUnpremultipliedColors(false);
Again, if the client wanted unpremultiplied colors, this would change the
behavior for followup calls.

(FWIW, our main client of SkImageDecoder, Android, does not reuse a decoder.)

[krajcevski, 2014-06-09 17:23:45]

On 2014/06/09 14:49:45, scroggo wrote:
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> File src/images/SkImageDecoder_ktx.cpp (right):
> 
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> src/images/SkImageDecoder_ktx.cpp:144:
> this->setRequireUnpremultipliedColors(true);
> This is intended as a setting for the client. If the client actually wanted
> unpremultiplied colors, this will incorrectly give them premultiplied colors.
> 
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> src/images/SkImageDecoder_ktx.cpp:165:
> this->setRequireUnpremultipliedColors(false);
> Again, if the client wanted unpremultiplied colors, this would change the
> behavior for followup calls.
> 
> (FWIW, our main client of SkImageDecoder, Android, does not reuse a decoder.)

What is the intended behavior if a bitmap contains premultiplied alpha on disk,
but the client asks for unpremultiplied alpha. Does the sampler need an
additional setting that unpremultiplies the color components?

[scroggo, 2014-06-09 18:01:15]

On 2014/06/09 17:23:45, krajcevski wrote:
> On 2014/06/09 14:49:45, scroggo wrote:
> >
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> > File src/images/SkImageDecoder_ktx.cpp (right):
> > 
> >
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> > src/images/SkImageDecoder_ktx.cpp:144:
> > this->setRequireUnpremultipliedColors(true);
> > This is intended as a setting for the client. If the client actually wanted
> > unpremultiplied colors, this will incorrectly give them premultiplied
colors.
> > 
> >
>
https://codereview.chromium.org/312353003/diff/20001/src/images/SkImageDecode...
> > src/images/SkImageDecoder_ktx.cpp:165:
> > this->setRequireUnpremultipliedColors(false);
> > Again, if the client wanted unpremultiplied colors, this would change the
> > behavior for followup calls.
> > 
> > (FWIW, our main client of SkImageDecoder, Android, does not reuse a
decoder.)
> 
> What is the intended behavior if a bitmap contains premultiplied alpha on
disk,
> but the client asks for unpremultiplied alpha.

Get unpremultiplied data (SkImageDecoder_CG.cpp does this) or NULL (in some
cases we return NULL
if we cannot/choose not to honor the request).

> Does the sampler need an
> additional setting that unpremultiplies the color components?

That would make sense. Currently it just looks the SkImageDecoder, since we
previously
did not have a case where the encoded data was already premultiplied. I do think
it
makes sense to use the same path inside the sampler, but we just need to make
sure
we don't interfere with the intent of setRequireUnpremultipliedColors.