Bug 142693 – [GStreamer] share GL context in pipeline

• Component: Platform
• Status: RESOLVED FIXED

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

When dealing with GstGL elements, they need the display and the gl context to work on. These parameters are held by the application and it needs to share it to the pipeline through a GstContext message.

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

Created attachment 248649 [details]
Patch

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

Created attachment 248650 [details]
hack for adding glfiltersobel as video filter in the pipeline

This is a hack for adding a GL sobel filter in the pipeline, causing the sharing of GL context. After that all the videos only shows the image's edges :)

I tested it with the gst-libav decoders.

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

It seems that we need to enable the XInitThreads() for make this working correctly:

--- a/Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp
+++ b/Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp
@@ -51,7 +51,7 @@ public:
             sleep(30);
 #endif
 
-#if USE(COORDINATED_GRAPHICS_THREADED) && PLATFORM(X11)
+#if PLATFORM(X11)
         XInitThreads();
 #endif
         gtk_init(nullptr, nullptr);

[Philippe Normand]

Comment on attachment 248649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248649&action=review

Looks good, only a few small fixes needed

> Source/WebCore/ChangeLog:4
> +        Bug 142693 - [GStreamer] share GL context in pipeline
> +

Please remove these 2 lines

> Source/WebCore/ChangeLog:11
> +        In order to the gstreamer pipeline could use gstreamer-gl elements,
> +        the must be aware of the application display and GL context. This

