Bug 200626

Summary: [GL][GStreamer] Instantiate GstGLContext when the shared GLContext is created
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: MediaAssignee: Víctor M. Jáquez L. <vjaquez>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, calvaris, cgarcia, commit-queue, cturner, dino, eric.carlson, ews-watchlist, glenn, gustavo, gyuyoung.kim, jer.noble, mcatanzaro, menard, philipj, pnormand, ryuan.choi, sam, sergio, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Víctor M. Jáquez L.
Reported 2019-08-12 05:57:37 PDT
Currently one GstGLContext is created per video tag, and (as in https://bugs.webkit.org/show_bug.cgi?id=196966 ) it has to make current the shared context, which may add processing. A better approach would be to create one GstGLContext when the GLContext is created and current, and when a video media element created, the created GstGLContext increases its reference count and passed to the GStreamer pipeline.
Attachments
WIP Patch (13.20 KB, patch)
2020-02-04 07:59 PST, Víctor M. Jáquez L.
no flags
Patch (13.88 KB, patch)
2020-02-05 04:41 PST, Víctor M. Jáquez L.
no flags
Patch (13.90 KB, patch)
2020-02-05 04:46 PST, Víctor M. Jáquez L.
no flags
Patch (14.70 KB, patch)
2020-02-11 09:56 PST, Víctor M. Jáquez L.
no flags
Patch (21.79 KB, patch)
2020-02-18 08:03 PST, Víctor M. Jáquez L.
no flags
Patch (21.69 KB, patch)
2020-02-18 09:08 PST, Víctor M. Jáquez L.
no flags
Patch (19.90 KB, patch)
2020-02-24 07:25 PST, Víctor M. Jáquez L.
no flags
Patch (19.38 KB, patch)
2020-02-25 08:58 PST, Víctor M. Jáquez L.
no flags
Patch (19.38 KB, patch)
2020-02-26 03:30 PST, Víctor M. Jáquez L.
no flags
Philippe Normand
Comment 1 2019-11-26 10:05:01 PST
The GLVideoSink added in Bug 204624 could have a "context" property, similarly to glimagesink. The player could set that property.
Víctor M. Jáquez L.
Comment 2 2020-02-04 07:59:05 PST
Created attachment 389659 [details] WIP Patch
Philippe Normand
Comment 3 2020-02-04 08:11:51 PST
Comment on attachment 389659 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389659&action=review > Source/WebCore/ChangeLog:11 > + keep the wrapper, so it won't need to be recreated every time is every time *it* is > Source/WebCore/ChangeLog:16 > + instantianted and associated to the GL context when probing the instantiated > Source/WebCore/ChangeLog:21 > + Finally, the wrapper class is retrieved through GLContext and > + retrieve the GstGLDisplay and the GstGLContext associated to store > + them as private variables for the video sink. This could be reworded to avoid too many "and" and "retrieve". > Source/WebCore/ChangeLog:36 > + implemenation for WrappedGLContext. implementation > Source/WebCore/ChangeLog:37 > + (webKitGLVideoSinkProbePlatform): instianciates instantiates > Source/WebCore/ChangeLog:38 > + GstWarpperGLContext given the platform's shared context. GstWrappedGLContext > Source/WebCore/platform/graphics/GLContext.h:91 > + WrappedGLContext *wrappedGLContext() const { return m_wrappedGlContext.get(); } Could this return a WrappedGLContext& instead of a pointer? > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:66 > + GRefPtr<GstGLContext> Context() { return m_glContext.get(); } > + GRefPtr<GstGLDisplay> Display() { return m_glDisplay.get(); } Method names should start with a lowercase letter. Seems like these two should be const as well?
Carlos Garcia Campos
Comment 4 2020-02-05 00:08:34 PST
Comment on attachment 389659 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389659&action=review > Source/WebCore/ChangeLog:22 > + This explains how, but I miss a why. What is this fixing? > Source/WebCore/platform/graphics/GLContext.h:90 > + void setWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; I'm not sure what wrapped actually means here. Is it like a sub-context? or associated context? It seems we are setting it here only to sync the lifetime of the gst gl context to the GLContext one. Maybe an observer approach makes more sense? >> Source/WebCore/platform/graphics/GLContext.h:91 >> + WrappedGLContext *wrappedGLContext() const { return m_wrappedGlContext.get(); } > > Could this return a WrappedGLContext& instead of a pointer? I guess it can be nullptr if called before setWrappedGLContext. WrappedGLContext * -> WrappedGLContext* > Source/WebCore/platform/graphics/GLContext.h:105 > + std::unique_ptr<WrappedGLContext> m_wrappedGlContext { nullptr }; std::unique_ptr already initializes the pointer to nullptr on construction. >> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:66 >> + GRefPtr<GstGLDisplay> Display() { return m_glDisplay.get(); } > > Method names should start with a lowercase letter. Seems like these two should be const as well? Do we really want to return GRefPtr? In that case we don't need the .get() and it can probably return a ref instead of a copy. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:162 > GLContext* sharedContext = sharedDisplay.sharingGLContext(); It seems sharedDisplay is only used here now, we can just GLContext* sharedContext = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext(); > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:169 > + auto wrappedGLContext = sharedContext->wrappedGLContext(); auto* > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:379 > + assert(contextHandle); ASSERT > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:412 > + if (!sharedContext->wrappedGLContext()) { > + std::unique_ptr<GLContext::WrappedGLContext> wrappedGlContext = makeUnique<GstWrappedGLContext>(sharedDisplay, sharedContext); > + sharedContext->setWrappedGLContext(WTFMove(wrappedGlContext)); > + } Shouldn't we do this only if isGStreamerPluginAvailable("opengl")? You can do: sharedContext->setWrappedGLContext(makeUnique<GstWrappedGLContext>(sharedDisplay, sharedContext));
Víctor M. Jáquez L.
Comment 5 2020-02-05 04:41:16 PST
Víctor M. Jáquez L.
Comment 6 2020-02-05 04:46:12 PST
Víctor M. Jáquez L.
Comment 7 2020-02-05 04:47:07 PST
(In reply to Philippe Normand from comment #3) > Comment on attachment 389659 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389659&action=review > > > Source/WebCore/ChangeLog:11 > > + keep the wrapper, so it won't need to be recreated every time is > > every time *it* is Done > > > Source/WebCore/ChangeLog:16 > > + instantianted and associated to the GL context when probing the > > instantiated Done > > > Source/WebCore/ChangeLog:21 > > + Finally, the wrapper class is retrieved through GLContext and > > + retrieve the GstGLDisplay and the GstGLContext associated to store > > + them as private variables for the video sink. > > This could be reworded to avoid too many "and" and "retrieve". Done. I hope it's better now. > > > Source/WebCore/ChangeLog:36 > > + implemenation for WrappedGLContext. > > implementation Done > > > Source/WebCore/ChangeLog:37 > > + (webKitGLVideoSinkProbePlatform): instianciates > > instantiates Done > > > Source/WebCore/ChangeLog:38 > > + GstWarpperGLContext given the platform's shared context. > > GstWrappedGLContext > > > Source/WebCore/platform/graphics/GLContext.h:91 > > + WrappedGLContext *wrappedGLContext() const { return m_wrappedGlContext.get(); } > > Could this return a WrappedGLContext& instead of a pointer? As Carlos said, it can be nullptr before setting it. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:66 > > + GRefPtr<GstGLContext> Context() { return m_glContext.get(); } > > + GRefPtr<GstGLDisplay> Display() { return m_glDisplay.get(); } > > Method names should start with a lowercase letter. Seems like these two > should be const as well? Done
Víctor M. Jáquez L.
Comment 8 2020-02-05 04:52:45 PST
(In reply to Carlos Garcia Campos from comment #4) > Comment on attachment 389659 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389659&action=review > > > Source/WebCore/ChangeLog:22 > > + > > This explains how, but I miss a why. What is this fixing? Done. > > > Source/WebCore/platform/graphics/GLContext.h:90 > > + void setWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; > > I'm not sure what wrapped actually means here. Is it like a sub-context? or > associated context? It seems we are setting it here only to sync the > lifetime of the gst gl context to the GLContext one. Maybe an observer > approach makes more sense? GstGLContext can be created from an existing GLContext, which is what we do. Thus, it can be viewed as a wrapper. And yes, the purpose is to sync the life time of the GstGLContext with GLContext, to avoid creating a GstGLContext every time a new video is rendered. AFAIU it is like already as an observer pattern. > > >> Source/WebCore/platform/graphics/GLContext.h:91 > >> + WrappedGLContext *wrappedGLContext() const { return m_wrappedGlContext.get(); } > > > > Could this return a WrappedGLContext& instead of a pointer? > > I guess it can be nullptr if called before setWrappedGLContext. > > WrappedGLContext * -> WrappedGLContext* > > > Source/WebCore/platform/graphics/GLContext.h:105 > > + std::unique_ptr<WrappedGLContext> m_wrappedGlContext { nullptr }; > > std::unique_ptr already initializes the pointer to nullptr on construction. Done > > >> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:66 > >> + GRefPtr<GstGLDisplay> Display() { return m_glDisplay.get(); } > > > > Method names should start with a lowercase letter. Seems like these two should be const as well? Done > > Do we really want to return GRefPtr? In that case we don't need the .get() > and it can probably return a ref instead of a copy. I have a hard time thinking in terms of GRefPtr. I hope it is correct now. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:162 > > GLContext* sharedContext = sharedDisplay.sharingGLContext(); > > It seems sharedDisplay is only used here now, we can just GLContext* > sharedContext = > PlatformDisplay::sharedDisplayForCompositing().sharingGLContext(); Nope, the display is also needed :) > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:169 > > + auto wrappedGLContext = sharedContext->wrappedGLContext(); > > auto* Done > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:379 > > + assert(contextHandle); > > ASSERT Done > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:412 > > + if (!sharedContext->wrappedGLContext()) { > > + std::unique_ptr<GLContext::WrappedGLContext> wrappedGlContext = makeUnique<GstWrappedGLContext>(sharedDisplay, sharedContext); > > + sharedContext->setWrappedGLContext(WTFMove(wrappedGlContext)); > > + } > > Shouldn't we do this only if isGStreamerPluginAvailable("opengl")? Done > > You can do: > > sharedContext- > >setWrappedGLContext(makeUnique<GstWrappedGLContext>(sharedDisplay, > sharedContext)); Done Thanks!
Charlie Turner
Comment 9 2020-02-10 10:55:17 PST
Comment on attachment 389794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389794&action=review Nice fix Victor! > Source/WebCore/platform/graphics/GLContext.h:90 > + void setWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; Nit, it seems more natural to have setWrappedGLContext take the wrappedContext by && here. If you change the callsite to something that isn't an rv, it will error out on unique_ptr(unique_ptr&) being deleted. Also makes clear it is taking ownership. Note that currently you get away with the by-val parameter due to implicit move construction. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:-157 > - if (is<PlatformDisplayX11>(sharedDisplay)) { It looks weird to me that PlatformDisplayX11, PlatformDisplayWayland, etc don't all have a polymorphic getDisplay() that returns the appropriate glDisplay value. Don't want to cause another iteration for this, but maybe a FIXME if you agree and I haven't just misunderstood the wider context of this code. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:171 > + GST_ELEMENT_ERROR(GST_ELEMENT_CAST(sink), RESOURCE, NOT_FOUND, (("WebKit shared GL wrapperd context unavailable")), s/wrapperd/wrapped/ > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:172 > + ("The WebKit shared GL wrapped context somehow disappeared. Video textures rendering will fail.")); Video *texture* rendering will fail.
Víctor M. Jáquez L.
Comment 10 2020-02-11 09:56:03 PST
Víctor M. Jáquez L.
Comment 11 2020-02-11 10:06:29 PST
(In reply to Charlie Turner from comment #9) > Comment on attachment 389794 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389794&action=review > > Nice fix Victor! Thanks! > > > Source/WebCore/platform/graphics/GLContext.h:90 > > + void setWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; > > Nit, it seems more natural to have setWrappedGLContext take the > wrappedContext by && here. If you change the callsite to something that > isn't an rv, it will error out on unique_ptr(unique_ptr&) being deleted. > Also makes clear it is taking ownership. Note that currently you get away > with the by-val parameter due to implicit move construction. As not always a wrapper is going to be attached to the GLContext we need something that can be nullptr, in other words a pointer, not a reference :( > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:-157 > > - if (is<PlatformDisplayX11>(sharedDisplay)) { > > It looks weird to me that PlatformDisplayX11, PlatformDisplayWayland, etc > don't all have a polymorphic getDisplay() that returns the appropriate > glDisplay value. Don't want to cause another iteration for this, but maybe a > FIXME if you agree and I haven't just misunderstood the wider context of > this code. Yeah, that belongs to another issue :) > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:171 > > + GST_ELEMENT_ERROR(GST_ELEMENT_CAST(sink), RESOURCE, NOT_FOUND, (("WebKit shared GL wrapperd context unavailable")), > > s/wrapperd/wrapped/ Done. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:172 > > + ("The WebKit shared GL wrapped context somehow disappeared. Video textures rendering will fail.")); > > Video *texture* rendering will fail. Done. Also renamed setWrapperGLContext to addWrapperGLContext to follow the observer kind of API and also added a (currently noop) didDestroyContext called at GLContext destructor (before the leaf classes destructors)
Carlos Garcia Campos
Comment 12 2020-02-12 01:36:48 PST
Comment on attachment 390380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390380&action=review > Source/WebCore/platform/graphics/GLContext.cpp:139 > + if (m_wrappedGlContext.get()) > + m_wrappedGlContext->didDestroyGLContext(); The idea of using this observer pattern was to avoid making the GLContext handle the lifetime of the gst context. The observer gets notified that the GL context is destroyed and it destroys the gst context. > Source/WebCore/platform/graphics/GLContext.h:92 > + void addWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; > + WrappedGLContext* wrappedGLContext() const { return m_wrappedGlContext.get(); } So, this makes it possible to have a wrapped context for every GLContext, but what we really want here is a sharingGstGLContext() right? In that case, maybe it's easier to add PlatformDisplay::sharingGstGLContext() > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:65 > + void didDestroyGLContext() override { }; This is pointless if the impl does nothing. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:70 > + GRefPtr<GstGLDisplay> m_glDisplay; If we move it to PlastrformDisplay we would also need to add ::sharingGstGLDisplay().
Víctor M. Jáquez L.
Comment 13 2020-02-12 02:23:19 PST
Hi Carlos, Thanks for you review :) (In reply to Carlos Garcia Campos from comment #12) > Comment on attachment 390380 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390380&action=review > > > Source/WebCore/platform/graphics/GLContext.cpp:139 > > + if (m_wrappedGlContext.get()) > > + m_wrappedGlContext->didDestroyGLContext(); > > The idea of using this observer pattern was to avoid making the GLContext > handle the lifetime of the gst context. The observer gets notified that the > GL context is destroyed and it destroys the gst context. But the purpose of the patch is to delegate gst context lifetime to GLContext, since it is a wrapper of it. If the gl context is destroyed, then the gst context one has to be destroyed. > > > Source/WebCore/platform/graphics/GLContext.h:92 > > + void addWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; > > + WrappedGLContext* wrappedGLContext() const { return m_wrappedGlContext.get(); } > > So, this makes it possible to have a wrapped context for every GLContext, > but what we really want here is a sharingGstGLContext() right? In that case, > maybe it's easier to add PlatformDisplay::sharingGstGLContext() That wouldn't be an API breakage, adding a very specific context implementation?? That sharingGstGLContext has to be created from a sharingGLContext. Perhaps this need a better explanation: GStreamer GL creates its own internal GL context used to hold its own textures, framebuffers, etc. If we want to share these objects with other GL context (the one used by the application to render stuff) we have to tell GStreamerGL that its internal GL Context that its objects are going to be shared with another GL context. In order to pass this application GL Context with GStreamerGL it is required to wrap it as a GstGLContext, and its lifetime is handled by the GL Context itself. Right now, a GstGLContext is created every time a video tag is instantiated. This is expensive and requires the GL Context to be current in order to fill GstGLContext internal structure. And it is destroyed when the video tag is destroyed. This is not efficient. It's preferable to have a singleton GstGLContext, which is a Wrapper of the application GL Context, and reuse it for all the video tags. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:65 > > + void didDestroyGLContext() override { }; > > This is pointless if the impl does nothing. Indeed. Sorry. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:70 > > + GRefPtr<GstGLDisplay> m_glDisplay; > > If we move it to PlastrformDisplay we would also need to add > ::sharingGstGLDisplay(). I'm not sure to follow this comment.
Víctor M. Jáquez L.
Comment 14 2020-02-12 02:57:58 PST
By the way, instead of using std::unique_ptr I'm going to explore using std::optional, as in Rust :D
Carlos Garcia Campos
Comment 15 2020-02-12 04:31:57 PST
(In reply to Víctor M. Jáquez L. from comment #13) > Hi Carlos, > > Thanks for you review :) > > (In reply to Carlos Garcia Campos from comment #12) > > Comment on attachment 390380 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=390380&action=review > > > > > Source/WebCore/platform/graphics/GLContext.cpp:139 > > > + if (m_wrappedGlContext.get()) > > > + m_wrappedGlContext->didDestroyGLContext(); > > > > The idea of using this observer pattern was to avoid making the GLContext > > handle the lifetime of the gst context. The observer gets notified that the > > GL context is destroyed and it destroys the gst context. > > But the purpose of the patch is to delegate gst context lifetime to > GLContext, since it is a wrapper of it. If the gl context is destroyed, then > the gst context one has to be destroyed. PlatformDisplay is the owner of the sharing GL context, so it can be the owner of the GstGLContext as well. > > > > > Source/WebCore/platform/graphics/GLContext.h:92 > > > + void addWrappedGLContext(std::unique_ptr<WrappedGLContext> wrappedContext) { m_wrappedGlContext = WTFMove(wrappedContext); }; > > > + WrappedGLContext* wrappedGLContext() const { return m_wrappedGlContext.get(); } > > > > So, this makes it possible to have a wrapped context for every GLContext, > > but what we really want here is a sharingGstGLContext() right? In that case, > > maybe it's easier to add PlatformDisplay::sharingGstGLContext() > > That wouldn't be an API breakage, adding a very specific context > implementation?? I don't see why. There's also eglDisplay handled by PlatformDisplay. > That sharingGstGLContext has to be created from a sharingGLContext. > > Perhaps this need a better explanation: > > GStreamer GL creates its own internal GL context used to hold its own > textures, framebuffers, etc. If we want to share these objects with other GL > context (the one used by the application to render stuff) we have to tell > GStreamerGL that its internal GL Context that its objects are going to be > shared with another GL context. > > In order to pass this application GL Context with GStreamerGL it is required > to wrap it as a GstGLContext, and its lifetime is handled by the GL Context > itself. > > Right now, a GstGLContext is created every time a video tag is instantiated. > This is expensive and requires the GL Context to be current in order to fill > GstGLContext internal structure. And it is destroyed when the video tag is > destroyed. > > This is not efficient. It's preferable to have a singleton GstGLContext, > which is a Wrapper of the application GL Context, and reuse it for all the > video tags. I understand, that's why I think PlatformDisplay would be a good plance, it already provides the singleton we want now. > > > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:65 > > > + void didDestroyGLContext() override { }; > > > > This is pointless if the impl does nothing. > > Indeed. Sorry. > > > > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:70 > > > + GRefPtr<GstGLDisplay> m_glDisplay; > > > > If we move it to PlastrformDisplay we would also need to add > > ::sharingGstGLDisplay(). > > I'm not sure to follow this comment. We would add PlatformDisplay::sharingGstGLContext() and PlatformDisplay::sharingGstGLDisplay()
Víctor M. Jáquez L.
Comment 16 2020-02-18 08:03:28 PST
Víctor M. Jáquez L.
Comment 17 2020-02-18 08:05:26 PST
Ready another round for review :) It's a complete overhaul of the previous patches following Carlos suggestions in https://bugs.webkit.org/show_bug.cgi?id=200626#c15 Now there's a new class GLContextGStreamer, created and handled by PlatformDisplay.
Víctor M. Jáquez L.
Comment 18 2020-02-18 09:08:00 PST
Carlos Garcia Campos
Comment 19 2020-02-19 01:39:43 PST
Comment on attachment 391054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391054&action=review > Source/WebCore/ChangeLog:23 > + GLContextGStreamer is class instantiated as singleton by > + PlatformDisplay. When the media player request for the > + GLContextGStreamer a GL context is created, if it wasn't before, > + and wraps it. The class has two getters, one for GstGLContext > + object, and other for GstGLDisplay. Both are used to configure the > + internal GStreamer GL context in the multimedia pipeline. This is indeed simpler, but I don't think we need another class, because PlatformDisplay is already a singleton, we can simploy add two new methods: PlatformDisplay::sharingGstGLContext() and PlatformDisplay::sharingGstGLDisplay() as I suggested before. > Source/WebCore/platform/graphics/PlatformDisplay.cpp:262 > +GLContextGStreamer* PlatformDisplay::glGstreamerContext() > +{ > + if (!m_gstGLContext) > + m_gstGLContext = GLContextGStreamer::create(); > + return m_gstGLContext.get(); > +} So, instead of this we could get direct getters for the gst context and display. We would need a private method to initialize the gst wrappers, something like tryEnsureGstGLContext(), called from both getters. > Source/WebCore/platform/graphics/PlatformDisplay.cpp:265 > + You should also destroy the gst wrappers in PlatformDisplay::terminateEGLDisplay(), before m_sharingGLContext = nullptr; > Source/WebCore/platform/graphics/PlatformDisplay.h:40 > +class GLDisplayGStreamer; This doesn't exist, right? > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.cpp:19 > + I would move this to a a file Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.cpp:64 > +GLContextGStreamer::GLContextGStreamer(PlatformDisplay& sharedDisplay, GLContext* sharedContext) And this would be bool PlatformDisplay::tryEnsureGstGLContext() > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.h:37 > + GRefPtr<GstGLContext> getContext() { return m_context; } > + GRefPtr<GstGLDisplay> getDisplay() { return m_display; } I think it's better to return plain pointers for these, the caller doesn't really need to take a reference. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:126 > + gst_context_set_gl_display(displayContext, glGstContext->getDisplay().get()); The display can be nullptr at this point, no? we returned early before if ensureGstGLContext() failed, but now glGstContext is never nullptr. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:136 > - gst_structure_set(structure, "context", GST_TYPE_GL_CONTEXT, priv->glContext.get(), nullptr); > + gst_structure_set(structure, "context", GST_TYPE_GL_CONTEXT, glGstContext->getContext().get(), nullptr); > #else > - gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, priv->glContext.get(), nullptr); > + gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, glGstContext->getContext().get(), nullptr); Same here for the context.
Víctor M. Jáquez L.
Comment 20 2020-02-19 02:05:54 PST
(In reply to Carlos Garcia Campos from comment #19) > Comment on attachment 391054 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391054&action=review > > > Source/WebCore/ChangeLog:23 > > + GLContextGStreamer is class instantiated as singleton by > > + PlatformDisplay. When the media player request for the > > + GLContextGStreamer a GL context is created, if it wasn't before, > > + and wraps it. The class has two getters, one for GstGLContext > > + object, and other for GstGLDisplay. Both are used to configure the > > + internal GStreamer GL context in the multimedia pipeline. > > This is indeed simpler, but I don't think we need another class, because > PlatformDisplay is already a singleton, we can simploy add two new methods: > PlatformDisplay::sharingGstGLContext() and > PlatformDisplay::sharingGstGLDisplay() as I suggested before. I tried that first but it looked bit more complex, since I wanted to keep glContextGStreamer class. But taking it out and implementing tryEnsureGstGLContext() instead, sounds like a simpler approach. > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:262 > > +GLContextGStreamer* PlatformDisplay::glGstreamerContext() > > +{ > > + if (!m_gstGLContext) > > + m_gstGLContext = GLContextGStreamer::create(); > > + return m_gstGLContext.get(); > > +} > > So, instead of this we could get direct getters for the gst context and > display. We would need a private method to initialize the gst wrappers, > something like tryEnsureGstGLContext(), called from both getters. Agree. > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:265 > > + > > You should also destroy the gst wrappers in > PlatformDisplay::terminateEGLDisplay(), before m_sharingGLContext = nullptr; Nice advice! > > > Source/WebCore/platform/graphics/PlatformDisplay.h:40 > > +class GLDisplayGStreamer; > > This doesn't exist, right? Oops... crumbs of the first iteration. > > > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.cpp:19 > > + > > I would move this to a a file > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp Ok > > > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.cpp:64 > > +GLContextGStreamer::GLContextGStreamer(PlatformDisplay& sharedDisplay, GLContext* sharedContext) > > And this would be bool PlatformDisplay::tryEnsureGstGLContext() > > > Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.h:37 > > + GRefPtr<GstGLContext> getContext() { return m_context; } > > + GRefPtr<GstGLDisplay> getDisplay() { return m_display; } > > I think it's better to return plain pointers for these, the caller doesn't > really need to take a reference. But we need to increase the reference count in order to pass them into GStreamer pipelines. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:126 > > + gst_context_set_gl_display(displayContext, glGstContext->getDisplay().get()); > > The display can be nullptr at this point, no? we returned early before if > ensureGstGLContext() failed, but now glGstContext is never nullptr. AFAIU, it cannot. If glGstContext is instantiated, it should have both gst gl objects. > > > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:136 > > - gst_structure_set(structure, "context", GST_TYPE_GL_CONTEXT, priv->glContext.get(), nullptr); > > + gst_structure_set(structure, "context", GST_TYPE_GL_CONTEXT, glGstContext->getContext().get(), nullptr); > > #else > > - gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, priv->glContext.get(), nullptr); > > + gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, glGstContext->getContext().get(), nullptr); > > Same here for the context.
Carlos Garcia Campos
Comment 21 2020-02-19 06:28:55 PST
Comment on attachment 391054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391054&action=review >>> Source/WebCore/platform/graphics/gstreamer/GLContextGStreamer.cpp:64 >>> +GLContextGStreamer::GLContextGStreamer(PlatformDisplay& sharedDisplay, GLContext* sharedContext) >> >> And this would be bool PlatformDisplay::tryEnsureGstGLContext() > > But we need to increase the reference count in order to pass them into GStreamer pipelines. Caller is free to use a GRefPtr >>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:126 >>> + gst_context_set_gl_display(displayContext, glGstContext->getDisplay().get()); >> >> The display can be nullptr at this point, no? we returned early before if ensureGstGLContext() failed, but now glGstContext is never nullptr. > > AFAIU, it cannot. If glGstContext is instantiated, it should have both gst gl objects. There's no null check of glGstContext. Anyway, this won't be a problem when having direct getters in PlatformDisplay
Víctor M. Jáquez L.
Comment 22 2020-02-24 07:25:11 PST
Víctor M. Jáquez L.
Comment 23 2020-02-24 07:28:26 PST
Another round ready :)
Carlos Garcia Campos
Comment 24 2020-02-25 04:40:26 PST
Comment on attachment 391535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391535&action=review This is the idea, only a few more details. Thanks! > Source/WebCore/platform/graphics/PlatformDisplay.cpp:156 > + destroyGstGLContext(); Use GRefPtr and you don't need this. > Source/WebCore/platform/graphics/PlatformDisplay.cpp:235 > + destroyGstGLContext(); And here you can just set both members to nullptr. > Source/WebCore/platform/graphics/PlatformDisplay.h:81 > + WEBCORE_EXPORT GstGLDisplay* sharingGstGLDisplay(); > + WEBCORE_EXPORT GstGLContext* sharingGstGLContext(); We don't use WEBCORE_EXPORT macros. I know I proposed these names before, but now I've realized the "sharing" is confusing here. The sharingGLContext is the one use to share GL resources like textures. The GstGL context is the one used by Gst for rendering, so I would remove the sharing prefix. gstGLDisplay() and gstGLContext(). I'm sorry I didn't realize before. I also think we can make these const by marking the members as mutable. > Source/WebCore/platform/graphics/PlatformDisplay.h:118 > + GstGLDisplay* m_gstGLDisplay; > + GstGLContext* m_gstGLContext; GRefPtr > Source/WebCore/platform/graphics/PlatformDisplay.h:120 > +#else > + void destroyGstGLContext() { }; You shouldn't need this, add #if ENABLE(VIDEO) && USE(GSTREAMER_GL) guards when accessing the members. > Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:124 > + GRefPtr<GstGLDisplay> sharedGLDisplay = sharedDisplay.sharingGstGLDisplay(); > + GRefPtr<GstGLContext> sharedGLContext = sharedDisplay.sharingGstGLContext(); Why do we need to take a reference here? Since diaply and context are not expected to be nullptr at this point, add ASSERTS to ensure it.
Víctor M. Jáquez L.
Comment 25 2020-02-25 07:38:25 PST
Thanks again, Carlos. I appreciate your time :) (In reply to Carlos Garcia Campos from comment #24) > Comment on attachment 391535 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391535&action=review > > This is the idea, only a few more details. Thanks! > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:156 > > + destroyGstGLContext(); > > Use GRefPtr and you don't need this. I thought it but I preferred to not to include GRefPtrGStreamer.h in platform/graphics. It didn't look like a good idea to mix it. Can you confirm me that there's nothing wrong on it?
Carlos Garcia Campos
Comment 26 2020-02-25 07:40:20 PST
(In reply to Víctor M. Jáquez L. from comment #25) > Thanks again, Carlos. I appreciate your time :) > > > (In reply to Carlos Garcia Campos from comment #24) > > Comment on attachment 391535 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=391535&action=review > > > > This is the idea, only a few more details. Thanks! > > > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:156 > > > + destroyGstGLContext(); > > > > Use GRefPtr and you don't need this. > > I thought it but I preferred to not to include GRefPtrGStreamer.h > in platform/graphics. It didn't look like a good idea to mix it. > > Can you confirm me that there's nothing wrong on it? There isn't any problem, it's all platform layer.
Víctor M. Jáquez L.
Comment 27 2020-02-25 08:58:25 PST
Carlos Garcia Campos
Comment 28 2020-02-26 00:44:27 PST
Comment on attachment 391657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391657&action=review > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:134 > + if (!const_cast<PlatformDisplay*>(this)->tryEnsureGstGLContext()) I had never seen this, I think I prefer to make tryEnsureGstGLContext() const and mark m_gstGLContext and m_gstGLDisplay as mutable, instead of a const cast of this. Unless this is better for whatever reason...
Víctor M. Jáquez L.
Comment 29 2020-02-26 02:01:07 PST
(In reply to Carlos Garcia Campos from comment #28) > Comment on attachment 391657 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391657&action=review > > > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:134 > > + if (!const_cast<PlatformDisplay*>(this)->tryEnsureGstGLContext()) > > I had never seen this, I think I prefer to make tryEnsureGstGLContext() > const and mark m_gstGLContext and m_gstGLDisplay as mutable, instead of a > const cast of this. Unless this is better for whatever reason... let me try that approach. I copied it from here https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/PlatformDisplay.cpp#L179 :)
Carlos Garcia Campos
Comment 30 2020-02-26 02:27:31 PST
(In reply to Víctor M. Jáquez L. from comment #29) > (In reply to Carlos Garcia Campos from comment #28) > > Comment on attachment 391657 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=391657&action=review > > > > > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:134 > > > + if (!const_cast<PlatformDisplay*>(this)->tryEnsureGstGLContext()) > > > > I had never seen this, I think I prefer to make tryEnsureGstGLContext() > > const and mark m_gstGLContext and m_gstGLDisplay as mutable, instead of a > > const cast of this. Unless this is better for whatever reason... > > let me try that approach. > > I copied it from here > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/ > graphics/PlatformDisplay.cpp#L179 :) That case was tricky because initializeEGLDisplay() is virtual, IIRC.
Víctor M. Jáquez L.
Comment 31 2020-02-26 03:30:48 PST
Víctor M. Jáquez L.
Comment 32 2020-02-26 03:42:56 PST
(In reply to Carlos Garcia Campos from comment #28) > Comment on attachment 391657 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391657&action=review > > > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:134 > > + if (!const_cast<PlatformDisplay*>(this)->tryEnsureGstGLContext()) > > I had never seen this, I think I prefer to make tryEnsureGstGLContext() > const and mark m_gstGLContext and m_gstGLDisplay as mutable, instead of a > const cast of this. Unless this is better for whatever reason... Done! But I still have to call sharingGLContext() in this way since it's not const, and changing it showed up several compilation errors, so that might have be handled in other patch.
WebKit Commit Bot
Comment 33 2020-02-26 04:35:17 PST
Comment on attachment 391733 [details] Patch Clearing flags on attachment: 391733 Committed r257463: <https://trac.webkit.org/changeset/257463>
WebKit Commit Bot
Comment 34 2020-02-26 04:35:20 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 35 2020-02-26 04:36:15 PST
Note You need to log in before you can comment on or make changes to this bug.