[Webkit-unassigned] [Bug 138562] [GStreamer] GstGL support in the video sink

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138562

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

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

> Source/WebCore/ChangeLog:15
> +        [GStreamer] GstGL support in the video sink
> +        https://bugs.webkit.org/show_bug.cgi?id=138562
> +
> +        Use GStreamer's glimagesink for video rendering instead of our
> +        custom video sink if a recent-enough version of GstGL is found
> +        during the build. When glimagesink is used it passes a texture to
> +        the media player which then wraps it inside a TextureMapper
> +        texture later used for actual rendering.
> +
> +        Using this new code path will allow us to remove our custom sink
> +        entirely in the long term.
> +
> +        No new test, existing media tests cover video rendering already.

Reviewed by line is missing in the changelog.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:354
> +    g_return_if_fail(GST_IS_SAMPLE(sample));

Is it really possible that the client-draw signal is emitted with a NULL or invalid sample?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:360
> +        if (m_sample)
> +            gst_sample_unref(m_sample);
> +        m_sample = gst_sample_ref(sample);

m_sample should probably be a GRefPtr

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:369
> +            if (supportsAcceleratedRendering() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player) && client())

I would check client() is not nullptr, before asking the m_player->client().

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:373
> +        }, std::chrono::milliseconds(0), G_PRIORITY_HIGH, nullptr, g_main_context_get_thread_default());

What's the point of scheduling after a 0 delay? Why not using schedule() instead? G_PRIORITY_HIGH is too much, it will affect any other source in the context.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:374
> +        g_cond_wait(&m_drawCondition, &m_drawMutex);

Why does this need to be blocking? what thread is this supposed to be run? We should probably add an ASSERT(isMainThread()) or !isMainThread.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:443
> +    GstBuffer* buffer = gst_sample_get_buffer(m_sample);

You could do this later, it's not needed if caps is NULL or gst_video_info_from_caps returns FALSE.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:457
> +    guint textureID = *(guint *) videoFrame.data[0];

Use C++ style cast for this.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:506
> +        videoSink = m_webkitVideoSink.get();
> +        return videoSink;

why do you need the local variable? can't you just return m_webkitVideoSink.get()?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:82
> +#if USE(GSTREAMER_GL)
> +    void triggerDraw(GstSample*);
> +#endif
>      void triggerRepaint(GstSample*);

Since you are reusing the m_repaintHandler, maybe you could reuse the trigger function too, since they share some code. You could add the specific code of triggerDraw into triggerRepaint inside a #if USE(GSTREAMER_GL) block.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150324/1b3bf297/attachment-0002.html>