I'm not a native English speaker but it seems this sentence could use some rewording :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:943
> +    case GST_MESSAGE_NEED_CONTEXT: {
> +#if USE(GSTREAMER_GL)

Hum perhaps include the case statement inside the ifdef?

> ChangeLog:4
> +        Bug 142693 - [GStreamer] share GL context in pipeline
> +

Please remove these 2 lines as well

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

Created attachment 248651 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248651 [details]
Patch

Clearing flags on attachment: 248651

Committed r181499: <http://trac.webkit.org/changeset/181499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 248651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248651&action=review

Sorry to be late here, but I have some comments/questions.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:938
> +gboolean MediaPlayerPrivateGStreamer::handleSyncMessage(GstMessage* message)

Nit: This should probably be bool.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:944
> +        const gchar* contextType;
> +        gst_message_parse_context_type(message, &contextType);

Could this contextType be something different than GST_GL_DISPLAY_CONTEXT_TYPE or gst.gl.app_context? In that case we should probably return early here and avoid creating (and leaking) the contexts. Otherwise, it should probably be moved before the if (!g_strcmp0

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:957
> +            m_glDisplay = reinterpret_cast<GstGLDisplay*>(gstGLDisplay);

You could use GST_GL_DISPLAY instead of the C++ style cast here, and then yu don't need to cast again in gst_gl_context_new_wrapped(), using m_glDisplay instead.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:960
> +            GLXContext* glxSharingContext = reinterpret_cast<GLXContext*>(webkitContext->platformContext());

This is only defined when 3D_GRAPHICS is defined, so you need to use #if USE(3D_GRAPHICS). platformContext() is a pure virtual method, you don't need to do this inside the GLX/EGL ifdefs using casts, you could do that before using PlatformGraphicsContext3D or auto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:961
> +            if (glxSharingContext && !m_glContext)

Can glxSharingContext really be nullptr? what happens in that case? we might be using a null m_glContext below.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:966
> +                m_glContext = gst_gl_context_new_wrapped(GST_GL_DISPLAY(gstGLDisplay), reinterpret_cast<guintptr>(eglSharingContext), GST_GL_PLATFORM_EGL, GST_GL_API_GLES2);

hmm, I'm not OpenGL expert, but I think it's possible to use OpenGL API with EGL, so instead of hardcoding GST_GL_API_GLES2 here, you should be using GST_GL_API_OPENGL or GST_GL_API_GLES2 depending on ENABLE(GLES2)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:972
> +            gst_context_set_gl_display(displayContext, m_glDisplay);

So, here the GstContext takes the ownership of the m_glDisplay by consuming the sink reference, right? Is there any guarantee that the GstContext is not going to release its ref while the media player object is alive? Because otherwise we should consume the sink ref and let the GstContext take its own.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:979
> +            gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, m_glContext, nullptr);

Same for the m_glContext

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:983
> +        break;

So, we use the m_glDisplay for the GST_GL_DISPLAY_CONTEXT_TYPE and m_glContext for the gst.gl.app_context. Shouldn't we check before the context type and only create the m_glContext if contextType is gst.gl.app_context? Or is it guaranteed that both messages will be called? Otherwise we would be leaking.

> Source/cmake/OptionsGTK.cmake:278
> +    if (PC_GSTREAMER_GL_FOUND)
> +        add_definitions(-DWTF_USE_GSTREAMER_GL)
> +        set(USE_GSTREAMER_GL TRUE)
> +    endif ()

Is this enough? Shouldn't we also check OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND? If there's gst gl support but we are building without opengl, the code would probably crash because it assumes that m_glDisplay and m_glcontext are created.

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

(In reply to comment #8)
> Comment on attachment 248651 [details]

Thanks for your review Carlos!

I'll take care of them by the end of the week.

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

Comment on attachment 248651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248651&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:972
>> +            gst_context_set_gl_display(displayContext, m_glDisplay);
> 
> So, here the GstContext takes the ownership of the m_glDisplay by consuming the sink reference, right? Is there any guarantee that the GstContext is not going to release its ref while the media player object is alive? Because otherwise we should consume the sink ref and let the GstContext take its own.

GstContext doesn't take the ownership of m_glDisplay. That's why we keep m_glDisplay along the media player's life.

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

Comment on attachment 248651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248651&action=review

>> Source/cmake/OptionsGTK.cmake:278
>> +    endif ()
> 
> Is this enough? Shouldn't we also check OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND? If there's gst gl support but we are building without opengl, the code would probably crash because it assumes that m_glDisplay and m_glcontext are created.

The purpose of this snippet is to find if we have the GstGL API, what kind of GL API/Platform is supported should be handled in other place, IMO.

[Carlos Garcia Campos]

Comment on attachment 248651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248651&action=review

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:972
>>> +            gst_context_set_gl_display(displayContext, m_glDisplay);
>> 
>> So, here the GstContext takes the ownership of the m_glDisplay by consuming the sink reference, right? Is there any guarantee that the GstContext is not going to release its ref while the media player object is alive? Because otherwise we should consume the sink ref and let the GstContext take its own.
> 
> GstContext doesn't take the ownership of m_glDisplay. That's why we keep m_glDisplay along the media player's life.

Then that means the m_glDisplay has a floating reference that nobody consumes?

>>> Source/cmake/OptionsGTK.cmake:278
>>> +    endif ()
>> 
>> Is this enough? Shouldn't we also check OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND? If there's gst gl support but we are building without opengl, the code would probably crash because it assumes that m_glDisplay and m_glcontext are created.
> 
> The purpose of this snippet is to find if we have the GstGL API, what kind of GL API/Platform is supported should be handled in other place, IMO.

But it's defining USE_GSTREAMER_GL, which means GSTREAMER_GL should be used, not that it was found.

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

(In reply to comment #12)
> Comment on attachment 248651 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248651&action=review
> 
> >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:972
> >>> +            gst_context_set_gl_display(displayContext, m_glDisplay);
> >> 
> >> So, here the GstContext takes the ownership of the m_glDisplay by consuming the sink reference, right? Is there any guarantee that the GstContext is not going to release its ref while the media player object is alive? Because otherwise we should consume the sink ref and let the GstContext take its own.
> > 
> > GstContext doesn't take the ownership of m_glDisplay. That's why we keep m_glDisplay along the media player's life.
> 
> Then that means the m_glDisplay has a floating reference that nobody
> consumes?

Yup. Right now m_glDisplay and m_glContext leak.

> 
> >>> Source/cmake/OptionsGTK.cmake:278
> >>> +    endif ()
> >> 
> >> Is this enough? Shouldn't we also check OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND? If there's gst gl support but we are building without opengl, the code would probably crash because it assumes that m_glDisplay and m_glcontext are created.
> > 
> > The purpose of this snippet is to find if we have the GstGL API, what kind of GL API/Platform is supported should be handled in other place, IMO.
> 
> But it's defining USE_GSTREAMER_GL, which means GSTREAMER_GL should be used,
> not that it was found.

USE_GSTREAMER_GL is true if PC_GSTREAMER_GL_FOUND is true, which comes, if I understand correctly, from GSTREAMER_COMPONENTS checked with pkg-config.

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

To fix the issues that Carlos García aimed.

[Philippe Normand]

We usually don't reopen bugs for follow-up patches, please open a new one

[Carlos Garcia Campos]

Comment on attachment 248651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248651&action=review

>>>>> Source/cmake/OptionsGTK.cmake:278
>>>>> +    endif ()
>>>> 
>>>> Is this enough? Shouldn't we also check OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND? If there's gst gl support but we are building without opengl, the code would probably crash because it assumes that m_glDisplay and m_glcontext are created.
>>> 
>>> The purpose of this snippet is to find if we have the GstGL API, what kind of GL API/Platform is supported should be handled in other place, IMO.
>> 
>> But it's defining USE_GSTREAMER_GL, which means GSTREAMER_GL should be used, not that it was found.
> 
> USE_GSTREAMER_GL is true if PC_GSTREAMER_GL_FOUND is true, which comes, if I understand correctly, from GSTREAMER_COMPONENTS checked with pkg-config.

Exactly, but that doesn't mean OPENGL_FOUND or OPENGLES2_FOUND and GLX_FOUND OR EGL_FOUND are ON.

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

Reopening to attach new patch.

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

Created attachment 249402 [details]
Patch

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

(In reply to comment #15)
> We usually don't reopen bugs for follow-up patches, please open a new one

oops...

I asked earlier to Carlos and he let me choose. Now I don't know what to do. :/

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

(In reply to comment #19)
> (In reply to comment #15)
> > We usually don't reopen bugs for follow-up patches, please open a new one
> 
> oops...
> 
> I asked earlier to Carlos and he let me choose. Now I don't know what to do.
> :/

OK. I'm going to close this and create a new one.