[Nice] API thoughts

[Dafydd Harries]

Ar 22/04/2008 am 14:06, ysgrifennodd 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...

Well, my motivation is to reduce bugs by making it impossible to forget an
unlock call.

> > 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. 

Oh, right, it's just changing the code to create a timeout for the non-default
main context.

Given that this seems to be done in at least three different places, please
factor this code out into a separate function (timeout_add_with_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?

If you are running a mainloop, you need to know which context to start it
with, so you need to think about it anyway.

I don't understand what you mean by "how to start stuff and which candidates
to provide">

Regardless, I don't think nice_agent_new makes the API any nicer to use, just
bigger.

-- 
Dafydd