Analyzing how the main loop sources were used in GST code I've noticed that in most of the cases they are used to send notifications to the main thread. The way it works in those cases is that some state is updated in whatever thread and we notify the main thread to use the new state. There's no data passed to the main thread, they are just notifications. I've also noticed that we are not doing this exactly as expected in several of those cases. GThreadSafeMainLoopSource cancels the current source when a new one is scheduled, and that was done this way because previous code in GST using GSources directly did it that way. But that's not what we want, if there's a notification pending, since the state is updated, we can just wait for it to happen instead of cancelling and scheduling a new one. I've also noticed that in most of the cases where we schedule notifications to the main thread, we can be already in the main thread, so we could avoid the schedule entirely.
Created attachment 264786 [details] Patch We can use RunLoop::dispatch() to send notifications to the main thread, but there's no way to cancel those tasks. This patch adds a new helper class MainThreadNotifier that uses an enum of flags to handle different kind of notifications. It uses RunLoop::dispatch() to send notifications to the main thread, but only if there isn't one pending for the given type.
Attachment 264786 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:326: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:378: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:36: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:615: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:660: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:674: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:720: More than one command on the same line [whitespace/newline] [4] Total errors found: 7 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264786&action=review Again, please have Phil review the patch as well. > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:29 > +template <typename T> > +class MainThreadNotifier { Neat implementation. > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:46 > + if (!weakThis) > + return; > + if (weakThis->removePendingNotitifation(notificationType)) > + callback(); Can this be a two-liner? > if (weakThis && weakThis->removePendingNotification(notificationType)) > callback(); > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:61 > + bool addPendingNotitifation(T notificationType) Typo: notitifation. > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:70 > + bool removePendingNotitifation(T notificationType) Ditto re: typo. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:615 > + if (isMainThread()) > + player->notifyPlayerOfVideo(); > + else > + player->m_notifier.notify(MainThreadNotification::VideoChanged, [player] { player->notifyPlayerOfVideo(); }); This isMainThread() check and the conditional immediate execution could be rolled up into the notifier class. Would save some lines. MainThreadNotifier::notify() would accept a templated functor as the second parameter, invoke it immediately if on the main thread, or wrap it in std::function<> and dispatch it onto the main thread via RunLoop otherwise.
(In reply to comment #3) > Comment on attachment 264786 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264786&action=review > > Again, please have Phil review the patch as well. Sure. > > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:29 > > +template <typename T> > > +class MainThreadNotifier { > > Neat implementation. :-) > > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:46 > > + if (!weakThis) > > + return; > > + if (weakThis->removePendingNotitifation(notificationType)) > > + callback(); > > Can this be a two-liner? Sure. > > if (weakThis && weakThis->removePendingNotification(notificationType)) > > callback(); > > > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:61 > > + bool addPendingNotitifation(T notificationType) > > Typo: notitifation. Oops > > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:70 > > + bool removePendingNotitifation(T notificationType) > > Ditto re: typo. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:615 > > + if (isMainThread()) > > + player->notifyPlayerOfVideo(); > > + else > > + player->m_notifier.notify(MainThreadNotification::VideoChanged, [player] { player->notifyPlayerOfVideo(); }); > > This isMainThread() check and the conditional immediate execution could be > rolled up into the notifier class. Would save some lines. > > MainThreadNotifier::notify() would accept a templated functor as the second > parameter, invoke it immediately if on the main thread, or wrap it in > std::function<> and dispatch it onto the main thread via RunLoop otherwise. I thought about that, but decided to do it in callers to avoid the function creation just to invoke a member method. But I'm not sure if creating the function would have any real impact in the end, what do you think?
(In reply to comment #4) > (In reply to comment #3) > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:615 > > > + if (isMainThread()) > > > + player->notifyPlayerOfVideo(); > > > + else > > > + player->m_notifier.notify(MainThreadNotification::VideoChanged, [player] { player->notifyPlayerOfVideo(); }); > > > > This isMainThread() check and the conditional immediate execution could be > > rolled up into the notifier class. Would save some lines. > > > > MainThreadNotifier::notify() would accept a templated functor as the second > > parameter, invoke it immediately if on the main thread, or wrap it in > > std::function<> and dispatch it onto the main thread via RunLoop otherwise. > > I thought about that, but decided to do it in callers to avoid the function > creation just to invoke a member method. But I'm not sure if creating the > function would have any real impact in the end, what do you think? You wouldn't create a std::function<> object until you know you're not on the main thread and have to use the RunLoop dispatch instead. template<typename F> void notify(T notificationType, const F& callbackFunctor) { if (!addPendingNotitifation(notificationType)) return; if (!isMainThread()) { auto weakThis = m_weakPtrFactory.createWeakPtr(); std::function<void ()> callback(callbackFunctor); RunLoop::main().dispatch([weakThis, notificationType, callback] { if (weakThis && weakThis->removePendingNotitifation(notificationType)) callback(); }); } else callbackFunctor(); }
Created attachment 264932 [details] Updated patch
Attachment 264932 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:323: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:372: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:50: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:612: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:654: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:665: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:708: More than one command on the same line [whitespace/newline] [4] Total errors found: 7 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264932 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=264932&action=review r=me if Phil doesn't object. > Source/WebCore/ChangeLog:38 > + (WebCore::MainThreadNotifier::addPendingNotitifation): > + (WebCore::MainThreadNotifier::removePendingNotitifation): Update this.
Committed r192099: <http://trac.webkit.org/changeset/192099>