- Add astcbitmap to gm slides

Add astcbitmap to gm slides.
Add additional ASTC formats.
Add astc image decoder files.

Committed: https://skia.googlesource.com/skia/+/95b1b3d82d227141647777d83324aa570b530096

[krajcevski, 2014-08-06 19:14:36]

This patch is for an image decoder that reads .astc files. These files are
produced by the Mali texture compression tool, (similar to PKM for ETC1). They
contain a very small header to determine the internal contents of the ASTC
image.

[reed1, 2014-08-06 19:45:40]

api lgtm

[robertphillips, 2014-08-06 21:13:16]

lgtm + nits & suggestions

https://codereview.chromium.org/444093002/diff/1/gm/astcbitmap.cpp
File gm/astcbitmap.cpp (right):

https://codereview.chromium.org/444093002/diff/1/gm/astcbitmap.cpp#newcode91
gm/astcbitmap.cpp:91: private:
The INHERITED goes last.

https://codereview.chromium.org/444093002/diff/1/src/images/SkImageDecoder_as...
File src/images/SkImageDecoder_astc.cpp (right):

https://codereview.chromium.org/444093002/diff/1/src/images/SkImageDecoder_as...
src/images/SkImageDecoder_astc.cpp:51: 
Since the magic number is used twice can we define it somewhere ?

https://codereview.chromium.org/444093002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (right):

https://codereview.chromium.org/444093002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:235: 
Can we not have all these fall through and call GetBlockDimensions ?

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

https://codereview.chromium.org/444093002/diff/1/tests/ImageDecodingTest.cpp#...
tests/ImageDecodingTest.cpp:57: case SkImageDecoder::kBMP_Format:
is -> are ?
format -> formats ?

[krajcevski, 2014-08-06 22:31:00]

https://codereview.chromium.org/444093002/diff/1/gm/astcbitmap.cpp
File gm/astcbitmap.cpp (right):

https://codereview.chromium.org/444093002/diff/1/gm/astcbitmap.cpp#newcode91
gm/astcbitmap.cpp:91: private:
On 2014/08/06 21:13:15, robertphillips wrote:
> The INHERITED goes last.

Done.

https://codereview.chromium.org/444093002/diff/1/src/images/SkImageDecoder_as...
File src/images/SkImageDecoder_astc.cpp (right):

https://codereview.chromium.org/444093002/diff/1/src/images/SkImageDecoder_as...
src/images/SkImageDecoder_astc.cpp:51: 
On 2014/08/06 21:13:15, robertphillips wrote:
> Since the magic number is used twice can we define it somewhere ?

Done.

https://codereview.chromium.org/444093002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (right):

https://codereview.chromium.org/444093002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:235: 
On 2014/08/06 21:13:15, robertphillips wrote:
> Can we not have all these fall through and call GetBlockDimensions ?

Can't believe I didn't think of that.

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

https://codereview.chromium.org/444093002/diff/1/tests/ImageDecodingTest.cpp#...
tests/ImageDecodingTest.cpp:57: case SkImageDecoder::kBMP_Format:
On 2014/08/06 21:13:15, robertphillips wrote:
> is -> are ?
> format -> formats ?

Done.

[krajcevski, 2014-08-07 17:29:49]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-08-07 17:30:42]

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

[commit-bot: I haz the power, 2014-08-07 17:38:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-08-07 17:42:16]

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

[commit-bot: I haz the power, 2014-08-07 17:42:17]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[krajcevski, 2014-08-07 17:47:37]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-08-07 17:48:38]

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

[commit-bot: I haz the power, 2014-08-07 17:57:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-08-07 18:00:33]

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

[commit-bot: I haz the power, 2014-08-07 18:00:38]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[krajcevski, 2014-08-07 18:08:23]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-08-07 18:08:54]

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

[commit-bot: I haz the power, 2014-08-07 18:18:26]

Change committed as 2fc05823fed4b9649338f3029cd1ba05ef49a02f

[krajcevski, 2014-08-07 18:31:26]

A revert of this CL has been created in
https://codereview.chromium.org/447343002/ by krajcevski@google.com.

The reason for reverting is: Breaking DM.

[krajcevski, 2014-08-07 19:49:03]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-08-07 19:49:22]

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

[commit-bot: I haz the power, 2014-08-07 19:58:46]

Change committed as 95b1b3d82d227141647777d83324aa570b530096

[scroggo, 2014-10-17 15:31:37]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2014-10-17 15:31:38]

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
File src/images/SkImageDecoder_astc.cpp (right):

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
src/images/SkImageDecoder_astc.cpp:162: if
(!SkTextureCompressor::DecompressBufferFromFormat(
Due to the way this statement was written to get around length limits, it almost
looks logical, but looking closer, it says:

if (!f()) {
    // do nothing different from if f() returned true
}

What is the intent here? Do we want to behave differently if f() returned false?
Return false from onDecode? Or should we remove the if?

[robertphillips, 2014-10-17 15:41:22]

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
File src/images/SkImageDecoder_astc.cpp (right):

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
src/images/SkImageDecoder_astc.cpp:162: if
(!SkTextureCompressor::DecompressBufferFromFormat(
Ha ha. Given the semantics and the other uses I believe this is missing a
"return false;"

[pavelk, 2014-10-19 20:09:41]

pavel@cs.unc.edu changed reviewers:
+ pavel@cs.unc.edu

[pavelk, 2014-10-19 20:09:42]

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
File src/images/SkImageDecoder_astc.cpp (right):

https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
src/images/SkImageDecoder_astc.cpp:162: if
(!SkTextureCompressor::DecompressBufferFromFormat(
On 2014/10/17 15:41:22, robertphillips wrote:
> Ha ha. Given the semantics and the other uses I believe this is missing a
> "return false;"

Yes, it most certainly is!

[scroggo, 2014-10-20 15:36:28]

On 2014/10/19 20:09:42, pavelk wrote:
>
https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
> File src/images/SkImageDecoder_astc.cpp (right):
> 
>
https://codereview.chromium.org/444093002/diff/100001/src/images/SkImageDecod...
> src/images/SkImageDecoder_astc.cpp:162: if
> (!SkTextureCompressor::DecompressBufferFromFormat(
> On 2014/10/17 15:41:22, robertphillips wrote:
> > Ha ha. Given the semantics and the other uses I believe this is missing a
> > "return false;"
> 
> Yes, it most certainly is!

For posterity, I created http://code.google.com/p/skia/issues/detail?id=3037 to
track the bug, and have a fix (as part of a larger change) in
https://codereview.chromium.org/647023006