Currently in the ANGLE backend for WebGL, the underlying context is created with all extensions enabled: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm#L327 This was a workaround done out of expediency in order to get bits on the screen. There is a note about it here: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp#L395 """ // Note there are no Extensions3D::ensureEnabled calls here. The ANGLE // backend currently assumes at a fairly deep level that // EGL_EXTENSIONS_ENABLED_ANGLE is set to true during context creation: for // the allocation of rectangular textures, etc. """ In order for WebKit's ANGLE implementation to be conformant in the long term, with a minimum of additional validation at the WebKit level, it'll be necessary to figure out which extensions the implementation actually relies on, and selectively enable them during context initialization.
Austin, may I ask you to take this bug?
I've got this initially working by requiring: GL_ANGLE_texture_rectangle GL_EXT_texture_format_BGRA8888 and placing: GL_ANGLE_framebuffer_multisample GL_ANGLE_framebuffer_blit GL_OES_rgb8_rgba8 GL_OES_packed_depth_stencil behind if(supports()) ensureEnabled(...);
GL_ANGLE_texture_rectangle is required for textures backed by IOSurfaces. GL_EXT_texture_format_BGRA8888 is required because GL_BGRA_EXT is used in EGL_CreatePbufferFromClientBuffer to make the pixel buffer from the IOSurface. The GraphicsContext3D supports antialiasing via GL_ANGLE_framebuffer_multisample GL_ANGLE_framebuffer_blit GL_OES_rgb8_rgba8 and we use them to resolve a multisampled renderbuffer. They're not required because `antialias` may be false. Finally, stencil-only is not allowed so the context only supports stencil via GL_OES_packed_depth_stencil. The extension is not required because `antialias` may be false.
Created attachment 377156 [details] Patch
Comment on attachment 377156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377156&action=review Fantastic work Austin! It's exciting to see how many extension-related conformance tests this patch fixes! Do any layout test expectations need to be updated? Deferring review to Dean and Alex. > Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:41 > + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ANGLE_instanced_arrays"); Does it seem necessary to conditionalize this with USE(ANGLE)? Many of the other ensureEnabled calls added by this patch are unconditional. > Source/WebCore/html/canvas/WebGLDepthTexture.cpp:40 > + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ARB_depth_texture"); Is there any problem calling ensureEnabled with GL_ARB_depth_texture if the underlying context doesn't support it? The ANGLE backend won't - it only supports ES extensions and ARB extensions are desktop-only.
(In reply to Kenneth Russell from comment #5) > Comment on attachment 377156 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377156&action=review > > Fantastic work Austin! It's exciting to see how many extension-related > conformance tests this patch fixes! > > Do any layout test expectations need to be updated? > There shouldn't be any test expectation changes. > Deferring review to Dean and Alex. > > > Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:41 > > + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ANGLE_instanced_arrays"); > > Does it seem necessary to conditionalize this with USE(ANGLE)? Many of the > other ensureEnabled calls added by this patch are unconditional. > It's conditionalized because #if !PLATFORM(GTK) && !USE(ANGLE) then the header isn't included. We could unconditionally include the header though. Not sure what the conventions are in the project. > > Source/WebCore/html/canvas/WebGLDepthTexture.cpp:40 > > + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ARB_depth_texture"); > > Is there any problem calling ensureEnabled with GL_ARB_depth_texture if the > underlying context doesn't support it? The ANGLE backend won't - it only > supports ES extensions and ARB extensions are desktop-only. This would just no-op but I put it here for consistency.
Comment on attachment 377156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377156&action=review >>> Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:41 >>> + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ANGLE_instanced_arrays"); >> >> Does it seem necessary to conditionalize this with USE(ANGLE)? Many of the other ensureEnabled calls added by this patch are unconditional. > > It's conditionalized because #if !PLATFORM(GTK) && !USE(ANGLE) then the header isn't included. We could unconditionally include the header though. Not sure what the conventions are in the project. Sounds fine to conditionalize it then. >>> Source/WebCore/html/canvas/WebGLDepthTexture.cpp:40 >>> + context.graphicsContext3D()->getExtensions().ensureEnabled("GL_ARB_depth_texture"); >> >> Is there any problem calling ensureEnabled with GL_ARB_depth_texture if the underlying context doesn't support it? The ANGLE backend won't - it only supports ES extensions and ARB extensions are desktop-only. > > This would just no-op but I put it here for consistency. Sounds fine.
Comment on attachment 377156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377156&action=review > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:360 > + size_t requiredExtensionCount = sizeof(requiredExtensions) / sizeof(requiredExtensions[0]); We usually use WTF_ARRAY_LENGTH for things like this. > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:361 > + for (unsigned i = 0; i < requiredExtensionCount; ++i) { Nit: Why not use unsigned for both or size_t for both? > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:371 > + size_t optionalExtensionCount = sizeof(optionalExtensions) / sizeof(optionalExtensions[0]); > + for (unsigned i = 0; i < optionalExtensionCount; ++i) { ditto
Created attachment 377269 [details] Patch
Comment on attachment 377156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377156&action=review >> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:360 >> + size_t requiredExtensionCount = sizeof(requiredExtensions) / sizeof(requiredExtensions[0]); > > We usually use WTF_ARRAY_LENGTH for things like this. Done. >> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:361 >> + for (unsigned i = 0; i < requiredExtensionCount; ++i) { > > Nit: Why not use unsigned for both or size_t for both? Done. >> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:371 >> + for (unsigned i = 0; i < optionalExtensionCount; ++i) { > > ditto Done.
Can this have another review? Thanks.
Comment on attachment 377269 [details] Patch Clearing flags on attachment: 377269 Committed r249218: <https://trac.webkit.org/changeset/249218>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54805832>