Bug 142693

Summary: [GStreamer] share GL context in pipeline
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: PlatformAssignee: Víctor M. Jáquez L. <vjaquez>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, gns, mrobinson, ossy, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=143049
Attachments:
Description Flags
Patch
none
hack for adding glfiltersobel as video filter in the pipeline
none
Patch
none
Patch none

Description Víctor M. Jáquez L. 2015-03-14 08:01:23 PDT
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.
Comment 1 Víctor M. Jáquez L. 2015-03-14 08:30:53 PDT
Created attachment 248649 [details]
Patch
Comment 2 Víctor M. Jáquez L. 2015-03-14 08:37:48 PDT
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.
Comment 3 Víctor M. Jáquez L. 2015-03-14 09:01:20 PDT
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);
Comment 4 Philippe Normand 2015-03-14 09:01:39 PDT
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
Comment 5 Víctor M. Jáquez L. 2015-03-14 09:50:39 PDT
Created attachment 248651 [details]
Patch
Comment 6 WebKit Commit Bot 2015-03-14 11:26:31 PDT
Comment on attachment 248651 [details]
Patch

Clearing flags on attachment: 248651

Committed r181499: <http://trac.webkit.org/changeset/181499>
Comment 7 WebKit Commit Bot 2015-03-14 11:26:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Carlos Garcia Campos 2015-03-16 00:24:25 PDT
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.
Comment 9 Víctor M. Jáquez L. 2015-03-17 02:05:06 PDT
(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.
Comment 10 Víctor M. Jáquez L. 2015-03-25 03:24:39 PDT
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.
Comment 11 Víctor M. Jáquez L. 2015-03-25 03:47:46 PDT
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.
Comment 12 Carlos Garcia Campos 2015-03-25 04:02:50 PDT
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.
Comment 13 Víctor M. Jáquez L. 2015-03-25 07:47:05 PDT
(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.
Comment 14 Víctor M. Jáquez L. 2015-03-25 07:48:01 PDT
To fix the issues that Carlos García aimed.
Comment 15 Philippe Normand 2015-03-25 07:53:02 PDT
We usually don't reopen bugs for follow-up patches, please open a new one
Comment 16 Carlos Garcia Campos 2015-03-25 07:53:33 PDT
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.
Comment 17 Víctor M. Jáquez L. 2015-03-25 07:59:25 PDT
Reopening to attach new patch.
Comment 18 Víctor M. Jáquez L. 2015-03-25 07:59:30 PDT
Created attachment 249402 [details]
Patch
Comment 19 Víctor M. Jáquez L. 2015-03-25 08:07:07 PDT
(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. :/
Comment 20 Víctor M. Jáquez L. 2015-03-25 08:08:34 PDT
(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.