Bug 231468

Summary: Query EGL_BIND_TO_TEXTURE_TARGET_ANGLE to determine 2D/rectangular texture usage
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, gman, kbr, kkinnunen, kondapallykalyan, kpiddington, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

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]
Patch
Comment 3 Kenneth Russell 2021-10-14 15:24:14 PDT
Comment on attachment 441265 [details]
Patch

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]
Patch
Comment 5 Kenneth Russell 2021-10-15 16:45:33 PDT
Comment on attachment 441412 [details]
Patch

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
<rdar://problem/84324684>
Comment 7 Kyle Piddington 2021-10-18 17:11:07 PDT
Created attachment 441660 [details]
Patch
Comment 8 Kenneth Russell 2021-10-18 17:19:36 PDT
Comment on attachment 441660 [details]
Patch

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