Bug 200626 – [GL][GStreamer] Instantiate GstGLContext when the shared GLContext is created

• Component: Media
• Status: RESOLVED FIXED

[Víctor M. Jáquez L.]

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.

[Philippe Normand]

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

Created attachment 389659 [details]
WIP Patch

[Philippe Normand]

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

Created attachment 389793 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 389794 [details]
Patch

[Víctor M. Jáquez L.]

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

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

Created attachment 390380 [details]
Patch

[Víctor M. Jáquez L.]

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

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

By the way, instead of using std::unique_ptr I'm going to explore using std::optional, as in Rust :D

[Carlos Garcia Campos]

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

Created attachment 391045 [details]
Patch

[Víctor M. Jáquez L.]

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

Created attachment 391054 [details]
Patch

[Carlos Garcia Campos]

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

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

Created attachment 391535 [details]
Patch

[Víctor M. Jáquez L.]

Another round ready :)

[Carlos Garcia Campos]

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

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]

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

Created attachment 391657 [details]
Patch

[Carlos Garcia Campos]

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

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

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

Created attachment 391733 [details]
Patch

[Víctor M. Jáquez L.]

(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 on attachment 391733 [details]
Patch

Clearing flags on attachment: 391733

Committed r257463: <https://trac.webkit.org/changeset/257463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59802076>