Bug 200900 - Selectively enable extensions in ANGLE backend for WebGL
Summary: Selectively enable extensions in ANGLE backend for WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Austin Eng
URL:
Keywords: InRadar
Depends on:
Blocks: webglangle
  Show dependency treegraph
 
Reported: 2019-08-19 16:15 PDT by Kenneth Russell
Modified: 2019-08-28 13:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch (15.58 KB, patch)
2019-08-23 14:55 PDT, Austin Eng
no flags Details | Formatted Diff | Diff
Patch (15.41 KB, patch)
2019-08-26 14:24 PDT, Austin Eng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2019-08-19 16:15:44 PDT
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.
Comment 1 Kenneth Russell 2019-08-19 16:19:43 PDT
Austin, may I ask you to take this bug?
Comment 2 Austin Eng 2019-08-19 18:19:23 PDT
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(...);
Comment 3 Austin Eng 2019-08-20 09:59:07 PDT
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.
Comment 4 Austin Eng 2019-08-23 14:55:45 PDT
Created attachment 377156 [details]
Patch
Comment 5 Kenneth Russell 2019-08-23 16:12:39 PDT
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.
Comment 6 Austin Eng 2019-08-23 16:33:10 PDT
(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 7 Kenneth Russell 2019-08-23 17:32:11 PDT
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 8 Alex Christensen 2019-08-26 10:00:07 PDT
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
Comment 9 Austin Eng 2019-08-26 14:24:22 PDT
Created attachment 377269 [details]
Patch
Comment 10 Austin Eng 2019-08-26 14:26:51 PDT
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.
Comment 11 Austin Eng 2019-08-28 13:05:07 PDT
Can this have another review? Thanks.
Comment 12 WebKit Commit Bot 2019-08-28 13:34:25 PDT
Comment on attachment 377269 [details]
Patch

Clearing flags on attachment: 377269

Committed r249218: <https://trac.webkit.org/changeset/249218>
Comment 13 WebKit Commit Bot 2019-08-28 13:34:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-08-28 13:38:19 PDT
<rdar://problem/54805832>