liblo-devel Mailing List for LibLO

Fantastic, thanks for your reactivity guys!

On Sun, Feb 24, 2008 at 8:31 PM, Stephen Sinclair <rad...@gm...>
wrote:

> On Sun, Feb 24, 2008 at 1:55 PM, Steve Harris
> <S.W...@ec...> wrote:
> > Good catch.
> >
> >  You might want to check out the package that this code from, if the
> >  errors weren't thing I introduced when porting it then they might
> >  appreciate a patch.
> >
> >  I don't remember what package it was, but it should say in the
> comments.
>
> Cool, good idea.  I just emailed the contact address for libOSC++.
> The patch applied fairly cleanly to their codebase so it wasn't much
> work to generate a patch for them.
>
>
> Steve
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> liblo-devel mailing list
> lib...@li...
> https://lists.sourceforge.net/lists/listinfo/liblo-devel
>

Hi,

Some really good points here.
Sounds like everyone's interested in this functionality.

On Fri, Feb 22, 2008 at 5:40 AM, Steve Harris
<S.W...@ec...> wrote:
>  I think I would want an advertising API, something like
>
>  lo_server_thread_advertise(st, id, ...)
>  eg, for jamin:
>   char *extra = g_strdup_printf("file=%s", file_name);
>   lo_server_thread_advertise(st, "http://jamin.sf.net/osc_control",
>  extra);
>
>  MDNS records have a field for putting ID information, like
>  uri=http://... file=...

I definitely like the idea of providing a URL for information on the
expected OSC namespace.
This might be a very useful alternative (or even in addition to)
automatic namespace querying.


>  That implies we'd also want a lo_server_discover() function that scans
>  the available MDNS records, find the OSC ones, does some pattern
>  matching and returns an array of lo_addresses and "extra data" strings
>  or something similar.

Could be useful, sure.  I think it's a distinct functionality though,
being discoverable, and doing the discovering.
But certainly the two go together.

>  lo_server_thread_stop and _start could implicitly publish and
>  unpublish the advert, and lo_server_thread_free() implicitly free the
>  associated data.

It's probably how I'd do it.  I guess the question is more whether it
should be done by default, or if it should require enabling by the
user.
And should it be enabled on a per-server basis, or globally?

I think I'd vote on doing it automatically, unless explicitly disabled
at run-time.  (or configure-time of course.)


>  > The patch makes avahi support optional with a configure flag.
>
>  It's probably a good idea to autodetect, and have -enable and --
>  disable flags, it doesn't really cost anything if it's compiled in but
>  not used.

Sure.


>  > - if an OSC service is announced, does this imply that it should
>  > respond to certain standard messages? as far as I know no such
>  > standard exists.
>
>  Not as far as I know wither, it would have to be per-application and/
>  or emerging consensus.

I guess that discussion belongs on the OSC list.
It's about time someone brought it up again anyway.. :)
Namespace discovery is really is an outstanding issue with OSC, and
the longer it remains unattended, the more people will implement
various incompatible solutions.


>  > Since it's optional at configure time, would it be harmful to have
>  > lo_servers simply always announce on zeroconf if this option is
>  > enabled?
>
>  I think to be useful you have to give them some ID information, so a
>  dedicated client can find a (set of) dedicated server(s). You can
>  imagine for eg. the superlooper client autoconnecting if there's only
>  one server on the local network and put up a request dialogue if there
>  more than one.
>
>  Just knowing that there's some OSC server at osc.udp://
>  192.168.0.1:9999/ is not terribly useful.

Absolutely.  I'll have to read up a bit on avahi/zeroconf to figure
out how to add more information to the announce.
Patches would be accepted here .. ;-)  But it's on my todo list, for sure.



On Fri, Feb 22, 2008 at 5:44 AM, Camille Troillard
<ca...@os...> wrote:
> Is there a specific reason to add "overload" methods to server
> instantiation?
> I think Bonjour service publication is relatively independant from server
> creation, and I would suggest something like:
>
> lo_zeroconf_publish (lo_server *)
> lo_zeroconf_unpublish (lo_server *)

I think your idea looks cleaner, admittedly, but liblo is (in general)
organized such that functions affecting a particular data structure
start with the name of that data structure.  So, since a zeroconf
announce is tied to a lo_server, the function should start with that
in the name, I think.

