[Nice] complete example

Bryce Allen ballen at ci.uchicago.edu
Wed Feb 6 16:31:16 PST 2013 

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.

I would rename gloopthread to gexamplethread or similar; took me a
minute to realize that was running example_thread and not the mainloop.

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

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.

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
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threaded-example-exit-fix.diff
Type: text/x-patch
Size: 423 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/nice/attachments/20130206/b09479eb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/nice/attachments/20130206/b09479eb/attachment.pgp>

More information about the nice mailing list