[Nice] complete example

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

I found a way to fix the blocking read_lines :
http://cgit.collabora.com/git/user/kakaroto/libnice.git/commit/?id=030270775d9fc8d4f0381361b2ded0a38eb727b9

If you find other issues, let me know!

Thanks,
Youness

On 02/06/2013 08:09 PM, Youness Alaoui wrote:
> 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
> 
> 
> 
> 
> _______________________________________________
> 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/b04ed58d/attachment-0001.pgp>

More information about the nice mailing list