Implement the texture uploading of ASTC compression for WebGL

Implement the texture uploading of ASTC compression for WebGL

This is part I (the Chromium src repo.)

This feature is for ARM and mobile devices.
It's focused the power efficiency benefits
of texture compression, most of which come from reducing
the bandwidth needed for data transfers on the mobile device.

The ASTC compression is involved including reducing memory
footprint and loading time and even increase performance.

This implementation has been verified on the Galaxy S6.
I also plan to apply this feature on the Smart TV.

BUG=516672
Committed: https://crrev.com/0a85bd6751240fba49cd7e1d30279dc888096aeb
Cr-Commit-Position: refs/heads/master@{#346198}

[cyzero.kim, 2015-08-07 06:56:44]

cyzero.kim@samsung.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org

[cyzero.kim, 2015-08-07 06:56:44]

It should be possible to upload texture of the ASTC compression  on the Mali
GPU.
Please review this one.

[bajones, 2015-08-07 21:35:46]

Minor issue in gles2_cmd_decoder, but generally looking good! Thanks!

[bajones, 2015-08-07 21:35:59]

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9111: 4, 4,   /*
GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR */
These values are not currently used. May want to remove (see further comments
below).

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9185: (int)format -
GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR;
The way this algorithim is set up it will never index into the upper half of
kASTCBlockSize. That looks like an error at first glance, but it turns out that
the values are the same either way, so it doesn't make a difference even if it
feels weird. This should be modified to either index a unique kASTCBlockSize for
each enum or the latter half of kASTCBlockSize should be dropped.

[Ken Russell (switch to Gerrit), 2015-08-07 22:32:33]

kbr@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2015-08-07 22:32:33]

Thanks for this contribution. The overall direction looks good. One comment, and
I echo Brandon's comments about the block size table. Adding zmo and piman for
further review.

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:388: bool have_astc =
extensions.Contains("GL_OES_texture_compression_astc") ||
I don't think we should reference the GL_OES_texture_compression_atc extension
now that it's been superseded by the KHR version.

https://codereview.chromium.org/1263043006/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:402: for (GLint i = 0; i <=
astc_rgba_type_max; i++) {
Why not just iterate from COMPRESSED_RGBA_ASTC_4x4_KHR to
COMPRESSED_RGBA_ASTC_12x12_KHR and add all of those values?

[cyzero.kim, 2015-08-10 13:23:17]

Please take another look.

[piman, 2015-08-17 23:00:21]

Can you also add unit tests for the validation of the new formats?

https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:147: *size_in_bytes = width
/ 2;
For ASTC (4x4), you need width%4 == 0, and size_in_bytes will be width/4, since
it uses 16 bytes for 16 pixels.

https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9095: static const ASTCBlockSize
kASTCBlockSize[] =
nit: you're in an anonymous namespace, so you don't need "static"

https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9170: (int)format -
GL_COMPRESSED_RGBA_ASTC_4x4_KHR :
nit: no c-style cast.

https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9171: (int)format -
GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR;
nit: no c-style cast.

https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9181: bytes_required = (kBlockX
* kBlockY) << 4;
nit: rather than << 4, can you write * kASTCBlockBytes, where kASTCBlockBytes is
defined to 16?

https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:48: ASTC,
Can you name this one ASTC_4X4? For future-proofing as well as making it clearer
everywhere which block size we're talking about?

[cyzero.kim, 2015-08-19 07:18:30]

On 2015/08/17 23:00:21, piman (slow to review) wrote:
> Can you also add unit tests for the validation of the new formats?
> 
>
https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):
> 
>
https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:147: *size_in_bytes =
width
> / 2;
> For ASTC (4x4), you need width%4 == 0, and size_in_bytes will be width/4,
since
> it uses 16 bytes for 16 pixels.
> 
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9095: static const
ASTCBlockSize
> kASTCBlockSize[] =
> nit: you're in an anonymous namespace, so you don't need "static"
> 
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9170: (int)format -
> GL_COMPRESSED_RGBA_ASTC_4x4_KHR :
> nit: no c-style cast.
> 
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9171: (int)format -
> GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR;
> nit: no c-style cast.
> 
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9181: bytes_required =
(kBlockX
> * kBlockY) << 4;
> nit: rather than << 4, can you write * kASTCBlockBytes, where kASTCBlockBytes
is
> defined to 16?
> 
>
https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffer.h
> File ui/gfx/gpu_memory_buffer.h (right):
> 
>
https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffe...
> ui/gfx/gpu_memory_buffer.h:48: ASTC,
> Can you name this one ASTC_4X4? For future-proofing as well as making it
clearer
> everywhere which block size we're talking about?

