Bug 200900 – Selectively enable extensions in ANGLE backend for WebGL

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

[Kenneth Russell]

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.

[Kenneth Russell]

Austin, may I ask you to take this bug?

[Austin Eng]

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(...);

[Austin Eng]

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.

[Austin Eng]

Created attachment 377156 [details]
Patch

[Kenneth Russell]

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.

[Austin Eng]

(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.

[Kenneth Russell]

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.

[Alex Christensen]

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

[Austin Eng]

Created attachment 377269 [details]
Patch

[Austin Eng]

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.

[Austin Eng]

Can this have another review? Thanks.

[WebKit Commit Bot]

Comment on attachment 377269 [details]
Patch

Clearing flags on attachment: 377269

Committed r249218: <https://trac.webkit.org/changeset/249218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54805832>