Move ETC1 and LATC enums value to GrPixelConfig

Move the LATC and ETC1 enum values to GrPixelConfig. I also tried to put in checks in a few places to make sure that we weren't using these pixel configurations in places that we shouldn't be.

LATC is a DXT-esque alpha compression format that goes by a few other names (RGTC, 3DC). It might be useful to investigate using it to compress the alpha masks that we get from software rasterization. This patch set adds enums for that and recognition whether or not the device can support it.

Committed: http://code.google.com/p/skia/source/detail?r=14991

[krajcevski, 2014-05-28 22:09:39]

I added LATC in this patch set because after adding only ETC1, clang bots failed
(you can see this between patch sets one and two). There's something weird
having to do with how clang analyzed the code...

[robertphillips, 2014-05-29 12:53:18]

lgtm + nits/questions.

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrColor.h
File include/gpu/GrColor.h (right):

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrColor.h#ne...
include/gpu/GrColor.h:135: kRGB_GrColorComponentFlags,     //
kETC1_GrPixelConfig
align kLATC comment with others ?

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrTypes.h#ne...
include/gpu/GrTypes.h:284: 
I think these comments should only have one * or be formatted to match the other
comments.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:70: 
// Both these formats have 4x4 blocks of 8 bytes ?

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:75: } else {
Maybe: SkFAIL("Unknown compression format"); return 0; ?

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:459: 
Shouldn't this comment go above the prior line of code?

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:461: // however, it is only available on standard OpenGL
after version 1.3
I think setting this to false and just using 'hasCompressTex2D' in the following
logic might be clearer. What do you think?

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.h#ne...
src/gpu/gl/GrGLCaps.h:272: 
// LATC can appear under one of three possible names. The specific supported
form (i.e., the alias) needs to be known in order to set OpenGL's internalFormat
correctly. ?

[bsalomon, 2014-05-29 13:10:30]

How do we know at runtime if there is support for the compressed formats? We've
got GrDrawTargetCaps:isConfigRenderable() to check for renderable formats. And
we've got this one-off function GrGLCaps::bgraFormatSupport(). Maybe we need a
general function GrDrawTargetCaps::isConfigTexturable() (with a better name ).

[krajcevski, 2014-05-29 14:28:22]

Re isConfigRenderable:

Is this function meant to mean "This pixel config can be rendered into" or does
it mean "Something with data configured in this way can be rendered"?

If it's the former (which I suspect it is since it takes a with MSAA flag) then
we probably do need a separate isConfigTexturable() (or better).

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrColor.h
File include/gpu/GrColor.h (right):

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrColor.h#ne...
include/gpu/GrColor.h:135: kRGB_GrColorComponentFlags,     //
kETC1_GrPixelConfig
On 2014/05/29 12:53:18, robertphillips wrote:
> align kLATC comment with others ?

Done.

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/304743004/diff/40001/include/gpu/GrTypes.h#ne...
include/gpu/GrTypes.h:284: 
On 2014/05/29 12:53:18, robertphillips wrote:
> I think these comments should only have one * or be formatted to match the
other
> comments.

Done.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:70: 
On 2014/05/29 12:53:18, robertphillips wrote:
> // Both these formats have 4x4 blocks of 8 bytes ?

Done.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:75: } else {
On 2014/05/29 12:53:18, robertphillips wrote:
> Maybe: SkFAIL("Unknown compression format"); return 0; ?

Done.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:459: 
On 2014/05/29 12:53:18, robertphillips wrote:
> Shouldn't this comment go above the prior line of code?

Done.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:461: // however, it is only available on standard OpenGL
after version 1.3
On 2014/05/29 12:53:18, robertphillips wrote:
> I think setting this to false and just using 'hasCompressTex2D' in the
following
> logic might be clearer. What do you think?

I agree. This is left over from when I was reordering the checks.

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/304743004/diff/40001/src/gpu/gl/GrGLCaps.h#ne...
src/gpu/gl/GrGLCaps.h:272: 
On 2014/05/29 12:53:18, robertphillips wrote:
> // LATC can appear under one of three possible names. The specific supported
> form (i.e., the alias) needs to be known in order to set OpenGL's
internalFormat
> correctly. ?

Done.

[bsalomon, 2014-05-29 15:28:18]

On 2014/05/29 14:28:22, krajcevski wrote:
> Re isConfigRenderable:
> 
> Is this function meant to mean "This pixel config can be rendered into" or
does
> it mean "Something with data configured in this way can be rendered"?

The former. I'm thinking we need a parallel isTexturable() for "it can be
uploaded and bound as a texture". It doesn't need to be part of this CL, though.

lgtm

[krajcevski, 2014-05-29 17:10:52]

Hrm, I added the isConfigTexturable() command because having the logic that
detects whether or not certain formats are available set the fConfigRenderable[]
variables was just plain wrong.

[bsalomon, 2014-05-29 19:11:29]

https://codereview.chromium.org/304743004/diff/70001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/70001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:471: fConfigTextureSupport[kBGRA_8888_GrPixelConfig] =
true;
I think this needs to duplicate the logic to setup fBGRAFormatSupport. Also, we
should remove that and the associated getter and use this instead. Same with
f8BitPaletteSupport/kIndex_8_GrPixelConfig.

[krajcevski, 2014-05-29 19:46:42]

https://codereview.chromium.org/304743004/diff/70001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/70001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:471: fConfigTextureSupport[kBGRA_8888_GrPixelConfig] =
true;
On 2014/05/29 19:11:29, bsalomon wrote:
> I think this needs to duplicate the logic to setup fBGRAFormatSupport. Also,
we
> should remove that and the associated getter and use this instead. Same with
> f8BitPaletteSupport/kIndex_8_GrPixelConfig.

Done.

[bsalomon, 2014-05-29 19:59:10]

lgtm

[robertphillips, 2014-05-29 20:08:43]

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:615: const GrDrawTargetCaps* caps = fGpu->caps();
Wrong config?

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:1138: 
Can we just start the loop at 1 ?

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:66: if (GrPixelConfigIsCompressed(fDesc.fConfig)) {
Do we need some assert about no mipmaps in here?

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:79: default:
comrpessed ?

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:451: for (int i = 0; i < numFormats; ++i) {
Yoda-fy this ?

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:477: // however, it is only available on standard OpenGL
after version 1.3
>= ?!?

[krajcevski, 2014-05-29 20:31:27]

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:615: const GrDrawTargetCaps* caps = fGpu->caps();
On 2014/05/29 20:08:43, robertphillips wrote:
> Wrong config?

Done.

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:1138: 
On 2014/05/29 20:08:43, robertphillips wrote:
> Can we just start the loop at 1 ?

Done.

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:66: if (GrPixelConfigIsCompressed(fDesc.fConfig)) {
On 2014/05/29 20:08:43, robertphillips wrote:
> Do we need some assert about no mipmaps in here?

I don't think so, since the texture can be both compressed and contain mip maps.
If there's a function that generates mipmaps, then we should make sure that it
doesn't get called on compressed textures...

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:79: default:
On 2014/05/29 20:08:43, robertphillips wrote:
> comrpessed ?

The if statement checks that the config is compressed.

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:451: for (int i = 0; i < numFormats; ++i) {
On 2014/05/29 20:08:43, robertphillips wrote:
> Yoda-fy this ?

Done.

https://codereview.chromium.org/304743004/diff/90001/src/gpu/gl/GrGLCaps.cpp#...
src/gpu/gl/GrGLCaps.cpp:477: // however, it is only available on standard OpenGL
after version 1.3
On 2014/05/29 20:08:43, robertphillips wrote:
> >= ?!?

Done.

[bsalomon, 2014-05-29 20:38:19]

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/304743004/diff/90001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:66: if (GrPixelConfigIsCompressed(fDesc.fConfig)) {
On 2014/05/29 20:31:28, krajcevski wrote:
> On 2014/05/29 20:08:43, robertphillips wrote:
> > Do we need some assert about no mipmaps in here?
> 
> I don't think so, since the texture can be both compressed and contain mip
maps.
> If there's a function that generates mipmaps, then we should make sure that it
> doesn't get called on compressed textures...

Oh yes, we lazily compute mipmaps for bitmap-textures. Search for GenMipmaps (or
whatever that gl function is called...)

[krajcevski, 2014-05-29 21:03:20]

I added a check at the last minute (right before the call to glGenerateMipmap).

[bsalomon, 2014-05-29 21:05:36]

lgtm

[krajcevski, 2014-05-30 13:46:37]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-05-30 13:46:59]

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

[commit-bot: I haz the power, 2014-05-30 13:56:03]

Change committed as 14991