Texture compression unit tests along with a couple of bug fixes

Texture compression unit tests along with a couple of bug fixes

Committed: https://skia.googlesource.com/skia/+/2b310e46eb28bbbe375369e47068accbdfeff3a7

[robertphillips, 2014-06-11 18:16:36]

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
File tests/TextureCompressionTest.cpp (right):

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:22: SkBitmap bitmap;
kWidth & kHeight ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:24: static const int height = 63;
MakeA8? kOpaque ??

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:30: 
Make the next two lines one line ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:34: 
1 line?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:40: const SkTextureCompressor::Format
kLATCFormat = SkTextureCompressor::kLATC_Format;
What makes this bitmap white?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:49: SkBitmap bitmap;
kWidth, kHeight?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:51: static const int height = 8;
Same as above ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:55: 
1 line ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:59: 
1 line ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:63: 
I would just ditch this test here and check it every time you grab the pixels in
the loop.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:72: for (int lum = 0; lum < 256; ++lum) {
regrab the pixels in here ?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:83: 
Move kSizeToBe out of the loop?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:87: // Make sure that it all matches a given
block encoding.
Rename 'encoding' 'encodedConstColorBlock' - or something?
Add a comment about why this is the encoding for a constant color block?

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:89: const uint64_t* blockPtr =
reinterpret_cast<const uint64_t*>(latcData->data());
++i ?

[krajcevski, 2014-06-11 18:51:11]

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
File tests/TextureCompressionTest.cpp (right):

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:22: SkBitmap bitmap;
On 2014/06/11 18:16:35, robertphillips wrote:
> kWidth & kHeight ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:24: static const int height = 63;
On 2014/06/11 18:16:35, robertphillips wrote:
> MakeA8? kOpaque ??

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:30: 
On 2014/06/11 18:16:36, robertphillips wrote:
> Make the next two lines one line ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:34: 
On 2014/06/11 18:16:35, robertphillips wrote:
> 1 line?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:40: const SkTextureCompressor::Format
kLATCFormat = SkTextureCompressor::kLATC_Format;
On 2014/06/11 18:16:35, robertphillips wrote:
> What makes this bitmap white?

Artifact of copypasta. Fixed.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:49: SkBitmap bitmap;
On 2014/06/11 18:16:35, robertphillips wrote:
> kWidth, kHeight?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:51: static const int height = 8;
On 2014/06/11 18:16:35, robertphillips wrote:
> Same as above ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:55: 
On 2014/06/11 18:16:36, robertphillips wrote:
> 1 line ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:59: 
On 2014/06/11 18:16:36, robertphillips wrote:
> 1 line ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:63: 
On 2014/06/11 18:16:35, robertphillips wrote:
> I would just ditch this test here and check it every time you grab the pixels
in
> the loop.

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:72: for (int lum = 0; lum < 256; ++lum) {
On 2014/06/11 18:16:36, robertphillips wrote:
> regrab the pixels in here ?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:83: 
On 2014/06/11 18:16:36, robertphillips wrote:
> Move kSizeToBe out of the loop?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:87: // Make sure that it all matches a given
block encoding.
On 2014/06/11 18:16:35, robertphillips wrote:
> Rename 'encoding' 'encodedConstColorBlock' - or something?
> Add a comment about why this is the encoding for a constant color block?

Done.

https://codereview.chromium.org/330453005/diff/1/tests/TextureCompressionTest...
tests/TextureCompressionTest.cpp:89: const uint64_t* blockPtr =
reinterpret_cast<const uint64_t*>(latcData->data());
On 2014/06/11 18:16:36, robertphillips wrote:
> ++i ?

Done.

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

lgtm + a comment suggestion

https://codereview.chromium.org/330453005/diff/20001/tests/TextureCompression...
File tests/TextureCompressionTest.cpp (right):

https://codereview.chromium.org/330453005/diff/20001/tests/TextureCompression...
tests/TextureCompressionTest.cpp:47: static const int kHeight = 64;
// LATC only works for A8 bitmaps ?

[krajcevski, 2014-06-11 19:11:17]

https://codereview.chromium.org/330453005/diff/20001/tests/TextureCompression...
File tests/TextureCompressionTest.cpp (right):

https://codereview.chromium.org/330453005/diff/20001/tests/TextureCompression...
tests/TextureCompressionTest.cpp:47: static const int kHeight = 64;
On 2014/06/11 19:02:56, robertphillips wrote:
> // LATC only works for A8 bitmaps ?

Done.

[krajcevski, 2014-06-11 19:11:20]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-06-11 19:12:12]

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

[commit-bot: I haz the power, 2014-06-11 19:26:53]

Change committed as 2b310e46eb28bbbe375369e47068accbdfeff3a7