Summary: | [GStreamer] Use MainThreadNotifier to send notifications to main thread in WebKitWebSourceGStreamer | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | Platform | Assignee: | 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
Carlos Garcia Campos
2015-11-04 03:40:36 PST
Created attachment 264788 [details]
Patch
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 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. Created attachment 264937 [details]
Updated patch
(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. 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 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(). Committed r192102: <http://trac.webkit.org/changeset/192102> |