<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Use MainThreadNotifier to send notifications to main thread in WebKitWebSourceGStreamer"
href="https://bugs.webkit.org/show_bug.cgi?id=150890#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Use MainThreadNotifier to send notifications to main thread in WebKitWebSourceGStreamer"
href="https://bugs.webkit.org/show_bug.cgi?id=150890">bug 150890</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=150890#c3">comment #3</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=264788&action=diff" name="attach_264788" title="Patch">attachment 264788</a> <a href="attachment.cgi?id=264788&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=264788&action=review">https://bugs.webkit.org/attachment.cgi?id=264788&action=review</a>
>
> All the uses of MainThreadNotifier<>::notify() need updating, otherwise
> looks fine.
>
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:177
> > + // NeedData
>
> I usually write these in the same C-style they're declared in: need_data,
> enough_data, seek_data.
>
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:194
> > + {
> > + WTF::GMutexLocker<GMutex> locker(*GST_OBJECT_GET_LOCK(src));
> > + if (!priv->paused)
> > + return;
> > +
> > + if (!isMainThread()) {
> > + GRefPtr<WebKitWebSrc> protector(src);
> > + priv->notifier.notify(MainThreadSourceNotification::NeedData, [protector] { webKitWebSrcNeedData(protector.get()); });
> > + return;
> > + }
> > + }
> > +
> > + webKitWebSrcNeedData(src);
>
> Can webKitWebSrcNeedData(), webKitWebSrcEnoughData() and webKitWebSrcSeek()
> all be rewritten and used with the expectation that the
> GST_OBJECT_GET_LOCK() is already locked? That way the lock can be held
> throughout the lambda scope, and there's no need for the isMainThread()
> check at this level.</span >
I thought it was possible but it's not that easy. We don't want to keep the lock until the notification happens in the main thread, so we need to take lock again before calling the callback in the lambda.
The notifier is thread say, so we can just release the lock before calling notify()
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:656
> > + if (isMainThread())
> > + webKitWebSrcStart(src);
> > + else {
> > + GRefPtr<WebKitWebSrc> protector(src);
> > + priv->notifier.notify(MainThreadSourceNotification::Start, [protector] { webKitWebSrcStart(protector.get()); });
> > + }
>
> Similarly, webKitWebSrcStart() and webKitWebSrcStop() could just work with
> the assumption (or an actual run-time assertion) that the GstObject lock is
> locked.
>
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:833
> > +static void webKitWebSrcSeek(WebKitWebSrc* src)
>
> If all the above is possible, don't forget to set up the lock here.</span ></pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>