WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
217212
Cocoa: Make WebGLLayer not dependent on GraphicsContextGLOpenGL
https://bugs.webkit.org/show_bug.cgi?id=217212
Summary
Cocoa: Make WebGLLayer not dependent on GraphicsContextGLOpenGL
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
Details
Formatted Diff
Diff
Patch
(40.92 KB, patch)
2020-10-12 10:23 PDT
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
Patch
(40.91 KB, patch)
2020-10-12 22:43 PDT
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-10-02 04:16:51 PDT
<
rdar://problem/69876022
>
Kimmo Kinnunen
Comment 2
2020-10-12 07:47:47 PDT
Created
attachment 411113
[details]
Patch
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
Created
attachment 411127
[details]
Patch
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
Created
attachment 411198
[details]
Patch
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
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.
EWS
Comment 16
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.
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.
Top of Page
Format For Printing
XML
Clone This Bug