Bug 217212

Summary: Cocoa: Make WebGLLayer not dependent on GraphicsContextGLOpenGL
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebGLAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, kbr, koivisto, kondapallykalyan, msatbentley, noam, reon90, sam, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: macOS 10.15   
See Also: https://bugs.webkit.org/show_bug.cgi?id=229606
Bug Depends on:    
Bug Blocks: 217211, 217581, 217213, 217697, 218100, 218177    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kimmo Kinnunen
Reported 2020-10-02 04:16:34 PDT
Cocoa: Make WebGLLayer surface swap polymorphic to GraphicsContextGL This is needed so remote WebGL can work Currently - (void)prepareForDisplay { if (!_context) return; // To avoid running any OpenGL code in `display`, this method should be called // at the end of the rendering task. We will flush all painting commands // leaving the buffers ready to composite. #if USE(OPENGL) _context->prepareTexture(); if (_drawingBuffer) { std::swap(_contentsBuffer, _drawingBuffer); [self bindFramebufferToNextAvailableSurface]; } #elif USE(ANGLE) if (!_context->makeContextCurrent()) { // Context is likely being torn down. return; } _context->prepareTexture(); if (_drawingBuffer) { if (_latchedPbuffer) { WTF::Optional<ScopedRestoreTextureBinding> restoreBinding; // We don't need to restore GL_TEXTURE_RECTANGLE because it's not accessible from user code. if (WebCore::GraphicsContextGL::IOSurfaceTextureTarget() != WebCore::GraphicsContextGL::TEXTURE_RECTANGLE_ARB) restoreBinding.emplace(WebCore::GraphicsContextGL::IOSurfaceTextureTargetQuery(), WebCore::GraphicsContextGL::IOSurfaceTextureTarget()); GCGLenum texture = _context->platformTexture(); gl::BindTexture(WebCore::GraphicsContextGL::IOSurfaceTextureTarget(), texture); if (!EGL_ReleaseTexImage(_eglDisplay, _latchedPbuffer, EGL_BACK_BUFFER)) { // FIXME: report error. notImplemented(); } _latchedPbuffer = nullptr; } std::swap(_contentsBuffer, _drawingBuffer); std::swap(_contentsPbuffer, _drawingPbuffer); [self bindFramebufferToNextAvailableSurface]; } #endif [self setNeedsDisplay]; _preparedForDisplay = YES;
Attachments
Patch (40.73 KB, patch)
2020-10-12 07:47 PDT, Kimmo Kinnunen
no flags
Patch (40.92 KB, patch)
2020-10-12 10:23 PDT, Kimmo Kinnunen
no flags
Patch (40.91 KB, patch)
2020-10-12 22:43 PDT, Kimmo Kinnunen
no flags
Radar WebKit Bug Importer
Comment 1 2020-10-02 04:16:51 PDT
Kimmo Kinnunen
Comment 2 2020-10-12 07:47:47 PDT
Sam Weinig
Comment 3 2020-10-12 09:32:03 PDT
Comment on attachment 411113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411113&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:91 > +#if PLATFORM(COCOA) If this is really not universal, please use a more specific define. Please try to keep PLATFORM usage to a minimum.
Kimmo Kinnunen
Comment 4 2020-10-12 09:35:04 PDT
Comment on attachment 411113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411113&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:91 >> +#if PLATFORM(COCOA) > > If this is really not universal, please use a more specific define. Please try to keep PLATFORM usage to a minimum. It's universal to COCOA. This patch is about implementation of Cocoa-specific GraphicsContextGLOpenGL. Unfortunately the setup is what it is before this patch, and this patch does not yet address the fundamental problem with the ifdefs. This patch does work towards eventually being able to remove the ifdefs.
Kimmo Kinnunen
Comment 5 2020-10-12 10:23:22 PDT
Sam Weinig
Comment 6 2020-10-12 10:27:21 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 > +#if PLATFORM(COCOA) > + , private WebGLLayerClient > +#endif Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here.
Kimmo Kinnunen
Comment 7 2020-10-12 10:41:16 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >> +#endif > > Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ?
Sam Weinig
Comment 8 2020-10-12 11:07:19 PDT
(In reply to Kimmo Kinnunen from comment #7) > Comment on attachment 411127 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411127&action=review > > >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 > >> +#endif > > > > Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. > > Would you be able to suggest a define that would be more appropriate and > still work with other code defined inside PLATFORM(COCOA) ifdefs and be > implemented in the file GraphicsContextGLOpenGLCocoa.mm ? Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(.
Kimmo Kinnunen
Comment 9 2020-10-12 11:32:27 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>> +#endif >>> >>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >> >> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? > > Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ?
Sam Weinig
Comment 10 2020-10-12 11:38:09 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>> +#endif >>>> >>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>> >>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >> >> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. > > But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too.
Kimmo Kinnunen
Comment 11 2020-10-12 11:41:18 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>>> +#endif >>>>> >>>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>>> >>>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >>> >>> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. >> >> But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? > > Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too. Thanks. I'll keep the COCOA in this patch. Renaming COCOA->CA should be done in a different patch, targeting specifically that goal.
Dean Jackson
Comment 12 2020-10-12 14:02:38 PDT
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review > Source/WebCore/ChangeLog:18 > + Move the back buffer ownership to the GraphicsContextGLOpenGL. Nice. > Source/WebCore/ChangeLog:21 > + Make the front buffer ownership explicit in WebGLLayer. > + Move the EGL bindings ownerships of all buffers to > + GraphicsContextGLOpenGL. Nicer! > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:34 > + void* handle { nullptr }; // Client specific metadata handle (such as EGLSurface). Will a WebGLLayer that is getting remote buffers need a handle? If not, it might be ok if this is specific to EGL. > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:63 > +- (WebCore::WebGLLayerBuffer) recycleBuffer; Nit: We don't put a space between the return type and the name. >>>>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>>>> +#endif >>>>>> >>>>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>>>> >>>>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >>>> >>>> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. >>> >>> But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? >> >> Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too. > > Thanks. I'll keep the COCOA in this patch. Renaming COCOA->CA should be done in a different patch, targeting specifically that goal. I think it is ok to leave the files named Cocoa. I wonder if we could make WebGLLayerClient an unconditional inheritance though, and have an empty implementation for other ports.
Kimmo Kinnunen
Comment 13 2020-10-12 22:43:34 PDT
Kimmo Kinnunen
Comment 14 2020-10-12 22:50:20 PDT
> I wonder if we could make WebGLLayerClient an unconditional inheritance though, and have an empty implementation for other ports. I don't think the ifdef is a special. There are multiple as severe existing ifdefs, they don't just stand out because mental adaptation to having ifdefs for member functions but not for base classes. Adding empty WebGLLayerClient for other ports makes little sense, since it only rises questions of "why do we have WebGLLayerClient when we don't even have a thing called WebGLLayer nor we make this object a client of anything". The solution to the ifdef problem is to work on the inheritance tree so that the cocoa specific code can have normal final cocoa-specific class where the client inheritance is business as usual. This is a preliminary work to that.
EWS
Comment 15 2020-10-12 23:15:18 PDT
EWS
Comment 16 2020-10-12 23:20:05 PDT
EWS
Comment 17 2020-10-13 00:17:02 PDT
Committed r268386: <https://trac.webkit.org/changeset/268386> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411198 [details].
Kenneth Russell
Comment 18 2020-10-22 17:15:16 PDT
*** Bug 218100 has been marked as a duplicate of this bug. ***
Kenneth Russell
Comment 19 2020-10-23 18:28:07 PDT
Great fix for this Kimmo! This fixes real resource leaks that have been identified in Bug 218100 and Bug 217581. Could this please be merged back to the Safari 14 release branch?
Kimmo Kinnunen
Comment 20 2020-10-23 21:04:05 PDT
*** Bug 218100 has been marked as a duplicate of this bug. ***
Kenneth Russell
Comment 21 2020-12-22 14:19:40 PST
*** Bug 219780 has been marked as a duplicate of this bug. ***
reon90
Comment 22 2021-01-20 07:22:45 PST
Could you answer what version of Safari contains such fix?
Note You need to log in before you can comment on or make changes to this bug.