Bug 204624 – [GStreamer] Move GL video sink to its own GstBin sub-class

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

.

[Philippe Normand]

Created attachment 384363 [details]
Patch

[Philippe Normand]

Created attachment 384364 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 384364 [details]
Patch

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

This is just moving code, but I have a few comments, feel free to apply them before landing, or in a follow up patch.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:57
> +#include <gst/app/gstappsink.h>

This should be before the conditional includes, in the main includes block.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:80
> +G_DEFINE_TYPE_WITH_CODE(WebKitGLVideoSink, webkit_gl_video_sink, GST_TYPE_BIN, WEBKIT_GL_VIDEO_SINK_CATEGORY_INIT);

You could use WTF_DEFINE_TYPE

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:85
> +    sink->priv = WEBKIT_GL_VIDEO_SINK_GET_PRIVATE(sink);
> +    new (sink->priv) WebKitGLVideoSinkPrivate();

And then you don't need to do this.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:127
> +    ASSERT(WTF::isMainThread());

WTF:: is not needed here.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:136
> +    // We used a placement new for construction, the destructor won't be called automatically.
> +    priv->~_WebKitGLVideoSinkPrivate();

You don't need to do this either if using WTF_DEFINE_TYPE

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:151
> +    // The floating ref removal support was added in https://bugzilla.gnome.org/show_bug.cgi?id=743062.
> +    bool shouldAdoptRef = webkitGstCheckVersion(1, 14, 0);

I think we could simplify the code by adding adoptGLRef() or something like that, similar to ensureGRef, that only accepts GstGLDisplay org GstGLContext and adopts the returned ref or not depending on the GST version.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:196
> +    GLContext* webkitContext = sharedDisplay.sharingGLContext();

webkitContext -> sharedContext ?

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:210
> +    PlatformGraphicsContext3D contextHandle = webkitContext->platformContext();
> +    if (!contextHandle)
> +        return false;

This could be moved above, right after setting webkitContext

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:218
> +    auto previousActiveContext = GLContext::current();

auto*

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:59
> +#if USE(GSTREAMER_GL)
> +#include "GLVideoSinkGStreamer.h"
> +#endif

No need for the #if here, because GLVideoSinkGStreamer.h is already guarded

[Xabier Rodríguez Calvar]

Comment on attachment 384364 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:52
> +// defines None, breaking MediaPlayer::None enum

.

>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:151
>> +    bool shouldAdoptRef = webkitGstCheckVersion(1, 14, 0);
> 
> I think we could simplify the code by adding adoptGLRef() or something like that, similar to ensureGRef, that only accepts GstGLDisplay org GstGLContext and adopts the returned ref or not depending on the GST version.

I already said this in another patch review. I'm sure he would like to do it in a follow up, right?

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:233
> +static GstContext* requestGLContext(WebKitGLVideoSink* sink, const char* contextType)

Follow up or not, I observed that we always adopt the result of this function. I think we could make this return a GRefPtr<GstContext> and make the adoption inside, then we return the pointer and let the copy elision make wonders.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:252
> +#if GST_CHECK_VERSION(1, 12, 0)
> +        gst_structure_set(structure, "context", GST_TYPE_GL_CONTEXT, priv->glContext.get(), nullptr);
> +#else
> +        gst_structure_set(structure, "context", GST_GL_TYPE_CONTEXT, priv->glContext.get(), nullptr);
> +#endif

Follow up or not, I think we could #define this above and have just one line here.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:349
> +    auto sink = adoptGRef(gst_element_factory_make("appsink", nullptr));

This is transfer floating, we can't adopt or it will crash.

IMHO, this should be valid for transfer floating and I suggested to make the appropriate changes long ago but sadly nobody (mea culpa as well) made the change.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:356
> +    auto colorconvert = adoptGRef(gst_element_factory_make("glcolorconvert", nullptr));
> +    auto upload = adoptGRef(gst_element_factory_make("glupload", nullptr));

Ditto

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:360
> +
> +    auto colorconvert = adoptGRef(gst_element_factory_make("glcolorconvert", nullptr));
> +    auto upload = adoptGRef(gst_element_factory_make("glupload", nullptr));
> +    if (!colorconvert || !upload) {
> +        GST_WARNING("GstGL elements unavailable. Please check your GStreamer installation");
> +        return false;
> +    }

And regarding these checks and the checks above, I would create a small function, maybe in GStreamerCommon, to test the existance of a plugin, return true if it exists, and GST_WARNING if it does not. Then this function would be "return test && test && test";

[Philippe Normand]

Comment on attachment 384364 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:80
>> +G_DEFINE_TYPE_WITH_CODE(WebKitGLVideoSink, webkit_gl_video_sink, GST_TYPE_BIN, WEBKIT_GL_VIDEO_SINK_CATEGORY_INIT);
> 
> You could use WTF_DEFINE_TYPE

It's not the first time WEBKIT_DEFINE_TYPE is suggested for a GStreamer element. The problem is that macro tries to do too much magic and doesn't allow a custom instance_init() function. So we can't use that macro.

>>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:151
>>> +    bool shouldAdoptRef = webkitGstCheckVersion(1, 14, 0);
>> 
>> I think we could simplify the code by adding adoptGLRef() or something like that, similar to ensureGRef, that only accepts GstGLDisplay org GstGLContext and adopts the returned ref or not depending on the GST version.
> 
> I already said this in another patch review. I'm sure he would like to do it in a follow up, right?

This snark wasn't needed here.

[Philippe Normand]

Comment on attachment 384364 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:360
>> +    }
> 
> And regarding these checks and the checks above, I would create a small function, maybe in GStreamerCommon, to test the existance of a plugin, return true if it exists, and GST_WARNING if it does not. Then this function would be "return test && test && test";

Good idea!

[Philippe Normand]

Created attachment 384413 [details]
Patch

[Xabier Rodríguez Calvar]

Please, the changes you left, create a follow up patch.

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #5)
> Comment on attachment 384364 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384364&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:80
> >> +G_DEFINE_TYPE_WITH_CODE(WebKitGLVideoSink, webkit_gl_video_sink, GST_TYPE_BIN, WEBKIT_GL_VIDEO_SINK_CATEGORY_INIT);
> > 
> > You could use WTF_DEFINE_TYPE
> 
> It's not the first time WEBKIT_DEFINE_TYPE is suggested for a GStreamer
> element. The problem is that macro tries to do too much magic and doesn't
> allow a custom instance_init() function. So we can't use that macro.

You are expected to use constructed() instead of init().

[Philippe Normand]

Committed r252917: <https://trac.webkit.org/changeset/252917>

[Radar WebKit Bug Importer]

<rdar://problem/57517440>