Pass compressed blitters to our mask drawing algorithm

Pass compressed blitters to our mask drawing algorithm

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

[krajcevski, 2014-08-06 19:43:46]

+Mike

[krajcevski, 2014-08-06 20:45:11]

Updated the scope of the blitter allocation, and added support for an
SkTBlitterAllocator to be passed to the compressed texture blitter factory.

[robertphillips, 2014-08-06 20:54:49]

For PS #1 :(

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:153: 
Get reset -> Get rid of ?

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:211: 
If blitter creation failed in the above couldn't we wind up with munged cmpWidth
& cmpHeight values here?

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:312: this->sendTextureData(texture, desc,
fCompressedBuffer, 0);
break; ? for consistency.

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h#new...
src/gpu/GrSWMaskHelper.h:48: , fCompressedBlitter(NULL)
'{' goes on line above

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h#new...
src/gpu/GrSWMaskHelper.h:118: 
// These two are only non-NULL is fCompressionMode is not kNone ?

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

https://codereview.chromium.org/446103002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor_LATC.cpp:438: // and they will only be non-zero
(0xFF) if the index is 7. So bzero will do just fine.
// (8 bytes/block) * (w * h / 4*4) blocks = w * h / 2 bytes ?

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

https://codereview.chromium.org/446103002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor_R11EAC.cpp:616: // Memset the output buffer to an
encoding that decodes to zero...
ASTC -> R11 ?

[reed1, 2014-08-06 20:56:03]

wow, much cleaner looking (and no need for a persistent blitter member variable.

https://codereview.chromium.org/446103002/diff/20001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/20001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:89: if (NULL != fCompressedBuffer) {
1. can just call sk_free() w/o checking
2. don't need to NULL the field (in a destructor)
3. might as well inline this in the header

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

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:153: 
On 2014/08/06 20:54:49, robertphillips wrote:
> Get reset -> Get rid of ?

Done.

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:211: 
On 2014/08/06 20:54:48, robertphillips wrote:
> If blitter creation failed in the above couldn't we wind up with munged
cmpWidth
> & cmpHeight values here?

Yes, but the munged ones only happen if we have a compression method. If we
can't make a blitter we can at least compress the mask that we create (which
still needs munged dimensions).

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.cpp#n...
src/gpu/GrSWMaskHelper.cpp:312: this->sendTextureData(texture, desc,
fCompressedBuffer, 0);
On 2014/08/06 20:54:48, robertphillips wrote:
> break; ? for consistency.

Done.

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h#new...
src/gpu/GrSWMaskHelper.h:48: , fCompressedBlitter(NULL)
On 2014/08/06 20:54:49, robertphillips wrote:
> '{' goes on line above

Done.

https://codereview.chromium.org/446103002/diff/1/src/gpu/GrSWMaskHelper.h#new...
src/gpu/GrSWMaskHelper.h:118: 
On 2014/08/06 20:54:49, robertphillips wrote:
> // These two are only non-NULL is fCompressionMode is not kNone ?

Done.

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

https://codereview.chromium.org/446103002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor_LATC.cpp:438: // and they will only be non-zero
(0xFF) if the index is 7. So bzero will do just fine.
On 2014/08/06 20:54:49, robertphillips wrote:
> // (8 bytes/block) * (w * h / 4*4) blocks = w * h / 2 bytes ?

Done.

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

https://codereview.chromium.org/446103002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor_R11EAC.cpp:616: // Memset the output buffer to an
encoding that decodes to zero...
On 2014/08/06 20:54:49, robertphillips wrote:
> ASTC -> R11 ?

Done.

https://codereview.chromium.org/446103002/diff/20001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/20001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:89: if (NULL != fCompressedBuffer) {
On 2014/08/06 20:56:03, reed1 wrote:
> 1. can just call sk_free() w/o checking
> 2. don't need to NULL the field (in a destructor)
> 3. might as well inline this in the header

Done.

[reed1, 2014-08-07 13:29:47]

lgtm

[robertphillips, 2014-08-07 13:55:06]

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:164: 
Update comment ?

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:180: SkASSERT(cmpSz > 0);
SkASSERT(NULL == fCompressedBuffer); ?

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.h...
src/gpu/GrSWMaskHelper.h:122: // only allocated (non-null) if fCompressionMode
is kBlitter_CompressionMode
Can we use an SkAutoMalloc for this?

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
File src/utils/SkTextureCompressor_ASTC.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
src/utils/SkTextureCompressor_ASTC.cpp:2019: 
Why do we need to zero out here?

[krajcevski, 2014-08-07 14:48:12]

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:164: 
On 2014/08/07 13:55:06, robertphillips wrote:
> Update comment ?

Done.

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.c...
src/gpu/GrSWMaskHelper.cpp:180: SkASSERT(cmpSz > 0);
On 2014/08/07 13:55:06, robertphillips wrote:
> SkASSERT(NULL == fCompressedBuffer); ?

Done.

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/446103002/diff/60001/src/gpu/GrSWMaskHelper.h...
src/gpu/GrSWMaskHelper.h:122: // only allocated (non-null) if fCompressionMode
is kBlitter_CompressionMode
On 2014/08/07 13:55:06, robertphillips wrote:
> Can we use an SkAutoMalloc for this?

Done.

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
File src/utils/SkTextureCompressor_ASTC.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
src/utils/SkTextureCompressor_ASTC.cpp:2019: 
On 2014/08/07 13:55:06, robertphillips wrote:
> Why do we need to zero out here?

The blitter isn't required to write every row, and sometimes it skips entire row
blocks. If they're initialized with garbage then the resulting texture has
artifacts.

[robertphillips, 2014-08-07 14:52:24]

lgtm + a comment request & a nit

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
File src/utils/SkTextureCompressor_ASTC.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
src/utils/SkTextureCompressor_ASTC.cpp:2019: 
Can we add a comment re that at each of the three zeroing points?

https://codereview.chromium.org/446103002/diff/80001/src/gpu/GrSWMaskHelper.h
File src/gpu/GrSWMaskHelper.h (right):

https://codereview.chromium.org/446103002/diff/80001/src/gpu/GrSWMaskHelper.h...
src/gpu/GrSWMaskHelper.h:46: , fCompressionMode(kNone_CompressionMode)
Don't need this now.

[krajcevski, 2014-08-07 15:00:48]

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
File src/utils/SkTextureCompressor_ASTC.cpp (right):

https://codereview.chromium.org/446103002/diff/60001/src/utils/SkTextureCompr...
src/utils/SkTextureCompressor_ASTC.cpp:2019: 
On 2014/08/07 14:52:23, robertphillips wrote:
> Can we add a comment re that at each of the three zeroing points?

Done.

[krajcevski, 2014-08-07 15:00:54]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-08-07 15:01:41]

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

[krajcevski, 2014-08-07 15:07:39]

The CQ bit was checked by krajcevski@google.com

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

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

[commit-bot: I haz the power, 2014-08-07 15:15:20]

Change committed as b8ccc2f6d258a8466f79fc418e9e0a55aeaf58ce