Summary: | [GStreamer] Move GL video sink to its own GstBin sub-class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||
Component: | Platform | Assignee: | 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
Philippe Normand
2019-11-26 09:43:26 PST
Created attachment 384363 [details]
Patch
Created attachment 384364 [details]
Patch
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 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 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 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! Created attachment 384413 [details]
Patch
Please, the changes you left, create a follow up patch. (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(). Committed r252917: <https://trac.webkit.org/changeset/252917> |