Thank you for your feedback.
That's good points.
I'll checking all items.
I'm Out Of the Office (~23-Aug.) and  I'll be back at 24-Aug.

[cyzero.kim, 2015-08-24 00:40:33]

The CQ bit was checked by cyzero.kim@samsung.com

[cyzero.kim, 2015-08-24 00:40:34]

The CQ bit was unchecked by cyzero.kim@samsung.com

[cyzero.kim, 2015-08-25 06:16:34]

On 2015/08/19 07:18:30, cyzero.kim wrote:
> On 2015/08/17 23:00:21, piman (slow to review) wrote:
> > Can you also add unit tests for the validation of the new formats?
> > Yes, I have added unit test.
(https://codereview.chromium.org/1263043006/diff2/20001:40001/gpu/command_buff...)
> >
>
https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
> > File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1263043006/diff/20001/content/common/gpu/clie...
> > content/common/gpu/client/gpu_memory_buffer_impl.cc:147: *size_in_bytes =
> width
> > / 2;
> > For ASTC (4x4), you need width%4 == 0, and size_in_bytes will be width/4,
> since
> > it uses 16 bytes for 16 pixels.
> > The ASTC width is not a multiple of the format's block width. 
> > And Here is no need to add astc format for WebGL texture uploading.So, i
have removed it.
> > (This is 3x3 ASTC texture example :
http://chromium.googlecode.com/issues/attachment?aid=5166720008002&name=Scree...)
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:9095: static const
> ASTCBlockSize
> > kASTCBlockSize[] =
> > nit: you're in an anonymous namespace, so you don't need "static"
> > Ok, right.
> >
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:9170: (int)format -
> > GL_COMPRESSED_RGBA_ASTC_4x4_KHR :
> > nit: no c-style cast.
> > I have modified it.
> >
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:9171: (int)format -
> > GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR;
> > nit: no c-style cast.
> > I have modified it.
> >
>
https://codereview.chromium.org/1263043006/diff/20001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:9181: bytes_required =
> (kBlockX
> > * kBlockY) << 4;
> > nit: rather than << 4, can you write * kASTCBlockBytes, where
kASTCBlockBytes
> is
> > defined to 16?
> > Yes, I have modified it.
> >
>
https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffer.h
> > File ui/gfx/gpu_memory_buffer.h (right):
> > 
> >
>
https://codereview.chromium.org/1263043006/diff/20001/ui/gfx/gpu_memory_buffe...
> > ui/gfx/gpu_memory_buffer.h:48: ASTC,
> > Can you name this one ASTC_4X4? For future-proofing as well as making it
> clearer
> > everywhere which block size we're talking about?
> > Here is no need to add the ASTC format for WebGL texture uploading. So, I
have removed in gpu memory buffer.

Please take another look.

[Zhenyao Mo, 2015-08-25 14:15:16]

LGTM with style nits

https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9231: {
nit: this is against code style.  see
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initi...

https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9305: static_cast<int>(format -
GL_COMPRESSED_RGBA_ASTC_4x4_KHR) :
nit: 4 space indent here and next line

[cyzero.kim, 2015-08-26 05:47:47]

On 2015/08/25 14:15:16, Zhenyao Mo wrote:
> LGTM with style nits
> 
>
https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9231: {
> nit: this is against code style.  see
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initi...
> 
>
https://codereview.chromium.org/1263043006/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9305: static_cast<int>(format
-
> GL_COMPRESSED_RGBA_ASTC_4x4_KHR) :
> nit: 4 space indent here and next line

Thank you for your comment.
Done.

[cyzero.kim, 2015-08-28 15:34:15]

The CQ bit was checked by cyzero.kim@samsung.com

[cyzero.kim, 2015-08-28 15:34:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1263043006/#ps60001
(title: "Change coding style.")

[commit-bot: I haz the power, 2015-08-28 15:34:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1263043006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1263043006/60001

[commit-bot: I haz the power, 2015-08-28 15:43:34]

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

[commit-bot: I haz the power, 2015-08-28 15:43:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[cyzero.kim, 2015-08-28 17:23:23]

The CQ bit was checked by cyzero.kim@samsung.com

[cyzero.kim, 2015-08-28 17:23:23]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1263043006/#ps80001
(title: "Add braces and cleanup.")

[commit-bot: I haz the power, 2015-08-28 17:23:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1263043006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1263043006/80001

[commit-bot: I haz the power, 2015-08-28 18:48:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-28 18:49:45]

Patchset 5 (id:??) landed as
https://crrev.com/0a85bd6751240fba49cd7e1d30279dc888096aeb
Cr-Commit-Position: refs/heads/master@{#346198}