Implement the texture uploading of ASTC compression for WebGL(part II)

Implement the texture uploading of ASTC compression for WebGL(part II)

This is part II. (the Blink 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://src.chromium.org/viewvc/blink?view=rev&revision=201425

[cyzero.kim, 2015-08-07 06:57:29]

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

[cyzero.kim, 2015-08-07 06:57:30]

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

[bajones, 2015-08-07 22:01:09]

https://codereview.chromium.org/1265703005/diff/1/Source/modules/webgl/WebGLC...
File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):

https://codereview.chromium.org/1265703005/diff/1/Source/modules/webgl/WebGLC...
Source/modules/webgl/WebGLCompressedTextureASTC.cpp:81: return
"WEBGL_compressed_texture_astc";
The intermixing of "compressed_texture_astc" and "compressed_texture_astc_ldr"
in these patches feels strange. Shouldn't this be
"WEBGL_compressed_texture_astc_ldr"? Then again I'm also not sure if I
understand what the interface differences between the hdr/ldr extensions are. It
looks like they both expose the same format enums? Could you clarify how
developers use the two?

https://codereview.chromium.org/1265703005/diff/1/Source/modules/webgl/WebGLR...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1265703005/diff/1/Source/modules/webgl/WebGLR...
Source/modules/webgl/WebGLRenderingContextBase.cpp:5699: (int)format -
GL_COMPRESSED_RGBA_ASTC_4x4_KHR : (int)format -
GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR;
This has the same basic issue as I described in
https://codereview.chromium.org/1263043006/#msg4

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

Please take another look.

[Ken Russell (switch to Gerrit), 2015-08-18 00:05:03]

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.cpp:32: const int
WebGLCompressedTextureASTC::kSizeOfBlockSizeCompressASTC =
Please use arraysize(WebGLCompressedTextureASTC::BlockSizeCompressASTC) instead,
as is done in several other places.

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.cpp:45:
WebGLCompressedTextureASTC::kBlockSizeCompressASTC[i].CompressType + 20);
This addition looks wrong -- I think it should be 0x20, not 20. For exactly this
reason, please at least compute it, rather than hardcoding the contant.

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.cpp:71: return
"WEBGL_compressed_texture_astc_ldr";
At this point this should be "WEBGL_compressed_texture_astc".

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLCompressedTextureASTC.idl (right):

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.idl:8: WillBeGarbageCollected,
The Oilpan switch has been thrown for the WebGL module so this now needs to be
"GarbageCollected".

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.idl:10: ] interface
WebGLCompressedTextureASTC {
Note that the API for this extension was changed a bit to add a
getSupportedProfiles() method, but if you'd rather not implement that method (it
would basically require you finding all of the underlying OpenGL extensions
starting with GL_KHR_texture_compression_astc and adding their suffixes), we can
implement it after you've landed this initial patch.

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContext.cpp:126:
registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC);
This must be:

 registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC,
DraftExtension);

You can test your implementation by passing the command line argument
--enable-webgl-draft-extensions to the browser.

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContext.h (right):

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContext.h:82:
RefPtrWillBeMember<WebGLCompressedTextureASTC> m_webglCompressedTextureASTC;
Needs to be updated. Is now "PersistentWillBeMember".

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:5704: int kBlockX =
(max(width, kBlockWidth) + kBlockWidth - 1) / kBlockWidth;
These aren't constants so the "k" prefix is confusing.

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:5705: int kBlockY =
(max(height, kBlockHeight) + kBlockHeight - 1) / kBlockHeight;
Why are width and height being rounded up to kBlockWidth and kBlockHeight? Is
that correct? Should this code instead fail if someone attempts to upload a
texture that's smaller than a single block?

https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:5809: heightValid = true;
Is this correct? It seems wrong to me per
https://www.opengl.org/registry/specs/KHR/texture_compression_astc_hdr.txt -- it
says that INVALID_OPERATION is generated if:

  <width> is not a multiple of the format's block width, and <width> +
        <xoffset> is not equal to the value of TEXTURE_WIDTH.
  height is not a multiple of the format's block height, and <height>
        + <yoffset> is not equal to the value of TEXTURE_HEIGHT.

So it looks to me like the same sorts of size constraints that apply to the
other compressed texture formats apply here too.

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

