[Nice] API thoughts

[Youness Alaoui]

Dafydd Harries wrote:
> 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.
>
>   
can be done without making the code ugly :p I'll use gotos instead.
>>> 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?)>
>
>   
thought of factoring it, but only in 3 places, I thought it wasn't 
necessary.. if you think 3 is enough, then will do.
>>> 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 your point here...
> I don't understand what you mean by "how to start stuff and which candidates
> to provide">
>
>   
if you're in google/msn compatibility, it's candidate_id+password+qvalue 
per candidate...
if it's ID19, it's foundation+priority and a generic ufrag+password..
the API to use is bound to the compatibility mode.
> Regardless, I don't think nice_agent_new makes the API any nicer to use, just
> bigger.
>
>   

maybe not 'nicer', but 'easier to understand'.. at least, form my point 
of view..
and I don't think that nice_agent_new(factory, NULL, 
NICE_COMPATIBILITY_ID19); makes it much bigger/uglier..
what would you suggest instead ?