Bug 150888 – [GStreamer] Do not use GThreadSafeMainLoopSource to send notifications to the main thread in MediaPlayerPrivateGStreamer

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[WebKit Commit Bot]

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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

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

[Zan Dobersek]

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

[Carlos Garcia Campos]

Created attachment 264932 [details]
Updated patch

[WebKit Commit Bot]

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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

Committed r192099: <http://trac.webkit.org/changeset/192099>