Bug 35747 – [GStreamer] on Mac OS use a runloop observer to process the glib context iterations

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

Applications using the GTK+ port will most likely use a glib main loop but in the eventuality of reusing the media player backend from a port not depending on glib main loop we'd need to have a Timer periodically polling messages from the GStreamer bus.

[Philippe Normand]

Created attachment 50023 [details]
proposed patch

[Gustavo Noronha (kov)]

Comment on attachment 50023 [details]
proposed patch

Hrm. I don't know. I don't like the runtime checking, also the polling time seems to be pretty arbitrary. I'm worried we may end up in that situation in the gtk+ port, in some cases (say, the application hasn't run gtk_main(), but is running the mainloop itself). Why not have ports that would like to use the GStreamer backend run the glib main loop?

[Philippe Normand]

(In reply to comment #2)
> (From update of attachment 50023 [details])
> Hrm. I don't know. I don't like the runtime checking

What would be a better checking?

> also the polling time
> seems to be pretty arbitrary. 

Agreed, it'd probably need some tweaking

> I'm worried we may end up in that situation in
> the gtk+ port, in some cases (say, the application hasn't run gtk_main(), but
> is running the mainloop itself). Why not have ports that would like to use the
> GStreamer backend run the glib main loop?

I'm not sure this would be an option for, say, the mac or win port :) It'd be nice to propose the GStreamer backend as an optional replacement for these ports and avoiding use of a glib loop inside apps running on these platforms would be good, I think...

[Philippe Normand]

(In reply to comment #3)

> > I'm worried we may end up in that situation in
> > the gtk+ port, in some cases (say, the application hasn't run gtk_main(), but
> > is running the mainloop itself). Why not have ports that would like to use the
> > GStreamer backend run the glib main loop?
> 
> I'm not sure this would be an option for, say, the mac or win port :) It'd be
> nice to propose the GStreamer backend as an optional replacement for these
> ports and avoiding use of a glib loop inside apps running on these platforms
> would be good, I think...

Another approach would be to do the polling only on win and mac platforms (#if PLATFORM(WIN)...). So the GTK+ port wouldn't be affected, as webkitgtk apps use a glib mainloop in most (all?) cases.

BTW I tested the polling on my laptop and couldn't really notice a difference in terms of CPU usage, apart from the obvious overhead of wakeups :)

[Gustavo Noronha (kov)]

Eric, could you share some thoughts on this? =D

[Eric Carlson]

(In reply to comment #5)
> Eric, could you share some thoughts on this? =D

Well, I know *nothing* about the gtk main loop but I wonder if it is a good idea to do this by default, even on Mac in Windows. 

I assume anyone using WebKit with the GStreamer backend in their app will be building from source, so why not make it a compile time option?

[Eric Carlson]

(In reply to comment #4)
> BTW I tested the polling on my laptop and couldn't really notice a difference
> in terms of CPU usage, apart from the obvious overhead of wakeups :)

I'll bet you weren't monitoring battery consumption ;-)

[Gustavo Noronha (kov)]

(In reply to comment #6)
> (In reply to comment #5)
> > Eric, could you share some thoughts on this? =D
> 
> Well, I know *nothing* about the gtk main loop but I wonder if it is a good
> idea to do this by default, even on Mac in Windows. 

My question is more along the lines of - would it be a problem to add running the glib main loop along with the win/mac mainloop if you decide to build those ports with the GStreamer backend included? That would mean adding this call:

    g_main_context_iteration(NULL, FALSE);

To these functions:

[WebCore/platform/mac/EventLoopMac.mm]
void EventLoop::cycle()
{
    // FIXME: Should this use NSRunLoopCommonModes? Switching to NSRunLoopCommonModes causes Safari to hang in a tight loop.
    [NSApp setWindowsNeedUpdate:YES];
    NSEvent *event = [NSApp nextEventMatchingMask:NSAnyEventMask untilDate:[NSDate distantFuture] inMode:NSDefaultRunLoopMode dequeue:YES];
    [NSApp sendEvent:event];
}

[WebCore/platform/win/EventLoopWin.cpp]
void EventLoop::cycle()
{
    MSG msg;
    if (!GetMessage(&msg, 0, 0, 0)) {
        m_ended = true;
        return;
    }

    TranslateMessage(&msg);
    DispatchMessage(&msg);
}

> I assume anyone using WebKit with the GStreamer backend in their app will be
> building from source, so why not make it a compile time option?

Yes, build time option defeats runtime choice, IMO.

[Eric Carlson]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Eric, could you share some thoughts on this? =D
> > 
> > Well, I know *nothing* about the gtk main loop but I wonder if it is a good
> > idea to do this by default, even on Mac in Windows. 
> 
> My question is more along the lines of - would it be a problem to add running
> the glib main loop along with the win/mac mainloop if you decide to build those
> ports with the GStreamer backend included? That would mean adding this call:
> 
I don't know :-(

> > I assume anyone using WebKit with the GStreamer backend in their app will be
> > building from source, so why not make it a compile time option?
> 
> Yes, build time option defeats runtime choice, IMO.
My point was that *if* people that want to use the GStreamer backend are likely to build WebKit, they can easily enable polling if their app doesn't already run the gtk main loop.

[Philippe Normand]

Some news about this. I am now experimenting on a new approach, setting a synchronous handler on the gst bus with gst_bus_set_sync_handler() and processing the message in the mainloop using a one-shot timer. Songbird uses a similar approach.

It works fine if I use a g_timeout_add but not with the higher level WebCore Timer API... Trying to figure out why the timer callback is never fired :/

[Philippe Normand]

Created attachment 56226 [details]
proposed patch

I had some success with this patch. Would it make sense to extract it
from the player code? The EventLoop code mentionned by Gustavo doesn't
seem to be used by WebCore.

[Gustavo Noronha (kov)]

Comment on attachment 56226 [details]
proposed patch

I don't know a lot about the mac part of this, but it sounds sane. I do think this should be extracted from the player and added to the main loop part of the Mac port, though, since running the glib mainloop affects more and is useful to more than just the player.

[Philippe Normand]

Comment on attachment 56226 [details]
proposed patch

Thanks Gustavo, I'll see if I can do that :)

[Philippe Normand]

This bug is specific to Mac OS, a similar approach would be needed on Windows though.

[Philippe Normand]

Created attachment 59495 [details]
GLib runloop observer in WebView

[Gustavo Noronha (kov)]

Comment on attachment 59495 [details]
GLib runloop observer in WebView

glib-wise looks sane, might want to poke someone from the mac camp to look at the rest =)

[Eric Carlson]

Comment on attachment 59495 [details]
GLib runloop observer in WebView

The Mac specific code look OK to me, so marking r+ since kov signed off on the glib parts.

[Philippe Normand]

Landed in http://trac.webkit.org/changeset/62476
Thanks for the review :)

[WebKit Review Bot]

http://trac.webkit.org/changeset/62476 might have broken Leopard Intel Release (Tests)

[Philippe Normand]

(In reply to comment #19)
> http://trac.webkit.org/changeset/62476 might have broken Leopard Intel Release (Tests)

I don't see how this patch could break any test because the GSTREAMER feature is not enabled yet in the build.