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

Description Kimmo Kinnunen 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;
Comment 1 Radar WebKit Bug Importer 2020-10-02 04:16:51 PDT
<rdar://problem/69876022>
Comment 2 Kimmo Kinnunen 2020-10-12 07:47:47 PDT
Created attachment 411113 [details]
Patch
Comment 3 Sam Weinig 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.
Comment 4 Kimmo Kinnunen 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.
Comment 5 Kimmo Kinnunen 2020-10-12 10:23:22 PDT
Created attachment 411127 [details]
Patch
Comment 6 Sam Weinig 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.
Comment 7 Kimmo Kinnunen 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 ?
Comment 8 Sam Weinig 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 :(.
Comment 9 Kimmo Kinnunen 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 ?
Comment 10 Sam Weinig 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.
Comment 11 Kimmo Kinnunen 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.
Comment 12 Dean Jackson 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.
Comment 13 Kimmo Kinnunen 2020-10-12 22:43:34 PDT
Created attachment 411198 [details]
Patch
Comment 14 Kimmo Kinnunen 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.
Comment 15 EWS 2020-10-12 23:15:18 PDT
kkinnunen@apple.com does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 411198 [details] from commit queue.
Comment 16 EWS 2020-10-12 23:20:05 PDT
kkinnunen@apple.com does not have reviewer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 411198 [details] from commit queue.
Comment 17 EWS 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].
Comment 18 Kenneth Russell 2020-10-22 17:15:16 PDT
*** Bug 218100 has been marked as a duplicate of this bug. ***
Comment 19 Kenneth Russell 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?
Comment 20 Kimmo Kinnunen 2020-10-23 21:04:05 PDT
*** Bug 218100 has been marked as a duplicate of this bug. ***
Comment 21 Kenneth Russell 2020-12-22 14:19:40 PST
*** Bug 219780 has been marked as a duplicate of this bug. ***
Comment 22 reon90 2021-01-20 07:22:45 PST
Could you answer what version of Safari contains such fix?