[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 #36 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #34)
> (In reply to comment #33)
> > > 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?
> > 
> 
> No, I'll remove this g_return_if_fail.
> 
> > > 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
> > 
> 
> Ok I can do that in a separate patch.
> 
> > > 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().
> > 
> 
> I checked MediaPlayer, the client is never null there and no one reported a
> crash about this either.

What I meant was doing 

if (supportsAcceleratedRendering() && client() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player))

so that we don't even use the media player client if client() is nullptr. But I guess that hardly ever happens, so it's probably not so important.

> > > 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.
> > 
> 
> Oh yeah that's what I did first, I'll switch back to a simple schedule :)
> 
> > > 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.
> > 
> 
> This signal is emitted from the sink streaming thread but the texture can't
> be used from there, hence the switch to main thread. And it needs to be
> blocking because the sample emitted is freed from the sink after the signal
> emission completed.

Can't you just take a reference of the sample then?

-- 
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/54ed6d36/attachment-0002.html>