On 2015/08/18 00:05:03, Ken Russell wrote:
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.cpp:32: const int
> WebGLCompressedTextureASTC::kSizeOfBlockSizeCompressASTC =
> Please use arraysize(WebGLCompressedTextureASTC::BlockSizeCompressASTC)
instead,
> as is done in several other places.
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.cpp:45:
> WebGLCompressedTextureASTC::kBlockSizeCompressASTC[i].CompressType + 20);
> This addition looks wrong -- I think it should be 0x20, not 20. For exactly
this
> reason, please at least compute it, rather than hardcoding the contant.
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.cpp:71: return
> "WEBGL_compressed_texture_astc_ldr";
> At this point this should be "WEBGL_compressed_texture_astc".
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLCompressedTextureASTC.idl (right):
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.idl:8: WillBeGarbageCollected,
> The Oilpan switch has been thrown for the WebGL module so this now needs to be
> "GarbageCollected".
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.idl:10: ] interface
> WebGLCompressedTextureASTC {
> Note that the API for this extension was changed a bit to add a
> getSupportedProfiles() method, but if you'd rather not implement that method
(it
> would basically require you finding all of the underlying OpenGL extensions
> starting with GL_KHR_texture_compression_astc and adding their suffixes), we
can
> implement it after you've landed this initial patch.
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLRenderingContext.cpp (right):
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContext.cpp:126:
> registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC);
> This must be:
> 
>  registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC,
> DraftExtension);
> 
> You can test your implementation by passing the command line argument
> --enable-webgl-draft-extensions to the browser.
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLRenderingContext.h (right):
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContext.h:82:
> RefPtrWillBeMember<WebGLCompressedTextureASTC> m_webglCompressedTextureASTC;
> Needs to be updated. Is now "PersistentWillBeMember".
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContextBase.cpp:5704: int kBlockX =
> (max(width, kBlockWidth) + kBlockWidth - 1) / kBlockWidth;
> These aren't constants so the "k" prefix is confusing.
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContextBase.cpp:5705: int kBlockY =
> (max(height, kBlockHeight) + kBlockHeight - 1) / kBlockHeight;
> Why are width and height being rounded up to kBlockWidth and kBlockHeight? Is
> that correct? Should this code instead fail if someone attempts to upload a
> texture that's smaller than a single block?
> 
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContextBase.cpp:5809: heightValid = true;
> Is this correct? It seems wrong to me per
> https://www.opengl.org/registry/specs/KHR/texture_compression_astc_hdr.txt --
it
> says that INVALID_OPERATION is generated if:
> 
>   <width> is not a multiple of the format's block width, and <width> +
>         <xoffset> is not equal to the value of TEXTURE_WIDTH.
>   height is not a multiple of the format's block height, and <height>
>         + <yoffset> is not equal to the value of TEXTURE_HEIGHT.
> 
> So it looks to me like the same sorts of size constraints that apply to the
> other compressed texture formats apply here too.

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-25 06:39:01]

