Initial work to get ETC1 data up to the GPU

Initial work to get ETC1 data up to the GPU

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

Committed: https://skia.googlesource.com/skia/+/9c0e629c64c0fa93ac9bf5c2eaa1821370a6fbe5

[robertphillips, 2014-05-27 22:01:49]

https://codereview.chromium.org/302783002/diff/20001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302783002/diff/20001/gm/etc1bitmap.cpp#newcode41
gm/etc1bitmap.cpp:41: SkData *fileData =
SkData::NewFromFileName(filename.c_str());
NULL == fileData ?

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

https://codereview.chromium.org/302783002/diff/20001/include/gpu/GrTypes.h#ne...
include/gpu/GrTypes.h:658: desc.fBytesPerBlock = 4;
break; ?

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h#newcode96
src/gpu/GrGpu.h:96: * @param desc        describes the texture to be created.
Mention its compressedness ?

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h#newcode99
src/gpu/GrGpu.h:99: *                    with respect to the width and height.
format ?

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

https://codereview.chromium.org/302783002/diff/20001/src/gpu/SkGr.cpp#newcode181
src/gpu/SkGr.cpp:181: SkData *data = bitmap->pixelRef()->refEncodedData();
hmmm

https://codereview.chromium.org/302783002/diff/20001/src/gpu/SkGr.cpp#newcode183
src/gpu/SkGr.cpp:183: // Is this even encoded data?
NULL == data ?

https://codereview.chromium.org/302783002/diff/20001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/302783002/diff/20001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:731: 
Remove this (tempStorage) ?

[krajcevski, 2014-05-27 22:19:28]

https://codereview.chromium.org/302783002/diff/20001/gm/etc1bitmap.cpp
File gm/etc1bitmap.cpp (right):

https://codereview.chromium.org/302783002/diff/20001/gm/etc1bitmap.cpp#newcode41
gm/etc1bitmap.cpp:41: SkData *fileData =
SkData::NewFromFileName(filename.c_str());
On 2014/05/27 22:01:50, robertphillips wrote:
> NULL == fileData ?

Done.

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

https://codereview.chromium.org/302783002/diff/20001/include/gpu/GrTypes.h#ne...
include/gpu/GrTypes.h:658: desc.fBytesPerBlock = 4;
On 2014/05/27 22:01:50, robertphillips wrote:
> break; ?

Done.

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h#newcode96
src/gpu/GrGpu.h:96: * @param desc        describes the texture to be created.
On 2014/05/27 22:01:50, robertphillips wrote:
> Mention its compressedness ?

Done.

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h#newcode99
src/gpu/GrGpu.h:99: *                    with respect to the width and height.
On 2014/05/27 22:01:50, robertphillips wrote:
> format ?

Done.

https://codereview.chromium.org/302783002/diff/20001/src/gpu/GrGpu.h#newcode99
src/gpu/GrGpu.h:99: *                    with respect to the width and height.
On 2014/05/27 22:01:50, robertphillips wrote:
> format ?

Done.

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

https://codereview.chromium.org/302783002/diff/20001/src/gpu/SkGr.cpp#newcode183
src/gpu/SkGr.cpp:183: // Is this even encoded data?
On 2014/05/27 22:01:50, robertphillips wrote:
> NULL == data ?

Done.

https://codereview.chromium.org/302783002/diff/20001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/302783002/diff/20001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:731: 
On 2014/05/27 22:01:50, robertphillips wrote:
> Remove this (tempStorage) ?

Done.

[robertphillips, 2014-05-28 13:51:39]

What do you think about making the compressed types in GrCompressedFormat new
values in GrPixelConfig? Once we had that then we could reuse the existing
GrContext::createTexture entry point to create a compressed texture (we would
need to ignore the rowBytes parameter - or maybe reuse it for data size). This
would reduce the public API surface.

[krajcevski, 2014-05-30 14:43:47]

I've updated the CL to use the new API.

[robertphillips, 2014-05-30 15:00:22]

lgtm + a nit & a few questions

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

