[Nice] complete example

Youness Alaoui youness.alaoui at collabora.co.uk
Wed Feb 6 17:09:27 PST 2013 

On 02/06/2013 07:31 PM, Bryce Allen wrote:
> Did some more testing on threaded and sdp examples in your latest
> trunk and found a few issues:
> 
> I think the g_thread_unref after g_thread_join is unnecessary, if I am
> reading this correctly:
> http://developer.gnome.org/glib/2.35/glib-Threads.html#g-thread-join
> Just noticed that in my original nicetest.c.
Good catch!

> 
> I would rename gloopthread to gexamplethread or similar; took me a
> minute to realize that was running example_thread and not the mainloop.
> 
Yeah, I prefer the mainloop to be running on the main thread. Also, it allowed
all the libnice-related code to be sequential in a function without all the
boilerplate around it. It just felt more natural to do it that way. You have a
good point though and I've renamed the variable name to make it less confusing.

> If I Ctrl-D one end, the next send (or even Ctrl-D) on the other end
> results in crash with this error:
> 
>  (process:63): GLib-CRITICAL **: g_main_loop_quit: assertion
>   `g_atomic_int_get (&loop->ref_count) > 0' failed
> 
Yeah, I noticed that one too, it's a race condition fixed by unreffing the
mainloop after joining the thread.

> Attached is a diff on threaded-example.c that fixes that - moved the
> loop unref after the join. The other end still doesn't exit until you
> enter more input, because it's blocking on the read and not checking
> the exit flag. Minor issue though, not sure it really matters for the
> example. I think sdp-example.c needs a similar change.
Cool. I applied the changes. Also renamed gloopthread into gexamplethread.
As for the io_stdin blocking call, I didn't notice, weird..  I'll try to see how
I can make it stop the blocking call to read_lines.

> 
> The exit signaling does work flawlessly with simple-example.
> 
> -Bryce
> 
> On Mon, 04 Feb 2013 20:18:53 -0500
> Youness Alaoui <youness.alaoui at collabora.co.uk> wrote:
>>
>> On 02/04/2013 06:52 PM, Bryce Allen wrote:
>>> I'm glad we could make a useful contribution! Libnice is saving us
>>> tons of effort trying to implement ICE ourselves.
>>
>> That's why I wrote it (well, that, and because I was told to :p)!
>> And yeah, ICE is a beast, not so easy to implement correctly.
>>
>>> Found another doc issue: Ctrl-D no longer works to quit when in
>>> send/recv mode. Could just change the text to 'Ctrl-C to quit'.
>> Even better, I made it quit when Ctrl-D is pressed :)
>>
>>>
>>> Neither version shows how to detect disconnects - if you close one
>>> end, the other end continues to accept input and try to send. Would
>>> be nice to add that to the examples, if it's within scope of the
>>> ICE layer.
>> Humm.. technically, there is no mechanism for that. But I was sure it
>> would work anyways because there's a keepalive every 30 seconds and
>> if we don't get a response for it (and there was no data received in
>> those 30 seconds), we consider the connection to have died... it
>> doesn't actually work.. I checked the code and I realized that only
>> happens when you use GOOGLE compatibility mode. When you use RFC
>> mode, the keepailve is a binding indication, not a binding request,
>> so there is no response to be received. Anyways, I added to the
>> example of simple way of doing that, when one exits (with Ctrl-D), it
>> will send a "\0" before quitting, and when we receive "\0", we quit.
>> http://cgit.collabora.com/git/user/kakaroto/libnice.git/commit/?id=818427d0b9e6d34d6ceb05cb73c1bf41446c9050
>>
>>
>>>
>>> -Bryce
>>>
>>> On Mon, 04 Feb 2013 15:32:28 -0500
>>> Youness Alaoui <youness.alaoui at collabora.co.uk> wrote:
>>>> 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
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> 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/20130206/9b12da55/attachment.pgp>

More information about the nice mailing list