On 2015/08/19 07:19:12, cyzero.kim wrote:
> On 2015/08/18 00:05:03, Ken Russell wrote:
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLCompressedTextureASTC.cpp:32: const int
> > WebGLCompressedTextureASTC::kSizeOfBlockSizeCompressASTC =
> > Please use arraysize(WebGLCompressedTextureASTC::BlockSizeCompressASTC)
> instead,
> > as is done in several other places.
> > Yes, i have modified it.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLCompressedTextureASTC.cpp:45:
> > WebGLCompressedTextureASTC::kBlockSizeCompressASTC[i].CompressType + 20);
> > This addition looks wrong -- I think it should be 0x20, not 20. For exactly
> this
> > reason, please at least compute it, rather than hardcoding the contant.
> > you're right.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLCompressedTextureASTC.cpp:71: return
> > "WEBGL_compressed_texture_astc_ldr";
> > At this point this should be "WEBGL_compressed_texture_astc".
> > Ok, i also agree your comment.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > File Source/modules/webgl/WebGLCompressedTextureASTC.idl (right):
> > 
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLCompressedTextureASTC.idl:8:
WillBeGarbageCollected,
> > The Oilpan switch has been thrown for the WebGL module so this now needs to
be
> > "GarbageCollected".
> > I have modified it.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLCompressedTextureASTC.idl:10: ] interface
> > WebGLCompressedTextureASTC {
> > Note that the API for this extension was changed a bit to add a
> > getSupportedProfiles() method, but if you'd rather not implement that method
> (it
> > would basically require you finding all of the underlying OpenGL extensions
> > starting with GL_KHR_texture_compression_astc and adding their suffixes), we
> can
> > implement it after you've landed this initial patch.
> > Ok.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > File Source/modules/webgl/WebGLRenderingContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLRenderingContext.cpp:126:
> > registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC);
> > This must be:
> > 
> >  registerExtension<WebGLCompressedTextureASTC>(m_webglCompressedTextureASTC,
> > DraftExtension);
> > 
> > You can test your implementation by passing the command line argument
> > --enable-webgl-draft-extensions to the browser.
> > Yes, I have modified it and checking this flags in mobile browser.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > File Source/modules/webgl/WebGLRenderingContext.h (right):
> > 
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLRenderingContext.h:82:
> > RefPtrWillBeMember<WebGLCompressedTextureASTC> m_webglCompressedTextureASTC;
> > Needs to be updated. Is now "PersistentWillBeMember".
> > Ok.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLRenderingContextBase.cpp:5704: int kBlockX =
> > (max(width, kBlockWidth) + kBlockWidth - 1) / kBlockWidth;
> > These aren't constants so the "k" prefix is confusing.
> > I had cleanup.
> >
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLRenderingContextBase.cpp:5705: int kBlockY =
> > (max(height, kBlockHeight) + kBlockHeight - 1) / kBlockHeight;
> > Why are width and height being rounded up to kBlockWidth and kBlockHeight?
Is
> > that correct? Should this code instead fail if someone attempts to upload a
> > texture that's smaller than a single block?
> > You're right. it is unnecessary work. 
> > It's working fine that smaller size than a block size.
>
https://codereview.chromium.org/1265703005/diff/20001/Source/modules/webgl/We...
> > Source/modules/webgl/WebGLRenderingContextBase.cpp:5809: heightValid = true;
> > Is this correct? It seems wrong to me per
> > https://www.opengl.org/registry/specs/KHR/texture_compression_astc_hdr.txt
--
> it
> > says that INVALID_OPERATION is generated if:
> > 
> >   <width> is not a multiple of the format's block width, and <width> +
> >         <xoffset> is not equal to the value of TEXTURE_WIDTH.
> >   height is not a multiple of the format's block height, and <height>
> >         + <yoffset> is not equal to the value of TEXTURE_HEIGHT.
> > 
> > So it looks to me like the same sorts of size constraints that apply to the
> > other compressed texture formats apply here too.
> > Width and block size means that there is no relation. It's scalable.
And the Blocks are not required to be square, and ASTC block dimensions
are not required to be powers of two. So, size constraints is not required.
I have checking for the 3x3 size and 7x7 size of 4x4 block-size.(test result:
http://code.google.com/p/chromium/issues/detail?id=516672)
Please take another look.

[Ken Russell (switch to Gerrit), 2015-08-27 03:19:02]

Thanks, the compressed size computation looks better. LGTM. You can CQ this
yourself once the minor issue below is addressed in a following patch set.

https://codereview.chromium.org/1265703005/diff/40001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):

https://codereview.chromium.org/1265703005/diff/40001/Source/modules/webgl/We...
Source/modules/webgl/WebGLCompressedTextureASTC.cpp:35: const int
kAlphaFormatGap = 0x20;
Please compute this as COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR -
GL_COMPRESSED_RGBA_ASTC_4x4_KHR, rather than hardcoding it. There was obviously
already a mistake here previously.

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

On 2015/08/27 03:19:02, Ken Russell wrote:
> Thanks, the compressed size computation looks better. LGTM. You can CQ this
> yourself once the minor issue below is addressed in a following patch set.
> 
>
https://codereview.chromium.org/1265703005/diff/40001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLCompressedTextureASTC.cpp (right):
> 
>
https://codereview.chromium.org/1265703005/diff/40001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLCompressedTextureASTC.cpp:35: const int
> kAlphaFormatGap = 0x20;
> Please compute this as COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR -
> GL_COMPRESSED_RGBA_ASTC_4x4_KHR, rather than hardcoding it. There was
obviously
> already a mistake here previously.

I'm done:)

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

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1265703005/#ps60001
(title: "Compute the gap between GL_COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR and
GL_COMPRESSED_RGBA_ASTC_4x4_KHR.")

[commit-bot: I haz the power, 2015-08-28 15:33:27]

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

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

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

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

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

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

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

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

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

[commit-bot: I haz the power, 2015-08-28 17:25:31]

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

[commit-bot: I haz the power, 2015-08-28 19:16:39]

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

[commit-bot: I haz the power, 2015-08-28 19:16:39]

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

[Ken Russell (switch to Gerrit), 2015-08-28 20:05:25]

On 2015/08/28 19:16:39, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Filed http://crbug.com/526252 about this apparent flake.

[Ken Russell (switch to Gerrit), 2015-08-28 20:05:32]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2015-08-28 20:05:41]

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

[commit-bot: I haz the power, 2015-08-28 21:05:43]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201425