(Yes, lo_send_message() is a exception to this rule!  arrr..)

On the other hand, if it were made into a global flag that all
lo_servers respect, perhaps the functions could be:

lo_zeroconf_init();
lo_zeroconf_free();

The extra information requested by Steve could be passed to
lo_zeroconf_init()... no wait that doesn't work because each lo_server
will have its own URI.  I guess some lo_server_* function will have to
exist to support this.


> > - Is adding an extra function to initialize zeroconf announcement
> > adding extra complexity?
>
> IMHO, I think so.
> It also adds the need for the developper to learn another API calls, etc.
> I guess the default behaviour would be to publish Bonjour service every a
> server is created.
>
> It would be great to configure the liblo library with a function to disable
> Bonjour.
> The configuration could be done like a global register.

Agreed on both counts.
Well, whether or not configuration should be global is unclear to me.


> If it is not desired to publish every servers (which I think needs to be
> debated), then the default value for the global parameters would be to NOT
> publish the service.  The user would just have to change that switch to
> publish the subsequently created servers.

I'm having a hard time coming up with situations where one would not
like to publish the service.. ;-)
I think doing it by default is probably okay.
Users who are aware of the zeroconf functionality may wish to disable
it, but otherwise I think it's probably safe to use.



On Fri, Feb 22, 2008 at 6:35 AM, Lars Luthman <lar...@gm...> wrote:
>  If someone does decide to implement namespace querying it might be a
>  good idea to make it as similar to the D-Bus Introspection API as
>  possible (see
>  http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-introspectable ).
>
>  D-Bus and OSC are already very similar, and lots of programs for Linux
>  (and other GNOME/KDE-using systems) already use D-Bus, so having the
>  introspection methods use the same concepts would be nice.
>
>  Yeah, XML is ugly, but it's there and it obviously works reasonably
>  well.


I don't know enough about D-Bus to make an intelligent comment here,
but your comparison with OSC is interesting.
I think I'll re-post some of this stuff on the OSC list and see if
anyone has any comments there.
What I've read so far about the proposed namespace querying stuff
actually does already look very similar to that introspection XML,
since it's basically a set of names and parameter types.

Another thing to consider is making zeroconf support compatible with
Rémy Müller's "oscbonjour" object for Max/MSP.
(I think the proposed patch is already fairly compatible, but I'd have
to verify.)


Steve

On Sun, Feb 24, 2008 at 1:55 PM, Steve Harris
<S.W...@ec...> wrote:
> Good catch.
>
>  You might want to check out the package that this code from, if the
>  errors weren't thing I introduced when porting it then they might
>  appreciate a patch.
>
>  I don't remember what package it was, but it should say in the comments.

Cool, good idea.  I just emailed the contact address for libOSC++.
The patch applied fairly cleanly to their codebase so it wasn't much
work to generate a patch for them.


Steve

Good catch.

You might want to check out the package that this code from, if the  
errors weren't thing I introduced when porting it then they might  
appreciate a patch.

I don't remember what package it was, but it should say in the comments.

- Steve

On 24 Feb 2008, at 18:53, Stephen Sinclair wrote:

