[Nice] complete example

Youness Alaoui youness.alaoui at collabora.co.uk
Mon Feb 4 12:32:28 PST 2013 

On 02/04/2013 03:10 PM, Bryce Allen wrote:
> I'm all for simplifying the licensing and I have permission, so here is
> a copy of nicetest.c licensed under MPL/LGPL just like the rest of
> libnice. I think that is enough so you can change the license on your
> derived version?
Yes, I believe that's enough. Thanks a lot.

> 
> The async version looks good, the only thing I noticed was the
> doc comment needs to be changed with the new source/file and program
> name. I'm in the process of testing it now on different hosts, I'll
> let you know if I run into any issues.
Good point, I forgot I renamed the file! I will change that as well as the
license and commit the new version.
Let me know if you find any bugs in my modified version.

> 
> -Bryce
> 
> 
> On Fri, 01 Feb 2013 19:35:53 -0500
> Youness Alaoui <youness.alaoui at collabora.co.uk> wrote:
>> Thanks!
>> I've finished cleaning up the code and integrated it into libnice.
>> I've pushed the changes here :
>> http://cgit.collabora.com/git/user/kakaroto/libnice.git/log/
>> First of all, I added a nice_agent_get_selected_pair because I think
>> it's much more efficient and logical to have that than being forced
>> to listen to the signal and keep track of the foundations.
>> I think I will also add a way to get the state without storing it
>> from the state-changed signal handler.
>> I think it would be a good idea to make the license match the rest of
>> the code from libnice. If you don't want to, then it's not a big deal.
>> So.. here's the changelog :
>> - Better to use '-4 -t A' options to 'host' command because it seems
>> to freeze for a long time otherwise. Added that to the comment.
>> - include <agent.h> instead of <nice/agent.h>, since $includedir/nice
>> is added to CFLAGS with pkg-config --cflags. (and it caused it to
>> take the agent.h from my installation rather than the local version)
>> - I used a table for transforming the candidate_type and state into
>> strings, much easier, although less robust, but since the data comes
>> from libnice, it's expected to be correct.
>> - Reordered the functions to be more linear/sequential.. the main
>> function at the top, then followed by whichever function gets
>> executed next.
>> - Removed the thread, and used the mainloop directly and make the
>> code work asynchronously in the callbacks.
>> - stun address is now optional, and I check validity of the
>> controlling argument.
>> - I use a g_io_channel for stdin and listen asynchronously to data
>> written on stdin.
>> - I use nice_agent_get_selected_pair to print the ip/port used, and
>> do it on the state-changed signal.
>> - I exit the app in case there's a failure to connect
>> - I use g_strsplit_set instead of parse_args (which I removed).
>> - I use g_strsplit instead of sscanf for parsing the candidate
>> - I loop through the candidate_type_name instead of doing an if/else
>> of each possibility (when parsing the candidate)
>> - I prefered using if (!function) rather than 'ok = function(); if
>> (!ok)'.. it's a personal choice though.
>> - Other small coding style changes to better match libnice's coding
>> style (or what it should be), like indentation, \n{ in a function
>> declaration, etc..
>>
>> If you could review the code I wrote and let me know if there's
>> anything you think needs fixing, let me know.
>>
>> Thanks!
>> Youness.
>>
>>
>>
>> On 02/01/2013 04:28 PM, Bryce Allen wrote:
>>> Patch attached. Found a bug in print_local_data when there is more
>>> than one local candidate (wasn't printing spaces in between). Also
>>> replaced the busy waiting with condition variables.
>>>
>>> -Bryce
>>>
>>> On Fri, 1 Feb 2013 11:24:32 -0600
>>> Bryce Allen <ballen at ci.uchicago.edu> wrote:
>>>> Hi Rohan,
>>>>
>>>> Thanks for testing it out and for the feedback!
>>>>
>>>> Re parse_args, I think it is correct as is. The line returned by
>>>> fgets may not contains '\n' (if it's very long), so it's not safe
>>>> to only check for '\n' as end of string, and the !isspace(*p)
>>>> already handles the '\n' as well as other whitespace to make sure
>>>> it won't be included in the arg:
>>>>
>>>>         // find end of current arg
>>>>         while (*p != '\0' && !isspace(*p)) { p++; }
>>>>         if (*p == '\0')
>>>>             break;
>>>>         *p = '\0';
>>>>
>>>> Were you getting a crash or a hang? It works for me when I copy
>>>> just the single line, but if I include extra empty lines it just
>>>> hangs. Also if your terminal wraps the input line, it will fail.
>>>> It would be nice to have a less fragile format - I think that is
>>>> what Youness was suggesting with the SDP format. I've attached a
>>>> new version that handles empty lines and re-prompts if the parse
>>>> fails, and fixes a memory leak in parse_remote_data.
>>>>
>>>> Re removing the thread for mainloop, I'm not sure the STUN
>>>> binding/keep alive requests will continue if the thread is quit
>>>> after negotiation is complete (during the send/receive phase). It
>>>> seems nice to manually drive the loop for tests and avoid the
>>>> complexity of threads, but I'm not sure it's what we want for an
>>>> example.
>>>>
>>>> -Bryce
>>>>
>>>>
>>>> On Fri, 1 Feb 2013 12:37:43 +0530
>>>> Rohan Garg <rohan16garg at gmail.com> wrote:
>>>>> Hey everyone :)
>>>>> I went over the program a bit, and I'm attaching a patch that
>>>>> fixes a couple of issues, just minor stuff regarding the
>>>>> thread_mainloop and making the code a wee bit more async.
>>>>>
>>>>> There also seems to be a bug on line 241 of the original code
>>>>> where it only stops at \0 , I believe that should be \n. When
>>>>> using \0 the program simply crashes at " remote_candidates =
>>>>> g_slist_reverse(remote_candidates); " in parse_remote_data.
>>>>>
>>>>>
>>>>> On Fri, Feb 1, 2013 at 6:05 AM, Youness Alaoui
>>>>>> I have a plan of adding a new API call for transforming the
>>>>>> candidates into SDP and to parse candidates from SDP. I think it
>>>>>> will help people, and I think it would be a good idea to port
>>>>>> your code to it. I'll let you know tomorrow what I find!
>>>>>>
>>>>> Could you explain this a bit on the list in another thread? :)
>>>>>
>>>>> Regards
>>>>> Rohan Garg
>>>>>
>>>>>> Thanks,
>>>>>> Youness.
>>>>>>
>>>>>>
>>>>>> On 01/31/2013 07:30 PM, Bryce Allen wrote:
>>>>>>> I expanded my nicetest.c into a complete example, using a simple
>>>>>>> serialization format and copying/pasting the data between
>>>>>>> clients. It's inspired by icedemo from pjnath, but I didn't find
>>>>>>> having separate commands for each phase useful for testing and
>>>>>>> it made reading the code more difficult, so I just have three
>>>>>>> phases:
>>>>>>>
>>>>>>> 1) gather candidates, print serialized local data
>>>>>>> 2) prompt user for remote data, parse and set when entered
>>>>>>> 3) if successful, prompt for data to send to remote end
>>>>>>>
>>>>>>> I'm probably missing some failure cases and some error checking,
>>>>>>> but it seems like it could be a useful addition to the main
>>>>>>> distribution after a fixup. I can re-license under LGPL/MPL if
>>>>>>> needed.
>>>>>>>
>>>>>>> Also please let me know if I'm doing anything fundamentally
>>>>>>> wrong here. I'm using pretty much the same flow for a real
>>>>>>> application, with condition variables to synchronize instead
>>>>>>> busy waiting.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Bryce
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> nice mailing list
>>>>>>> nice at lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/nice
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> nice mailing list
>>>>>> nice at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/nice
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> nice mailing list
>>>>>> nice at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/nice
>>
>>
>>
>>
>> _______________________________________________
>> nice mailing list
>> nice at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20130204/98289256/attachment-0001.pgp>

More information about the nice mailing list