Summary: | Add async wrapper around nice_agent_gather_candidates() | ||
---|---|---|---|
Product: | nice | Reporter: | Philip Withnall <bugzilla> |
Component: | General | Assignee: | Olivier Crête <olivier.crete> |
Status: | RESOLVED MOVED | QA Contact: | |
Severity: | enhancement | ||
Priority: | medium | ||
Version: | unspecified | ||
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | |||
i915 platform: | i915 features: | ||
Attachments: | agent: Add an asychronous version of nice_agent_gather_candidates() |
Description
Philip Withnall
2014-09-22 14:29:49 UTC
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.