[webkit-reviews] review granted: [Bug 204624] [GStreamer] Move GL video sink to its own GstBin sub-class : [Attachment 384364] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 27 01:41:43 PST 2019
Carlos Garcia Campos <cgarcia at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 204624: [GStreamer] Move GL video sink to its own GstBin sub-class
https://bugs.webkit.org/show_bug.cgi?id=204624
Attachment 384364: Patch
https://bugs.webkit.org/attachment.cgi?id=384364&action=review
--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 384364
--> https://bugs.webkit.org/attachment.cgi?id=384364
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
More information about the webkit-reviews
mailing list