Bug 150890

Summary: [GStreamer] Use MainThreadNotifier to send notifications to main thread in WebKitWebSourceGStreamer
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, pnormand, zan
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150888    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated patch zan: review+

Description Carlos Garcia Campos 2015-11-04 03:40:36 PST
Instead of the GThreadSafeMainLoopSources.
Comment 1 Carlos Garcia Campos 2015-11-04 03:42:52 PST
Created attachment 264788 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-04 03:45:27 PST
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.
Comment 3 Zan Dobersek 2015-11-06 05:17:16 PST
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.
Comment 4 Carlos Garcia Campos 2015-11-06 06:05:55 PST
Created attachment 264937 [details]
Updated patch
Comment 5 Carlos Garcia Campos 2015-11-06 06:07:26 PST
(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.
Comment 6 WebKit Commit Bot 2015-11-06 06:08:35 PST
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.
Comment 7 Zan Dobersek 2015-11-06 06:24:39 PST
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().
Comment 8 Carlos Garcia Campos 2015-11-06 06:52:49 PST
Committed r192102: <http://trac.webkit.org/changeset/192102>