Bug 204624

Summary: [GStreamer] Move GL video sink to its own GstBin sub-class
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, gyuyoung.kim, jbedard, jer.noble, menard, philipj, ryuan.choi, sergio, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 204669    
Attachments:
Description Flags
Patch
none
Patch
none
Patch calvaris: review+

Description Philippe Normand 2019-11-26 09:43:26 PST
.
Comment 1 Philippe Normand 2019-11-26 09:51:05 PST
Created attachment 384363 [details]
Patch
Comment 2 Philippe Normand 2019-11-26 10:02:29 PST
Created attachment 384364 [details]
Patch
Comment 3 Carlos Garcia Campos 2019-11-27 01:41:43 PST
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
Comment 4 Xabier Rodríguez Calvar 2019-11-27 02:48:20 PST
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";
Comment 5 Philippe Normand 2019-11-27 07:06:38 PST
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.
Comment 6 Philippe Normand 2019-11-27 07:38:18 PST
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!
Comment 7 Philippe Normand 2019-11-27 08:30:47 PST
Created attachment 384413 [details]
Patch
Comment 8 Xabier Rodríguez Calvar 2019-11-28 00:59:00 PST
Please, the changes you left, create a follow up patch.
Comment 9 Carlos Garcia Campos 2019-11-28 02:03:40 PST
(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().
Comment 10 Philippe Normand 2019-11-28 02:09:24 PST
Committed r252917: <https://trac.webkit.org/changeset/252917>
Comment 11 Radar WebKit Bug Importer 2019-11-28 02:10:27 PST
<rdar://problem/57517440>