Bug 197900 – Please support WEBGL_compressed_texture_etc1 extension (and possibly WEBGL_compressed_texture_etc too)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

Per WebGL WG face-to-face meeting from May 14, 2019:

Could WebKit please expose the ETC compressed texture formats in the WebGL implementation?
https://www.khronos.org/registry/webgl/extensions/WEBGL_compressed_texture_etc1/

This should be easily supportable, and should work on essentially all iOS hardware.

Separately, there was a question of whether WebKit could start using OpenGL ES 3.0 contexts inside its current WebGL implementation. If that's feasible, then it would be easy to also support the ETC2 compressed texture formats and extension:
https://www.khronos.org/registry/webgl/extensions/WEBGL_compressed_texture_etc/

Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/50799006>

[Kenneth Russell]

Working on implementing this.

[Kenneth Russell]

Created attachment 379281 [details]
Patch

[Kenneth Russell]

Dean, Alex: could you please review?

The new enums violate the style guide but are in line with the format of the other OpenGL-related enums.

I had difficulty testing this. The only platform these will be supported on right now is the iOS Simulator with the current OpenGL ES backend and I couldn't fully debug it. Hoping we can land these changes and finish debugging them in a follow-on bug if they don't actually work on iOS hardware.

[Alex Christensen]

Comment on attachment 379281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379281&action=review

It definitely needs to compile on Linux.
I'll need to compile for an iOS device and open https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-compressed-texture-etc.html?webglVersion=1&quiet=0&quick=1 and https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-compressed-texture-etc1.html?webglVersion=1&quiet=0&quick=1 to make sure it's supported.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:235
> +        || name == "GL_CHROMIUM_compressed_texture_etc") {

Does the name have to have CHROMIUM in it?

[Alex Christensen]

(In reply to Kenneth Russell from comment #0)
> This should be easily supportable, and should work on essentially all iOS
> hardware.
Could you elaborate?  Is there hardware that you know isn't supported?  Will it fall back if it's not supported?

[Kenneth Russell]

(In reply to Alex Christensen from comment #6)
> (In reply to Kenneth Russell from comment #0)
> > This should be easily supportable, and should work on essentially all iOS
> > hardware.
> Could you elaborate?  Is there hardware that you know isn't supported?  Will
> it fall back if it's not supported?

I should have said "all iOS hardware". Note I haven't yet gotten this to work in the iOS simulator, but there were some bugs in my earlier patch which are now fixed.

[Kenneth Russell]

Comment on attachment 379281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379281&action=review

Linux build errors are hopefully fixed with the new patch.

>> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:235
>> +        || name == "GL_CHROMIUM_compressed_texture_etc") {
> 
> Does the name have to have CHROMIUM in it?

That's the name of the extension ANGLE advertises when the ETC2 compressed texture formats are natively supported. (WebGL, compared to ES 3.0, had to make them optional to avoid having to decompress them on some platforms.) It would be simplest to phrase the associated code in terms of that extension, rather than inventing a new extension name.

[Kenneth Russell]

Created attachment 379395 [details]
Patch

[Alex Christensen]

Comment on attachment 379395 [details]
Patch

r-
We do not want to add any CHROMIUM prefixed things in WebKit.  If ANGLE vends it, it should be ANGLE prefixed.
This also must pass all existing tests.

[Kenneth Russell]

Created attachment 380790 [details]
Patch

[Kenneth Russell]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 379395 [details]
> Patch
> 
> r-
> We do not want to add any CHROMIUM prefixed things in WebKit.  If ANGLE
> vends it, it should be ANGLE prefixed.
> This also must pass all existing tests.

ANGLE has been updated to vend GL_ANGLE_compressed_texture_etc instead and this patch has been updated.

The breakage of WebGL 2.0 tests in the last patch was caused by some testing code accidentally left in; should be fixed now.

[Kenneth Russell]

Created attachment 382872 [details]
Patch

[Kenneth Russell]

This revised patch has finally been successfully tested in the iOS simulator. The changes to GraphicsContext3DCocoa.mm were made in support of more easily setting m_attribs.isWebGL2 to true, to force-allocate an OpenGL ES 3.0 context.

Hoping this passes the EWS bots.

[Dean Jackson]

Comment on attachment 382872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382872&action=review

Thanks for turning on ASTC as well. We hadn't done that yet because we were worried about moving to a GLES 3 backend.

> Source/WebCore/html/canvas/WebGLCompressedTextureETC.cpp:64
> +    bool res = context.graphicsContext3D()->getExtensions().supports("GL_ANGLE_compressed_texture_etc");
> +    os_log(OS_LOG_DEFAULT, "WebGLCompressedTextureETC::supported returning %d", (int) res);
> +    return res;

I think you accidentally left this logging in.

> Source/WebCore/html/canvas/WebGLCompressedTextureETC1.cpp:55
> +    bool res = context.graphicsContext3D()->getExtensions().supports("GL_OES_compressed_ETC1_RGB8_texture");
> +    os_log(OS_LOG_DEFAULT, "WebGLCompressedTextureETC1::supported returning %d", (int) res);
> +    return res;

Same here.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5575
> +    case Extensions3D::ETC1_RGB8_OES:
>          {

Nit: I believe our style guide says the { here should go on the line above - although it might not be consistently applied in this file.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5653
> +        {

Ditto.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5656
> +            bytesRequired = (width + kEACAndETC2BlockSize - 1) / kEACAndETC2BlockSize;
> +            bytesRequired *= (height + kEACAndETC2BlockSize - 1) / kEACAndETC2BlockSize;
> +            bytesRequired *= 8;

Could this ever overflow?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5663
> +        {

Ditto.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5666
> +            bytesRequired = (width + kEACAndETC2BlockSize - 1) / kEACAndETC2BlockSize;
> +            bytesRequired *= (height + kEACAndETC2BlockSize - 1) / kEACAndETC2BlockSize;
> +            bytesRequired *= 16;

Overflow?

[Kenneth Russell]

Created attachment 382972 [details]
Patch

[Kenneth Russell]

Comment on attachment 382872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382872&action=review

>> Source/WebCore/html/canvas/WebGLCompressedTextureETC.cpp:64
>> +    return res;
> 
> I think you accidentally left this logging in.

Sorry about that. Fixed.

>> Source/WebCore/html/canvas/WebGLCompressedTextureETC1.cpp:55
>> +    return res;
> 
> Same here.

Sorry again. Removed.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5575
>>          {
> 
> Nit: I believe our style guide says the { here should go on the line above - although it might not be consistently applied in this file.

Thanks, fixed.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5653
>> +        {
> 
> Ditto.

Fixed.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5656
>> +            bytesRequired *= 8;
> 
> Could this ever overflow?

Not sure - Chromium's similar code doesn't use checked math - but have changed this to use checked math for safety.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5663
>> +        {
> 
> Ditto.

Fixed.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5666
>> +            bytesRequired *= 16;
> 
> Overflow?

Used CheckedMath here too.

[WebKit Commit Bot]

Comment on attachment 382972 [details]
Patch

Clearing flags on attachment: 382972

Committed r252226: <https://trac.webkit.org/changeset/252226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.