[Nice] API thoughts

[Olivier Crête]

On Tue, 2008-04-22 at 18:48 +0100, Dafydd Harries wrote:
> Ar 17/04/2008 am 09:30, ysgrifennodd Olivier Crête:
> > 
> > On Thu, 2008-04-17 at 09:53 +0300, Kai.Vehmanen at nokia.com wrote:
> > > Hi,
> > > 
> > > On 15 April 2008,  Dafydd Harries wrote:
> > > >> Could we somehow support both models? Or is there a way to run a set 
> > > >> of GMainContexts in a single GMainloop (without writing a custom 
> > > >
> > > >Oh, perhaps I wasn't clear. I absolutely want to have a good 
> > > >API for single-threaded applications too.
> > > 
> > > aa, ok, then we are on the same page! :)
> > > 
> > > >> Hmm, yes, this should be doable. Although the locking has to be done 
> > > >> very carefully (inbound STUN messages affect agent-wide state 
> > > >> in many ways, see conncheck.c:conn_check_handle_inbound_stun()).
> > > >
> > > >Well, I was thinking that the initial approach could just be 
> > > >to make any handling of inbound STUN packets mutually 
> > > >exclusive. We can make the locking finer later, or perhaps 
> > > >look at using some kind of lock-free update algorithm.
> > > 
> > > Yes, but you also need to lock the state in all public API 
> > > functions (e.g. in case remote peer restarts the ICE
> > > negotiation), plus in the timer callbacks (which are running
> > > in the "shared context"). But agreed, still very much doable.
> > 
> > Youness already did a branch to do that, its on the monkey.
> 
> Some comments:

> Or even:
> 
>   #define WITH_LOCK(function, t, params, args) \
>       t nice_agent_##function (params) \
>       { \
>           t ret; \
>           \
>           g_mutex_lock (agent->mutex); \
>           ret = function (args); \
>           g_mutex_unlock (agent->mutex);\
>           return ret; \
>       }

Eurk macro...


> I think the first is probably nicer though.
> 
> "Detach" has only one "t". :)
> 
> Could you add a NICE_COMPATIBILITY_LAST = NICE_COMPATIBILITY_MSN, and use that
      * in the compatibility property declaration? This will make it
        easier to add
      * other modes later.
      * 
      * I don't understand the changes that look like this:
      * 
>        res = priv_conn_check_tick_unlocked ((gpointer) agent);
>    
>        /* step: schedule timer if not running yet */
>   -    if (res && agent->conncheck_timer_id == 0) 
>   -      agent->conncheck_timer_id = 
>   -»·······g_timeout_add (agent->timer_ta, priv_conn_check_tick, agent);
>   +    if (res && agent->conncheck_timer_id == 0) {
>   +      GSource *source = g_timeout_source_new (agent->timer_ta);
>   +      g_source_set_callback (source, priv_conn_check_tick, agent, NULL);
>   +      agent->conncheck_timer_id = g_source_attach (source, agent->main_context);
>   +      g_source_unref (source);
>   +    }
> 
> Could you explain why they're necessary?

What if you use it with a Qt app or a TCL app, anything used in a gst
element should not rely on the presence or usefulness of the default
main context. 

> I'm not sure we should add the main context and the compatibility as
> parameters to nice_agent_new. In fact, I'm not sure we should have a
> nice_agent_new at all; it should default to creating a sane socket factory if
> you didn't give it one, to using ID19 compatibility, and to using NULL for the
> main context (i.e. the default main context). I.e. you can just call
> g_object_new (NICE_TYPE_AGENT, NULL) in most cases.

I think you need the maincontext if to start the timers and stuff.. and
you probably have to know about the compatibility too to know how to
start stuff and which candidates to provide?

-- 
Olivier Crête
olivier.crete at collabora.co.uk
Collabora Ltd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/nice/attachments/20080422/bc284a18/attachment.pgp