[Nice] New API and refactor

[Kai.Vehmanen at nokia.com]

Hi,

On 07 May 2008,  Youness Alaoui wrote:
>As some of you know, I'm working on a refactor of libnice. I'm 
>not sure yet whether the external API will change much or not, 
>but the internal API and code needs major refactoring.

well, could you elaborate why the code needs major refactoring? 
I can understand you might have some style and API-usage issues (as
e.g. nice/stun is not using glib), but it does work and is very clean 
and tested code. What's the purpose of refactoring?

>The way I currently see it is : modular, layered, simple and beautiful!
				
Like the current STUN implementation? ;)

>let me be clear, do we want to have the ICE agent use the STUN 
>library to build messages, then send them on the socket, and 
>when it receives a message, try to parse it with the STUN 
>library to see if it's a valid stun message, then handle it, 
>otherwise, give it to the application (first design)...
[...]
>Or do we want to have the ICE agent build messages using the 
>STUN library, then tell the lib to send the message, and when 
>it receives a message, the STUN library handles it, if it's a 
>stun message, it will tell the ICE agent that it has an 
>inbound STUN message, if it's not, then it tell the ICE agent 
>that a data message was received.

The current implementation follows the latter approach. One design
goal was to keep the nice/stun module mainloop-independent (to 
make it easier to use it in other contexts). This had the 
design side-effect that nice/stun module couldn't wake up and
deliver incoming messages by itself. Thus the current implementation
(where nice/agent has to try and parse incoming messages
with STUN).

>Since I am better with code than words, let's talk C instead...
>I'm currently looking exclusively at the STUN library and API, 
>and here's what I have so far for a design (incomplete) :
[...]
>StunMessage * msg = stun_message_new(STUN_RFC3489BIS);

Oh please no! :) STUN messages are used on the hotpath and you 
shouldn't do dynamic memory allocs on it unless you have a pressing 
need. Some systems (like gstreamer) might provide a real-time safe 
memory allocator, but in a general purpose library avoiding these 
allocs is much, much preferred (the client app can do with a normal
allocator and still use the library). And in the case of STUN, 
these allocs can be easily minimized (as demonstrated by the current 
implementation).

>So now my question to you is : what do you think? How do you think the 

In the end I'm not opposed to refactoring, but I want to make
sure it's made for the right reasons. I'm worried that some
ugly-by-design code (because of requirements), is now turned 
beatiful, and in the process we no longer meet the original 
requirements. But if this worry is unwarrented, then no problem...

PS We can also discuss the reqs of course... 

br,
-- 
first.surname at nokia.com (Kai Vehmanen)