> On Sun, Feb 24, 2008 at 1:49 PM, Stephen Sinclair  
> <rad...@gm...> wrote:
>> So the /* problem should probably be addressed.
>>
>
> And apparently it wasn't too difficult.
> Here's a revised patch:
>
> Index: pattern_match.c
> ===================================================================
> --- pattern_match.c	(revision 100)
> +++ pattern_match.c	(working copy)
> @@ -97,7 +97,7 @@
>         switch (c = *p++) {
>
>             case '*':
> -                while (*p == '*')
> +                while (*p == '*' && *p != '/')
>                     p++;
>
>                 if (!*p)
> @@ -191,7 +191,7 @@
>
>                     c = *p++;
>
> -                    while (*p) {
> +                    while (c) {
>                         if (c == ',') {
>                             if(lo_pattern_match(str, remainder)) {
>                                 return true;
> @@ -206,20 +206,22 @@
>                         }
>                         else if (c == '}') {
>                             // continue normal pattern matching
> -                            if(!*p++)
> -                                return false;
> +                            if (!*p && !*str) return true;
> +                            str--; // str is incremented again below
>                             break;
>                         } else if (c == *str) {
>                             str++;
>                             if (!*str && *remainder)
>                                 return false;
> -                            // p++;
>                         } else { // skip to next comma
>                             str = place;
>                             while (*p != ',' && *p != '}' && *p)
>                                 p++;
>                             if (*p == ',')
>                                 p++;
> +                            else if (*p == '}') {
> +                                return false;
> +                            }
>                         }
>                         c = *p++;
>                     }
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> liblo-devel mailing list
> lib...@li...
> https://lists.sourceforge.net/lists/listinfo/liblo-devel

On Sun, Feb 24, 2008 at 1:49 PM, Stephen Sinclair <rad...@gm...> wrote:
>  So the /* problem should probably be addressed.
>

And apparently it wasn't too difficult.
Here's a revised patch:

Index: pattern_match.c
===================================================================
--- pattern_match.c	(revision 100)
+++ pattern_match.c	(working copy)
@@ -97,7 +97,7 @@
         switch (c = *p++) {

             case '*':
-                while (*p == '*')
+                while (*p == '*' && *p != '/')
                     p++;

                 if (!*p)
@@ -191,7 +191,7 @@

                     c = *p++;

-                    while (*p) {
+                    while (c) {
                         if (c == ',') {
                             if(lo_pattern_match(str, remainder)) {
                                 return true;
@@ -206,20 +206,22 @@
                         }
                         else if (c == '}') {
                             // continue normal pattern matching
-                            if(!*p++)
-                                return false;
+                            if (!*p && !*str) return true;
+                            str--; // str is incremented again below
                             break;
                         } else if (c == *str) {
                             str++;
                             if (!*str && *remainder)
                                 return false;
-                            // p++;
                         } else { // skip to next comma
                             str = place;
                             while (*p != ',' && *p != '}' && *p)
                                 p++;
                             if (*p == ',')
                                 p++;
+                            else if (*p == '}') {
+                                return false;
+                            }
                         }
                         c = *p++;
                     }

On Sun, Feb 24, 2008 at 1:47 PM, Stephen Sinclair <rad...@gm...> wrote:
> On Sun, Feb 24, 2008 at 11:54 AM, Stephen Sinclair <rad...@gm...> wrote:
>  >  It seems that cases 1, 4, and 5 are in error.  Likely to do with the
>  >  {} curly bracket processing, since which handler is called is
>  >  obviously related to the order in which they appear.
>  >  Another case I noticed, with handlers for /foo/bar and /foo/bing, is
>  >  that sending "/*" triggers both handlers, when I believe perhaps it
>  >  should not trigger either.  I may be wrong but it's my understanding
>  >  that wildcards should only match up to a '/' character.

Sorry, didn't mean to imply that my patch fixes the /* problem.  It
only fixes the {foo,bar} problem.
According to http://opensoundcontrol.org/spec-1_0

   1.  The OSC Address and the OSC Address Pattern contain the same
number of parts; and
   2. Each part of the OSC Address Pattern matches the corresponding
part of the OSC Address.

So the /* problem should probably be addressed.


Steve

On Sun, Feb 24, 2008 at 11:54 AM, Stephen Sinclair <rad...@gm...> wrote:
>  It seems that cases 1, 4, and 5 are in error.  Likely to do with the
>  {} curly bracket processing, since which handler is called is
>  obviously related to the order in which they appear.
>  Another case I noticed, with handlers for /foo/bar and /foo/bing, is
>  that sending "/*" triggers both handlers, when I believe perhaps it
>  should not trigger either.  I may be wrong but it's my understanding
>  that wildcards should only match up to a '/' character.

The following patch fixes this problem.
I'll commit it if there aren't any issues with it.

Steve


Index: src/pattern_match.c
===================================================================
--- src/pattern_match.c (revision 100)
+++ src/pattern_match.c (working copy)
@@ -191,7 +191,7 @@

                     c = *p++;

-                    while (*p) {
+                    while (c) {
                         if (c == ',') {
                             if(lo_pattern_match(str, remainder)) {
                                 return true;
@@ -206,20 +206,22 @@
                         }
                         else if (c == '}') {
                             // continue normal pattern matching
-                            if(!*p++)
-                                return false;
+                            if (!*p && !*str) return true;
+                            str--; // str is incremented again below
                             break;
                         } else if (c == *str) {
                             str++;
                             if (!*str && *remainder)
                                 return false;
-                            // p++;
                         } else { // skip to next comma
                             str = place;
                             while (*p != ',' && *p != '}' && *p)
                                 p++;
                             if (*p == ',')
                                 p++;
+                            else if (*p == '}') {
+                                return false;
+                            }
                         }
                         c = *p++;
                     }

Camille,

On Sun, Feb 24, 2008 at 10:52 AM, Camille Troillard
<ca...@os...> wrote:
> Hi,
>
> I have tested pattern matching in liblo, but it was not successful.
> For example I send a message like "/{foo,bar} ,i 1" to a server which has
> already two osc methods registered as "/foo" and "/bar".  I was expecting
> the handlers of those two methods to be called, but rather the wildcard
> handler is called with the path "/{foo,bar}".
>
> Maybe there is something I am missing, so I would appreciate greatly if
> someone could give me some info about the status of pattern matching in
> liblo.

You're right, there's an issue there.
I just did some tests with example_server modified to have /foo and
/bar handlers and got the following results:

(where "test -> handlers called")

1) /{foo,bar}   -> generic, /foo
2) /{fo,ba}*    -> generic, /foo, /bar
3) /*           -> generic, /foo, /bar
4) /[fb]{oo,ar} -> generic, /foo
5) /[fb]{ar,oo} -> generic, /bar
6) /*{ar,oo}    -> generic, /foo, /bar

It seems that cases 1, 4, and 5 are in error.  Likely to do with the
{} curly bracket processing, since which handler is called is
obviously related to the order in which they appear.
Another case I noticed, with handlers for /foo/bar and /foo/bing, is
that sending "/*" triggers both handlers, when I believe perhaps it
should not trigger either.  I may be wrong but it's my understanding
that wildcards should only match up to a '/' character.


Steve

Hi,
I have tested pattern matching in liblo, but it was not successful.
For example I send a message like "/{foo,bar} ,i 1" to a server which has
already two osc methods registered as "/foo" and "/bar".  I was expecting
the handlers of those two methods to be called, but rather the wildcard
handler is called with the path "/{foo,bar}".

Maybe there is something I am missing, so I would appreciate greatly if
someone could give me some info about the status of pattern matching in
liblo.


Best,
Cam

On Thu, 2008-02-21 at 23:25 -0500, Stephen Sinclair wrote:
> - if an OSC service is announced, does this imply that it should
> respond to certain standard messages? as far as I know no such
> standard exists.
> I know there have been some suggestions for namespace querying,
> including my own paper touching on the subject, but i don't know if
> one has ever become a defacto standard.  i imagine that implementing
> something like this in liblo might actually have potential to help
> boister a standard for namespace queries.

If someone does decide to implement namespace querying it might be a
good idea to make it as similar to the D-Bus Introspection API as
possible (see
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-introspectable ). 

D-Bus and OSC are already very similar, and lots of programs for Linux
(and other GNOME/KDE-using systems) already use D-Bus, so having the
introspection methods use the same concepts would be nice.

Yeah, XML is ugly, but it's there and it obviously works reasonably
well.


--ll

Hi Steve,

On Fri, Feb 22, 2008 at 5:25 AM, Stephen Sinclair <rad...@gm...>
wrote:

>
> There is a patch sitting in the patch tracker which adds support for
> avahi, which is a library enabling zeroconf announcement.
> In other words, a liblo program using it would be able to announce
> itself on the zeroconf (aka Bonjour) service as "_udp._osc".


Very interesting stuff.
I have implemented Bonjour support in my application, and I welcome this
addition to liblo.


The implementation seems fine, (I have tested it), but I was wondering
> if anyone had comments regarding the suggested API for this.
> To summarize, it would add the following functions:
>
> lo_server_avahi_init
> lo_server_avahi_free
> lo_server_thread_avahi_init
> lo_server_thread_avahi_free
>
> The patch makes avahi support optional with a configure flag.


Is there a specific reason to add "overload" methods to server
instantiation?
I think Bonjour service publication is relatively independant from server
creation, and I would suggest something like:

lo_zeroconf_publish (lo_server *)
lo_zeroconf_unpublish (lo_server *)



My initial reaction is that it's a pretty great idea to support
> zeroconf for OSC-related services.
> Some issues that I thought of:
>
> - if an OSC service is announced, does this imply that it should
> respond to certain standard messages? as far as I know no such
> standard exists.
> I know there have been some suggestions for namespace querying,
> including my own paper touching on the subject, but i don't know if
> one has ever become a defacto standard.  i imagine that implementing
> something like this in liblo might actually have potential to help
> boister a standard for namespace queries.


I would definitely vote for namespace querying support.




> - avahi, as far as i know, is a linux library, while on OS X the
> Bonjour framework takes care of this task.
> I suggest naming these functions lo_server_zeroconf_init() instead, to
> be more platform-agnostic.  (ps., i am currently aware of some
> compilation problems on OS X, working on it..)


right, please use something more standard based rather than implementation
based.



- Is adding an extra function to initialize zeroconf announcement
> adding extra complexity?


IMHO, I think so.
It also adds the need for the developper to learn another API calls, etc.
I guess the default behaviour would be to publish Bonjour service every a
server is created.

It would be great to configure the liblo library with a function to disable
Bonjour.
The configuration could be done like a global register.




> Since it's optional at configure time, would it be harmful to have
> lo_servers simply always announce on zeroconf if this option is
> enabled?
> I imagine that globally enabling zeroconf for all lo_servers might not
> be desired.. so I'm still debating this in my head a bit.  A goal of
> liblo is to reduce the number of required API calls for basic OSC
> functionality, so in a way it would be nice.


If it is not desired to publish every servers (which I think needs to be
debated), then the default value for the global parameters would be to NOT
publish the service.  The user would just have to change that switch to
publish the subsequently created servers.


Best,
Cam

On 22 Feb 2008, at 04:25, Stephen Sinclair wrote:
> Hello,
>
> There is a patch sitting in the patch tracker which adds support for
> avahi, which is a library enabling zeroconf announcement.
> In other words, a liblo program using it would be able to announce
> itself on the zeroconf (aka Bonjour) service as "_udp._osc".
>
> The implementation seems fine, (I have tested it), but I was wondering
> if anyone had comments regarding the suggested API for this.
> To summarize, it would add the following functions:
>
> lo_server_avahi_init
> lo_server_avahi_free
> lo_server_thread_avahi_init
> lo_server_thread_avahi_free

I use MDNS/Avahi a lot at work, and it's great. So a +1 from me.

I think I would want an advertising API, something like

lo_server_thread_advertise(st, id, ...)
eg, for jamin:
  char *extra = g_strdup_printf("file=%s", file_name);
  lo_server_thread_advertise(st, "http://jamin.sf.net/osc_control",  
extra);

MDNS records have a field for putting ID information, like  
uri=http://... file=...

That implies we'd also want a lo_server_discover() function that scans  
the available MDNS records, find the OSC ones, does some pattern  
matching and returns an array of lo_addresses and "extra data" strings  
or something similar.

lo_server_thread_stop and _start could implicitly publish and  
unpublish the advert, and lo_server_thread_free() implicitly free the  
associated data.

> The patch makes avahi support optional with a configure flag.

It's probably a good idea to autodetect, and have -enable and -- 
disable flags, it doesn't really cost anything if it's compiled in but  
not used.

> - if an OSC service is announced, does this imply that it should
> respond to certain standard messages? as far as I know no such
> standard exists.

Not as far as I know wither, it would have to be per-application and/ 
or emerging consensus.

> I know there have been some suggestions for namespace querying,
> including my own paper touching on the subject, but i don't know if
> one has ever become a defacto standard.  i imagine that implementing
> something like this in liblo might actually have potential to help
> boister a standard for namespace queries.

Yep, definitely.

> - avahi, as far as i know, is a linux library, while on OS X the
> Bonjour framework takes care of this task.
> I suggest naming these functions lo_server_zeroconf_init() instead, to
> be more platform-agnostic.  (ps., i am currently aware of some
> compilation problems on OS X, working on it..)

Yes. I've tried to build avahi on the mac and it's a pain.

> - Is adding an extra function to initialize zeroconf announcement
> adding extra complexity?

Yes.

> Since it's optional at configure time, would it be harmful to have
> lo_servers simply always announce on zeroconf if this option is
> enabled?

I think to be useful you have to give them some ID information, so a  
dedicated client can find a (set of) dedicated server(s). You can  
imagine for eg. the superlooper client autoconnecting if there's only  
one server on the local network and put up a request dialogue if there  
more than one.

Just knowing that there's some OSC server at osc.udp:// 
192.168.0.1:9999/ is not terribly useful.

- Steve

Hello,

There is a patch sitting in the patch tracker which adds support for
avahi, which is a library enabling zeroconf announcement.
In other words, a liblo program using it would be able to announce
itself on the zeroconf (aka Bonjour) service as "_udp._osc".

The implementation seems fine, (I have tested it), but I was wondering
if anyone had comments regarding the suggested API for this.
To summarize, it would add the following functions:

lo_server_avahi_init
lo_server_avahi_free
lo_server_thread_avahi_init
lo_server_thread_avahi_free

The patch makes avahi support optional with a configure flag.

My initial reaction is that it's a pretty great idea to support
zeroconf for OSC-related services.
Some issues that I thought of:

- if an OSC service is announced, does this imply that it should
respond to certain standard messages? as far as I know no such
standard exists.
I know there have been some suggestions for namespace querying,
including my own paper touching on the subject, but i don't know if
one has ever become a defacto standard.  i imagine that implementing
something like this in liblo might actually have potential to help
boister a standard for namespace queries.

- avahi, as far as i know, is a linux library, while on OS X the
Bonjour framework takes care of this task.
I suggest naming these functions lo_server_zeroconf_init() instead, to
be more platform-agnostic.  (ps., i am currently aware of some
compilation problems on OS X, working on it..)

- Is adding an extra function to initialize zeroconf announcement
adding extra complexity?
Since it's optional at configure time, would it be harmful to have
lo_servers simply always announce on zeroconf if this option is
enabled?
I imagine that globally enabling zeroconf for all lo_servers might not
be desired.. so I'm still debating this in my head a bit.  A goal of
liblo is to reduce the number of required API calls for basic OSC
functionality, so in a way it would be nice.

thoughts?

Steve

On 13 Feb 2008, at 14:28, Stephen Sinclair wrote:
>
>> Could scan the messages already in the bundle at insertion time to  
>> see
>> if they're already there. Over a certain length its more efficient  
>> not
>> to scan, and just assume there might be duplicates.
>>
>> But I wouldn't worry about optimising it too much - the cost of the
>> free()s in a multithreaded process is higher than the qsort()
>> operation will be anyway.
>
> That's true, I always forget that memory (de)allocation is not all  
> that fast.
>
> Speaking of which, I've wondered how possible it is to re-use
> allocated data structures in liblo.. Haven't done much experimentation
> to find out yet, but theoretically OSC functions should be considered
> semi-real-time and do as little dynamic work with the heap as
> possible.

Remember that the raison d'etre of liblo is sending IP packets, that's  
not a quick operation. Adding a bunch of places for bugs to lurk and  
leaks to occur is not worth a performance gain that's not usually  
measurable.

That said, there was a bug in recentish glibc that makes free()s in  
pthreads processes significantly slower than they should be, but I  
still think it's not much compared to the network IO cost. A pair of  
test apps and a run with oprofile would tell you quite quickly if it's  
worth the effort.

When I first designed liblo I wanted to do the message construction on  
the stack (which is why the types hide the *), but it's not really  
practical unfortunately.

- Steve

On Feb 13, 2008 9:28 AM, Stephen Sinclair <rad...@gm...> wrote:
> On Feb 13, 2008 9:18 AM, Steve Harris <S.W...@ec...> wrote:
> >
> >
> > On 13 Feb 2008, at 14:08, Stephen Sinclair wrote:
> >
> > > On Feb 13, 2008 5:45 AM, Steve Harris <S.W...@ec...>
> > > wrote:
> > >
> > >>> I can imagine looping through the msgs[] list to look for doubles,
> > >>> but
> > >>> this probably would not be very efficient.
> > >>> On the other hand, there are not likely too many messages to
> > >>> consider,
> > >>> so it would be bounded to a small number of iterations.
> > >>
> > >> The most efficient thing would be to qsort the array, then just free
> > >> the unique elements of the array.
> > >>
> > >> Could add special cases for 1 and 2 messages, to avoid the overhead.
> > >
> > > Yeah, not a bad idea.  Thinking about it, this would really be quite a
> > > corner case..
> > > I can't think of too many reasons for adding the same message twice
> > > to a bundle.
> > > But certainly it could happen..
> > >
> > > I wonder if the function should accept a boolean telling it whether or
> > > not to perform the qsort.
> >
> > I think that goes against the spirit of the rest of the API, needless
> > complexity for the developers.
>
> Yes, I agree.
> Sometimes a small, bounded performance hit in a tiny corner case can
> be worth the trade-off for simplicity. :)
>
> > Could scan the messages already in the bundle at insertion time to see
> > if they're already there. Over a certain length its more efficient not
> > to scan, and just assume there might be duplicates.
> >
> > But I wouldn't worry about optimising it too much - the cost of the
> > free()s in a multithreaded process is higher than the qsort()
> > operation will be anyway.
>
> That's true, I always forget that memory (de)allocation is not all that fast.
>
> Speaking of which, I've wondered how possible it is to re-use
> allocated data structures in liblo.. Haven't done much experimentation
> to find out yet, but theoretically OSC functions should be considered
> semi-real-time and do as little dynamic work with the heap as
> possible.
>

Here's my proposed amendment to Kentaro's patch:

diff --git a/src/bundle.c b/src/bundle.c
index 70df295..5851eb1 100644
--- a/src/bundle.c
+++ b/src/bundle.c
@@ -133,6 +133,36 @@ void lo_bundle_free(lo_bundle b)
     free(b);
 }

+static int compare_ptrs( const void* a, const void* b )
+{
+    if (*(void**)a <  *(void**)b) return -1;
+    if (*(void**)a == *(void**)b) return 0;
+    return 1;
+}
+
+void lo_bundle_free_messages(lo_bundle b)
+{
+    int i;
+    lo_message tmp = 0;
+
+    if (!b)
+        return;
+
+    /* avoid freeing the same message twice */
+    if (b->len > 2)
+        qsort(b->msgs, b->len, sizeof(lo_message*), compare_ptrs);
+
+    for(i = 0; i < b->len; i++) {
+        if (b->msgs[i] != tmp) {
+            tmp = b->msgs[i];
+            lo_message_free(b->msgs[i]);
+        }
+    }
+    free(b->msgs);
+    free(b->paths);
+    free(b);
+}
+

