Bug 150890 – [GStreamer] Use MainThreadNotifier to send notifications to main thread in WebKitWebSourceGStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

Instead of the GThreadSafeMainLoopSources.

[Carlos Garcia Campos]

Created attachment 264788 [details]
Patch

[WebKit Commit Bot]

Attachment 264788 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:178:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:189:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:197:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:208:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:233:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:655:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:667:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 264788 [details]
Patch

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

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.

> 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.

[Carlos Garcia Campos]

Created attachment 264937 [details]
Updated patch

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 264788 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264788&action=review
> 
> 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.

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()

> > 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.

[WebKit Commit Bot]

Attachment 264937 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:178:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:189:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:192:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:203:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:223:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:637:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:645:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 264937 [details]
Updated patch

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

r=me.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:418
> +    bool wasSeeking = priv->isSeeking;

Consider combining this with the priv->isSeeking override below into std::exchange().

[Carlos Garcia Campos]

Committed r192102: <http://trac.webkit.org/changeset/192102>