https://codereview.chromium.org/302783002/diff/160002/include/gpu/GrTypes.h#n...
include/gpu/GrTypes.h:658: // Unknown pixel config...
SkFAIL here?

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h#newcode89
src/gpu/GrGpu.h:89: /**
Can we test these cases (on a Xoom say) ?

https://codereview.chromium.org/302783002/diff/160002/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:181: SkData *data = bitmap->pixelRef()->refEncodedData();
What do you think about making this a static function (e.g.,
create_etc1_texture) and using it like:
   GrTexture* result = create_etc1_texture(desc, params, bitmap);
   if (NULL != result) {
       return result;
   }

?

https://codereview.chromium.org/302783002/diff/160002/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:708: 
Can we test this case?

[bsalomon, 2014-05-30 15:16:58]

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h#newcode107
src/gpu/GrGpu.h:107: GrTexture* createCompressedTexture(const GrTextureDesc&
desc, const void* srcData);
I'm wondering if this makes sense as a public GrGpu function. I can see wanting
to split in the implementation (either as onCreateTexture and
onCreateCompressedTexture or in the GrGpuGL code), but isn't the distinction
between creating a compressed or uncompressed texture now already captured in
the desc's config? Why make the caller worry about this detail?

[krajcevski, 2014-05-30 16:21:50]

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

https://codereview.chromium.org/302783002/diff/160002/include/gpu/GrTypes.h#n...
include/gpu/GrTypes.h:658: // Unknown pixel config...
On 2014/05/30 15:00:22, robertphillips wrote:
> SkFAIL here?

Done.

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/GrGpu.h#newcode107
src/gpu/GrGpu.h:107: GrTexture* createCompressedTexture(const GrTextureDesc&
desc, const void* srcData);
On 2014/05/30 15:16:58, bsalomon wrote:
> I'm wondering if this makes sense as a public GrGpu function. I can see
wanting
> to split in the implementation (either as onCreateTexture and
> onCreateCompressedTexture or in the GrGpuGL code), but isn't the distinction
> between creating a compressed or uncompressed texture now already captured in
> the desc's config? Why make the caller worry about this detail?

I agree to an extent... it's always better to be implicit rather than explicit.
That being said, the distinction between compressed versus uncompressed textures
probably doesn't need to be at this height of the API. I've changed the
description to reflect the new behavior of createTexture by moving
createComrpessedTexture logic into it.

https://codereview.chromium.org/302783002/diff/160002/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/302783002/diff/160002/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:181: SkData *data = bitmap->pixelRef()->refEncodedData();
On 2014/05/30 15:00:22, robertphillips wrote:
> What do you think about making this a static function (e.g.,
> create_etc1_texture) and using it like:
>    GrTexture* result = create_etc1_texture(desc, params, bitmap);
>    if (NULL != result) {
>        return result;
>    }
> 
> ?

Sounds good to me.

Done.

[bsalomon, 2014-05-30 17:59:57]

lgtm

https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:125: SkASSERT((desc.fFlags & kRenderTarget_GrTextureFlagBit)
== 0);
Since GrContext:createTexture() can be called by clients, perhaps we should
gracefully fail in this case (return null).

[krajcevski, 2014-05-30 18:35:00]

The CQ bit was checked by krajcevski@google.com

[krajcevski, 2014-05-30 18:35:20]

https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:125: SkASSERT((desc.fFlags & kRenderTarget_GrTextureFlagBit)
== 0);
On 2014/05/30 17:59:57, bsalomon wrote:
> Since GrContext:createTexture() can be called by clients, perhaps we should
> gracefully fail in this case (return null).

The caps() should make sure that compressed formats are not able to be rendered
into (isConfigRenderable should return false for any config that
GrPixelConfigIsCompressed returns true)

[commit-bot: I haz the power, 2014-05-30 18:35:41]

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

[commit-bot: I haz the power, 2014-05-30 18:41:54]

Change committed as 15001

[bsalomon, 2014-05-30 18:48:50]

On 2014/05/30 18:35:20, krajcevski wrote:
> https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp
> File src/gpu/GrGpu.cpp (right):
> 
>
https://codereview.chromium.org/302783002/diff/190001/src/gpu/GrGpu.cpp#newco...
> src/gpu/GrGpu.cpp:125: SkASSERT((desc.fFlags & kRenderTarget_GrTextureFlagBit)
> == 0);
> On 2014/05/30 17:59:57, bsalomon wrote:
> > Since GrContext:createTexture() can be called by clients, perhaps we should
> > gracefully fail in this case (return null).
> 
> The caps() should make sure that compressed formats are not able to be
rendered
> into (isConfigRenderable should return false for any config that
> GrPixelConfigIsCompressed returns true)

makes sense

[epoger, 2014-05-30 19:58:24]

See http://skbug.com/2631 ('ETC1 stuff broke our Chrome Canary build')

[krajcevski, 2014-05-30 20:32:39]

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

The reason for reverting is: ETC1 linking problems with chrome..

[krajcevski, 2014-06-02 14:32:57]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-06-02 14:33:16]

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

[commit-bot: I haz the power, 2014-06-02 14:38:19]

Change committed as 9c0e629c64c0fa93ac9bf5c2eaa1821370a6fbe5