I also modified a couple of the bundle cases in testlo to use the
bundle_free_messages() call to test both the sorted and  unsorted
cases, which pass okay for me.

Steve

On Feb 13, 2008 9:18 AM, Steve Harris <S.W...@ec...> wrote:
>
>
> On 13 Feb 2008, at 14:08, Stephen Sinclair wrote:
>
> > On Feb 13, 2008 5:45 AM, Steve Harris <S.W...@ec...>
> > wrote:
> >
> >>> I can imagine looping through the msgs[] list to look for doubles,
> >>> but
> >>> this probably would not be very efficient.
> >>> On the other hand, there are not likely too many messages to
> >>> consider,
> >>> so it would be bounded to a small number of iterations.
> >>
> >> The most efficient thing would be to qsort the array, then just free
> >> the unique elements of the array.
> >>
> >> Could add special cases for 1 and 2 messages, to avoid the overhead.
> >
> > Yeah, not a bad idea.  Thinking about it, this would really be quite a
> > corner case..
> > I can't think of too many reasons for adding the same message twice
> > to a bundle.
> > But certainly it could happen..
> >
> > I wonder if the function should accept a boolean telling it whether or
> > not to perform the qsort.
>
> I think that goes against the spirit of the rest of the API, needless
> complexity for the developers.

