Allow access to main context of ice events handler thread from plugins

[RSATom]

Sometimes it's better reuse loop for some lightweight tasks than create one else thread inside plugin.

[lminiero]

I'm not sure what you're trying to do here: I personally don't like the idea of plugins directly messing with core loops, as it means a single plugin can do a lot of damage. Can you please elaborate?

[RSATom]

setup_media/incoming_rtp/incoming_rtcp/hangup_media anyway are invoking from that thread. And in my case it's required to do additional RTP packets handling but with some timeout (for example detect missing incoming UDP for some period of time). Also in my case it's required to send some messages to that thread, but without access to main context, I will have to use mutex and it will kill RTP packets handling performance. So I would prefer add lightweight handlers directly on that thread.

Sorry if it's not clear enough. I can explain what exactly I'm trying to do if you like...

[lminiero]

I've checked the documentation for g_main_context_push_thread_default and it's still not clear to me what it does, and what could be the implications on existing plugins here. While we don't use Glib contexts/loops often in plugins, we do in some (e.g., VideoRoom for RTCP handling), and I fear this could break how they work. Pinging @atoppi too as he may have some additional insight.

[RSATom]

Some clarifications: first of all it's not possible to to attach any glib source to loop if there is no access to GMainContext used by that loop. And by default it's only possible to get access to default GMainContext (always accessible with g_main_context_default) but not context used by current thread (if there are many glib loops are running inside application, and it's the case of Janus). And many GSource creation functions in glib by default uses that main context (g_timeout_add_seconds or g_idle_add for example). So to be able attach GSource to context other than main, it's required to get pointer to it. And g_main_context_push_thread_default and g_main_context_get_thread_default can help with it without passing pointer to GMainContext explicitly. And I believe this can't affect anything in Janus, just because it doesn't use g_main_context_get_thread_default at all. It can affect some third party libs used by Janus if they are rely on g_main_context_get_thread_default, but if take into account Janus was working for a long time without g_main_context_push_thread_default without any problems - I believe it's safe...

[atoppi]

As far as I understand, the rationale here is getting access to the handle/loop context inside a plugin, in order to perform some activities on that specific context rather than using the default main context (e.g. the one returned by g_main_context_default).

According to glib docs, by calling g_main_context_push_thread_default():

This will cause certain asynchronous operations which are started in this thread to run under context and deliver their results to its main loop, rather than running under the global default context in the main thread

and

passing it a GMainContext which will be run by a GMainLoop in that thread, to set a new default context for all async operations in that thread

Hence if I understand correctly, we're basically setting the default context for threads to the loop context in place of the global default context and this could actually have some consequences.

In practice I guess nothing should change, since async operations happening in the plugins on that thread are being performed on the loop context due to

nice_agent_attach_recv(handle->agent, handle->stream_id, 1, g_main_loop_get_context(handle->mainloop),
        janus_ice_cb_nice_recv, pc);

There are only some exceptions in videoroom, lua and duktape plugins, but they create whole new threads, loops and contexts.

If the whole point of the PR is getting access to the loop context, I'd rather use an explicit pointer instead of subtly changing an existing behavior. IMHO we should first clarify the followings questions:

Should the plugins have access to that context? Is the loop context created in the core hidden by design?

[lminiero]

@RSATom was this closed by mistake?

[RSATom]

@lminiero No it was intended. I just was able to refactor audiobridge to state where I believe I don't need this patch anymore... So there is no need to bother you with it anymore.

[lminiero]

Got it! It definitely wasn't a bother, though. I think you raised a good point about allowing plugins to (possibly implicitly) leverage the existing loop one of the users is using for some functionality, which would indeed help do things from the same thread the core uses for its own callbacks related to that user. It's also true that we'd need to first better understand if this can cause problems, though: we'll keep the method you suggested in mind, to maybe add a new gateway callback function in the future to use this programmatically from plugins.

[RSATom]

Btw, it would be great to have documented what plugin callbacks from what exactly thread are invoking to be able to do some multithreading optimizations....

[lminiero]

Apart from create_session and query_session (which come from a thread in the core handling an API task), all other callbacks come from the thread running the loop associated with the user session, or at least they should. Some time ago we explicitly worked on this refactoring so that the handle loop would be the one firing those events to avoid race conditions in plugins.

[lminiero]

PS: obviously I was talking of handle/media related callbacks: handle_message and handle_admin_message come from a core API task/thread too.

[RSATom]

I can find all that info myself obviously (by debugging). The problem is since it's not documented - it's not reliable and can be changed at any moment...

[lminiero]

Well, since you've found all that info already, then maybe you can contribute documentation yourself :wink: You asked what threads they came from and I just told you. We don't plan to change that in the near future, since the plugins API is basically consolidated: when we do change something, it's always in a PR and always explicitly explained.

[RSATom]

I'm afraid my English is too bad for documentation :smiley: