Created attachment 106683 [details] [review] agent: Add an asychronous version of nice_agent_gather_candidates() WIP patch attached to add a GIO-style asynchronous wrapper around the libnice candidate gathering and connection establishment process. This does everything from nice_agent_gather_candidates() through to adding remote candidates and TURN servers, and establishing a TCP or UDP connection, at which point the asynchronous task completes. It does not handle emitting local candidates or adding remote candidates (though it has API which must be notified when the final remote candidate has been added to the NiceAgent). It also does not handle local or remote credentials. I wonder if it would be a good idea to incorporate those into the API as callbacks (which would be guaranteed to be emitted in the GTask’s main context, to avoid the g_main_context_invoke() dance which we have to do internally). For this reason, I’m declaring the API in this patch as a work in progress, and feedback would be greatly appreciated. Basically I hope this new API can become the default all-in-one easy way to use libnice, and the existing API can be reserved for more advanced use cases. http://cgit.collabora.com/git/user/pwith/libnice.git/log/?h=async-gather-candidates There are a few comments about what work still needs to be done in the commit message. I don’t currently have time to finish this off, but it’s close — the biggest remaining task is unit testing. :-(
Comment on attachment 106683 [details] [review] agent: Add an asychronous version of nice_agent_gather_candidates() Review of attachment 106683 [details] [review]: ----------------------------------------------------------------- ::: agent/async.h @@ +47,5 @@ > + > +GTask * > +nice_agent_gather_candidates_async (NiceAgent *agent, guint stream_id, > + guint component_id, > + GSocketProtocol protocol, Returning the GTask here is super strange.. Maybe it should be some "hidden" typedef (even if underneath it's actually the GTask. Also, gathering is a bit confusing, at this does more than gathering candidates, it does the whole connection process. I can't help but think that the nice_agent_gather_got_*() functions should just be called on the NiceAgent directly somehow.
(In reply to comment #1) > Comment on attachment 106683 [details] [review] [review] > agent: Add an asychronous version of nice_agent_gather_candidates() > > Review of attachment 106683 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: agent/async.h > @@ +47,5 @@ > > + > > +GTask * > > +nice_agent_gather_candidates_async (NiceAgent *agent, guint stream_id, > > + guint component_id, > > + GSocketProtocol protocol, > > Returning the GTask here is super strange.. Maybe it should be some "hidden" > typedef (even if underneath it's actually the GTask. > Also, gathering is a bit confusing, at this does more than gathering > candidates, it does the whole connection process. I can't help but think > that the nice_agent_gather_got_*() functions should just be called on the > NiceAgent directly somehow. Yeah, as I said, the API is a bit of a work in progress and I’m not happy with it. But at least it’s here and hackable on. I see two options which are equally appealing: 1. Integrate the whole lot completely into NiceAgent, so the GTask becomes private data in the NiceAgent, and the nice_agent_gathering_got_*() methods are integrated better. 2. Expose a new NiceStream object, and have nice_agent_gather_candidates_async()[1] return a new instance of that object. So it would additionally call nice_agent_add_stream(), and have callbacks (or signals) for emitting local candidates, credentials, etc., and the whole object would be tied to a specific GMainContext to avoid the g_main_context_invoke() dance. It could then have a nice_stream_dup_io_stream() method which returns a NiceIOStream for the given component ID. Overall, I guess we should decide what we’re looking for in a high-level API for libnice. My first thought is the following hierarchy: NiceAgent : GObject → NiceStream : GObject (object wrapper around streams) → NiceIOStream : GIOStream (object wrapper around components) where NiceStream is designed to expose as little API as possible, and all the low-level API is constrained to NiceAgent. Ideally, NiceStream would just have: nice_stream_connect_[async|finish](callback new_local_candidate, callback got_local_credentials) nice_stream_close_[async|finish]() nice_stream_add_remote_candidate() nice_stream_got_final_remote_candidate() nice_stream_add_relay_server() nice_stream_got_final_relay_server() nice_stream_set_remote_credentials() and perhaps something for ICE restarts? I know less about how they should fit in. So the main differences in this high-level API from the current one are: • All send/recv operations are left to NiceIOStream. • All state management is done internally, so the calling code does not have to duplicate any of libnice’s internal state (e.g. whether the final candidate/relay has been set, whether the component is in STATE_FAILED but still waiting for more candidates, etc.). • Increased dependence on self-contained GTasks and callbacks which have a strictly defined GMainContext to be emitted in. • Reduced dependence on signals, and the signals which are emitted are specific to a single stream (rather than having stream_id and component_id parameters which require demultiplexing). Sorry for the long rambly comment. Hopefully this makes sense and fits in with your ideas? [1]: Which we should probably rename to nice_agent_connect_async() or similar.
Migrated to Phabricator: http://phabricator.freedesktop.org/T106
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.