Yes, I agree.
Sometimes a small, bounded performance hit in a tiny corner case can
be worth the trade-off for simplicity. :)

> Could scan the messages already in the bundle at insertion time to see
> if they're already there. Over a certain length its more efficient not
> to scan, and just assume there might be duplicates.
>
> But I wouldn't worry about optimising it too much - the cost of the
> free()s in a multithreaded process is higher than the qsort()
> operation will be anyway.

That's true, I always forget that memory (de)allocation is not all that fast.

Speaking of which, I've wondered how possible it is to re-use
allocated data structures in liblo.. Haven't done much experimentation
to find out yet, but theoretically OSC functions should be considered
semi-real-time and do as little dynamic work with the heap as
possible.


Steve

On 13 Feb 2008, at 14:08, Stephen Sinclair wrote:

> On Feb 13, 2008 5:45 AM, Steve Harris <S.W...@ec...>  
> wrote:
>
>>> I can imagine looping through the msgs[] list to look for doubles,  
>>> but
>>> this probably would not be very efficient.
>>> On the other hand, there are not likely too many messages to  
>>> consider,
>>> so it would be bounded to a small number of iterations.
>>
>> The most efficient thing would be to qsort the array, then just free
>> the unique elements of the array.
>>
>> Could add special cases for 1 and 2 messages, to avoid the overhead.
>
> Yeah, not a bad idea.  Thinking about it, this would really be quite a
> corner case..
> I can't think of too many reasons for adding the same message twice  
> to a bundle.
> But certainly it could happen..
>
> I wonder if the function should accept a boolean telling it whether or
> not to perform the qsort.

