Bug 231468 – Query EGL_BIND_TO_TEXTURE_TARGET_ANGLE to determine 2D/rectangular texture usage

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

[Kenneth Russell]

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.

[Kyle Piddington]

Taking this one into my queue :)

[Kyle Piddington]

Created attachment 441265 [details]
Patch

[Kenneth Russell]

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.

[Kyle Piddington]

Created attachment 441412 [details]
Patch

[Kenneth Russell]

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.

[Radar WebKit Bug Importer]

<rdar://problem/84324684>

[Kyle Piddington]

Created attachment 441660 [details]
Patch

[Kenneth Russell]

Comment on attachment 441660 [details]
Patch

Still looks good. r+ again

[Kyle Piddington]

Created attachment 441693 [details]
Patch for landing

[EWS]

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