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!
<rdar://problem/50799006>
Working on implementing this.
Created attachment 379281 [details] Patch
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.
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?
(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?
(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.
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.
Created attachment 379395 [details] Patch
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.
Created attachment 380790 [details] Patch
(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.
Created attachment 382872 [details] Patch
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.
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?
Created attachment 382972 [details] Patch
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.
Comment on attachment 382972 [details] Patch Clearing flags on attachment: 382972 Committed r252226: <https://trac.webkit.org/changeset/252226>
All reviewed patches have been landed. Closing bug.