I think that goes against the spirit of the rest of the API, needless  
complexity for the developers.

Could scan the messages already in the bundle at insertion time to see  
if they're already there. Over a certain length its more efficient not  
to scan, and just assume there might be duplicates.

But I wouldn't worry about optimising it too much - the cost of the  
free()s in a multithreaded process is higher than the qsort()  
operation will be anyway.

- Steve

On Feb 13, 2008 5:45 AM, Steve Harris <S.W...@ec...> wrote:

> > I can imagine looping through the msgs[] list to look for doubles, but
> > this probably would not be very efficient.
> > On the other hand, there are not likely too many messages to consider,
> > so it would be bounded to a small number of iterations.
>
> The most efficient thing would be to qsort the array, then just free
> the unique elements of the array.
>
> Could add special cases for 1 and 2 messages, to avoid the overhead.

Yeah, not a bad idea.  Thinking about it, this would really be quite a
corner case..
I can't think of too many reasons for adding the same message twice to a bundle.
But certainly it could happen..

I wonder if the function should accept a boolean telling it whether or
not to perform the qsort.

Steve

On 13 Feb 2008, at 03:07, Stephen Sinclair wrote:

> Hello!
>
>>> Please review and consider adding the following patch. The patch  
>>> adds a new
>>> function "lo_bundle_free_messages()" that completly frees a bundle  
>>> object.
>
> Sorry I'm only getting around to this now.  I have had to work to keep
> up with my courses lately.. :)
>
> I looked at your patch..
> It looks like it should work and it probably a nice helper function,
> but the only concern is that I think it won't handle situations where
> the same message is added more than once to a bundle.  This would end
> with a double-free, and I guess a segfault.  Any idea how to handle
> this?
>
> I can imagine looping through the msgs[] list to look for doubles, but
> this probably would not be very efficient.
> On the other hand, there are not likely too many messages to consider,
> so it would be bounded to a small number of iterations.

The most efficient thing would be to qsort the array, then just free  
the unique elements of the array.

Could add special cases for 1 and 2 messages, to avoid the overhead.

- Steve

Hello!

> > Please review and consider adding the following patch. The patch adds a new
> > function "lo_bundle_free_messages()" that completly frees a bundle object.

Sorry I'm only getting around to this now.  I have had to work to keep
up with my courses lately.. :)

I looked at your patch..
It looks like it should work and it probably a nice helper function,
but the only concern is that I think it won't handle situations where
the same message is added more than once to a bundle.  This would end
with a double-free, and I guess a segfault.  Any idea how to handle
this?

I can imagine looping through the msgs[] list to look for doubles, but
this probably would not be very efficient.
On the other hand, there are not likely too many messages to consider,
so it would be bounded to a small number of iterations.

I'll repeat the code here for reference..

+void lo_bundle_free_messages(lo_bundle b)
+{
+    int i;
+    if (!b) {
+       return;
+    }
+
+    for(i = 0; i < b->len; i++) {
+        lo_message_free(b->msgs[i]);
+    }
+    free(b->msgs);
+    free(b->paths);
+    free(b);
+}


Steve