[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

Attachment 384364: Patch


--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 384364
  --> https://bugs.webkit.org/attachment.cgi?id=384364

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,

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
> +    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 =
> +    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();


> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:59
> +#include "GLVideoSinkGStreamer.h"
> +#endif

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

More information about the webkit-reviews mailing list