Bug 231468 - Query EGL_BIND_TO_TEXTURE_TARGET_ANGLE to determine 2D/rectangular texture usage
Summary: Query EGL_BIND_TO_TEXTURE_TARGET_ANGLE to determine 2D/rectangular texture usage
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
Keywords: InRadar
Depends on:
Reported: 2021-10-08 18:21 PDT by Kenneth Russell
Modified: 2021-10-19 01:06 PDT (History)
8 users (show)

See Also:

Patch (12.72 KB, patch)
2021-10-14 13:07 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2021-10-15 12:08 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.30 KB, patch)
2021-10-18 17:11 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (10.30 KB, patch)
2021-10-19 00:09 PDT, Kyle Piddington
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 2021-10-08 18:21:12 PDT
WebKit should query EGL_BIND_TO_TEXTURE_TARGET_ANGLE via eglGetConfigAttrib in order to determine whether to bind its IOSurfaces to 2D or rectangular textures, instead of hardcoding platform knowledge in GraphicsContextGLOpenGL::drawingBufferTextureTarget, GraphicsContextGLOpenGL::drawingBufferTextureTargetQuery, createPbufferAndAttachIOSurface, and other places.

This will allow ANGLE's Metal backend to drop support for binding IOSurfaces to rectangular textures. OpenGL on macOS only supported the rectangular texture path, but Metal allows IOSurfaces to be bound to ordinary 2D textures.

The support for rectangular textures that was added to ANGLE's Metal backend in WebKit's fork was incomplete, and broke a lot of unit and end-to-end tests. It's been removed upstream in ANGLE. Rolling upstream ANGLE back into WebKit is predicated on removing usage of rectangular textures with the Metal backend.
Comment 1 Kyle Piddington 2021-10-14 11:00:35 PDT
Taking this one into my queue :)
Comment 2 Kyle Piddington 2021-10-14 13:07:23 PDT
Created attachment 441265 [details]
Comment 3 Kenneth Russell 2021-10-14 15:24:14 PDT
Comment on attachment 441265 [details]

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

Good work overall. Please rebase and let's see green EWS runs before approving.

> Source/WebCore/ChangeLog:6
> +	Refactor GrraphicsContextGLCocoa to hold a reference to 

Grr -> Gr :)

But: GraphicsContextGLOpenGLCocoa

Also, please indent to match.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:473
> +        EGL_GetConfigAttrib(m_displayObj, m_configObj, EGL_BIND_TO_TEXTURE_TARGET_ANGLE, &m_drawingBufferTextureTarget);

Are we guaranteed to only call this when m_displayObj and m_configObj are valid? If there's any confusion then let's consider doing this upon display initialization and have any asserts possible about not calling this on a destroyed GraphicsContextGLOpenGL.
Comment 4 Kyle Piddington 2021-10-15 12:08:30 PDT
Created attachment 441412 [details]
Comment 5 Kenneth Russell 2021-10-15 16:45:33 PDT
Comment on attachment 441412 [details]

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

Looks good to me. Couple of small comments about the asserts. r+

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:471
> +    ASSERT(!"Invalid enum returned from EGL_GetConfigAttrib");

This is the first I've seen this trick used to incorporate a message. Consider ASSERT_WITH_MESSAGE(false, ...).

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:484
> +    ASSERT(!"Invalid drawing target");

Same here.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:497
> +    assert(!"Invalid drawing target");

Please use WebKit's ASSERT macro.
Comment 6 Radar WebKit Bug Importer 2021-10-15 18:22:15 PDT
Comment 7 Kyle Piddington 2021-10-18 17:11:07 PDT
Created attachment 441660 [details]
Comment 8 Kenneth Russell 2021-10-18 17:19:36 PDT
Comment on attachment 441660 [details]

Still looks good. r+ again
Comment 9 Kyle Piddington 2021-10-19 00:09:02 PDT
Created attachment 441693 [details]
Patch for landing
Comment 10 EWS 2021-10-19 01:06:51 PDT
Committed r284438 (243200@main): <https://commits.webkit.org/243200@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441693 [details].