documenting netjack

[Jörn Nettingsmeier-5]

hi torben, hi everyone!


after our very instructive walk through berlin yesterday, i'm now trying
to understand netjack to eventually be able to write some documentation.
as a first step, i have read the code again (but not necessarily
understood it). find attached a diff that mainly contains comments with
open questions, currently only about pretty superficial issues...
after a while, i hope to have intelligent questions about the actual
guts of netjack as well :)

some questions may be pretty stupid and obvious - ignore those, i can
probably answer them myself once i've gained somme more understanding as
i go along. but others might be worth commenting on *in the code*, so
that new developers can get a foot in the door more quickly.

i understand that the cores of netjack1 for jack1 and netjack1 for jack2
are identical, so i've based my questions on jack1 for now, because i'm
more familiar with that and use it daily. if preferred, i can base
future diffs on the netjack1/jack2 integration.

the only part that actually deals with functionality is the auto-config
feature. i may be missing something, but i don't see any use for not
auto-configuring the slave. it seems that autoconfig was tacked on, but
now that it exists, manual config should be disallowed imho. that would
get rid of quite a few lines of redundant initialisation. your comments
are appreciated.


best,

jörn




--
Jörn Nettingsmeier

Audio engineer
Meister für Veranstaltungstechnik (Bühne/Studio)

Lortzingstr. 11, D-45128 Essen, Germany
[phone]

http://stackingdwarves.net


[questions_netjack.diff]

---

Index: netjack/net_driver.c
===================================================================
--- netjack/net_driver.c (revision 3671)
+++ netjack/net_driver.c (working copy)
@@ -58,6 +58,8 @@
 
 #define MIN(x,y) ((x)<(y) ? (x) : (y))
 
+#define CELT_MODE 1000
+
 static int sync_state = TRUE;
 static jack_transport_state_t last_transport_state;
 
@@ -547,15 +549,15 @@
         driver->capture_ports =
             jack_slist_append (driver->capture_ports, port);
 
- if( driver->bitdepth == 1000 ) {
+ if( driver->bitdepth == CELT_MODE ) {
 #if HAVE_CELT
-    celt_int32_t lookahead;
+    celt_int32 lookahead;
     // XXX: memory leak
-    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, 1, driver->period_size, NULL );
+    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, driver->period_size, NULL );
     celt_mode_info( celt_mode, CELT_GET_LOOKAHEAD, &lookahead );
     driver->codec_latency = 2*lookahead;
 
-    driver->capture_srcs = jack_slist_append(driver->capture_srcs, celt_decoder_create( celt_mode ) );
+    driver->capture_srcs = jack_slist_append(driver->capture_srcs, celt_decoder_create( celt_mode, 1, NULL ) );
 #endif
  } else {
 #if HAVE_SAMPLERATE
@@ -594,11 +596,11 @@
 
         driver->playback_ports =
             jack_slist_append (driver->playback_ports, port);
- if( driver->bitdepth == 1000 ) {
+ if( driver->bitdepth == CELT_MODE ) {
 #if HAVE_CELT
     // XXX: memory leak
-    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, 1, driver->period_size, NULL );
-    driver->playback_srcs = jack_slist_append(driver->playback_srcs, celt_encoder_create( celt_mode ) );
+    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, driver->period_size, NULL );
+    driver->playback_srcs = jack_slist_append(driver->playback_srcs, celt_encoder_create( celt_mode, 1, NULL ) );
 #endif
  } else {
 #if HAVE_SAMPLERATE
@@ -733,7 +735,7 @@
     driver->engine = NULL;
 
 
-    if ((bitdepth != 0) && (bitdepth != 8) && (bitdepth != 16) && (bitdepth != 1000))
+    if ((bitdepth != 0) && (bitdepth != 8) && (bitdepth != 16) && (bitdepth != CELT_MODE))
     {
         jack_info ("Invalid bitdepth: %d (8, 16 or 0 for float) !!!", bitdepth);
         return NULL;
@@ -748,7 +750,7 @@
     driver->sockfd = socket (PF_INET, SOCK_DGRAM, 0);
     if (driver->sockfd == -1)
     {
-        jack_info ("socket error");
+        jack_info ("incoming socket error");
         return NULL;
     }
     address.sin_family = AF_INET;
@@ -756,14 +758,14 @@
     address.sin_addr.s_addr = htonl(INADDR_ANY);
     if (bind (driver->sockfd, (struct sockaddr *) &address, sizeof (address)) < 0)
     {
-        jack_info("bind error");
+        jack_info("incoming socket bind error");
         return NULL;
     }
 
     driver->outsockfd = socket (PF_INET, SOCK_DGRAM, 0);
     if (driver->outsockfd == -1)
     {
-        jack_info ("socket error");
+        jack_info ("outgoing socket error");
         return NULL;
     }
     driver->srcaddress_valid = 0;
@@ -778,8 +780,11 @@
 
  // XXX: netjack_poll polls forever.
  //      thats ok here.
+
+ //JN Looks like the first packet is special (autoconfig). what if it's lost?
  if (netjack_poll (driver->sockfd, 500))
-    first_pack_len = recvfrom (driver->sockfd, first_packet, sizeof (jacknet_packet_header), 0, (struct sockaddr*) & driver->syncsource_address, &address_size);
+    first_pack_len = recvfrom (driver->sockfd, first_packet, sizeof (jacknet_packet_header), 0,
+                                (struct sockaddr*) & driver->syncsource_address, &address_size);
  else
     first_pack_len = 0;
 
@@ -789,6 +794,9 @@
  {
     packet_header_ntoh (first_packet);
 
+            //JN if slave config can be overridden, we can't depend on it for calculations and prepartions
+            //JN before we are connected. and unless we are synced, we can't have clients connected anyways.
+            //JN so what's the purpose of being able to configure the slave at all?
     jack_info ("AutoConfig Override !!!");
     if (driver->sample_rate != first_packet->sample_rate)
     {
@@ -835,12 +843,12 @@
         (jack_time_t) floor ((((float) driver->period_size) / (float)driver->sample_rate)
                              * 1000000.0f);
 
-    if( driver->bitdepth == 1000 ) {
- // celt mode.
+    if( driver->bitdepth == CELT_MODE ) {
  // TODO: this is a hack. But i dont want to change the packet header.
  driver->net_period_down = resample_factor;
  driver->net_period_up = resample_factor_up;
     } else {
+        //JN what if the following terms yield non-integer period sizes?
  driver->net_period_down = (float) driver->period_size / (float) resample_factor;
  driver->net_period_up = (float) driver->period_size / (float) resample_factor_up;
     }
@@ -944,6 +952,7 @@
             "The socket port we are listening on for sync packets");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: users would expect a target sample rate, not a factor
     i++;
     strcpy (params[i].name, "factor");
     params[i].character  = 'f';
@@ -953,6 +962,9 @@
             "Factor for sample rate reduction");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: users would expect to set a target sample rate, not a factor
+    //JN: better find names that are consistent, i.e.
+    //JN: u for upstream sample rate, d for downstream sample rate.
     i++;
     strcpy (params[i].name, "upstream-factor");
     params[i].character  = 'u';
@@ -962,6 +974,11 @@
             "Factor for sample rate reduction on the upstream");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: number of bytes per channel is awkward.
+    //JN: better have bandwidth in total kbit/s (for all channels).
+    //JN: let the machine do the math instead.
+    //JN: in case CELT can't handle that particular byte count,
+    //JN: round it _down_ and inform the user via jack_info()
     i++;
     strcpy (params[i].name, "celt");
     params[i].character  = 'c';
@@ -989,6 +1006,12 @@
             "Whether to slave the transport to the master transport");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: is that the reason for a local, non-auto config?
+    //JN: that we can start right away?
+    //JN: but then, what if autoconfig packet arrives anyway?
+    //JN: then we should, in the absence of -a, ignore the autoconf
+    //JN: data from the master. but if we do and important parameters
+    //JN: don't match, we get garbage. so again: why not *always* autoconf?
     i++;
     strcpy (params[i].name, "autoconf");
     params[i].character  = 'a';
@@ -998,13 +1021,18 @@
             "Whether to use Autoconfig, or just start.");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: this parameter is not clear. is that the roundtrip latency?
+    //JN: what is its unit? (i think periods).
+    //JN: maybe let the user specify maximum milliseconds and then round _down_
+    //JN: to the nearest packets x periodsize / srate, and inform the user of
+    //JN: the change with a jack_info()?
     i++;
     strcpy (params[i].name, "latency");
     params[i].character  = 'L';
     params[i].type       = JackDriverParamUInt;
     params[i].value.ui   = 5U;
     strcpy (params[i].short_desc,
-            "Latency setting");
+            "Latency setting"); //JN: better "Roundtrip latency in periods"
     strcpy (params[i].long_desc, params[i].short_desc);
 
     i++;
@@ -1016,6 +1044,9 @@
             "Send packets N times");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: better: "assume identical byte order on master and slave (avoids conversion overhead)"
+    //JN: because we don't really care for network byte order, only that master and slave endianness
+    //JN: match...
     i++;
     strcpy (params[i].name, "no-htonl");
     params[i].character  = 'H';
@@ -1025,6 +1056,8 @@
             "Dont convert samples to network byte order.");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: can you explain to the layman user what the deadline is in this context?
+    //JN: does that imply an extra packet round trip of latency?
     i++;
     strcpy (params[i].name, "deadline");
     params[i].character  = 'D';
@@ -1034,6 +1067,7 @@
             "always wait for the deadline, more friendly to alsa_io");
     strcpy (params[i].long_desc, params[i].short_desc);
 
+    //JN: what does this mean?
     i++;
     strcpy (params[i].name, "jitter");
     params[i].character  = 'j';
@@ -1047,6 +1081,7 @@
     return desc;
 }
 
+//JN: "_pcm" is wrong now that CELT is an option. Just use "net"?
 const char driver_client_name[] = "net_pcm";
 
 jack_driver_t *
@@ -1129,7 +1164,7 @@
 
     case 'c':
 #if HAVE_CELT
- bitdepth = 1000;
+ bitdepth = CELT_MODE;
  resample_factor = param->value.ui;
 #else
  printf( "not built with celt support\n" );
@@ -1167,7 +1202,7 @@
         }
     }
 
-    return net_driver_new (client, "net_pcm", capture_ports, playback_ports,
+    return net_driver_new (client, driver_client_name[], capture_ports, playback_ports,
                            capture_ports_midi, playback_ports_midi,
                            sample_rate, period_size,
                            listen_port, handle_transport_sync,
Index: netjack/net_driver.h
===================================================================
--- netjack/net_driver.h (revision 3671)
+++ netjack/net_driver.h (working copy)
@@ -42,6 +42,7 @@
     jack_nframes_t  bitdepth;
     jack_nframes_t  period_size;
     int    dont_htonl_floats;
+//JN: can this typo ("ded") be fixed, or does it touch jackd core code?
     int    always_wait_dedline;
 
     jack_nframes_t  codec_latency;

---

_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org

[Stéphane Letz]

Le 27 oct. 2009 à 20:22, Jörn Nettingsmeier a écrit :

> hi torben, hi everyone!
>
>
> after our very instructive walk through berlin yesterday, i'm now  
> trying to understand netjack to eventually be able to write some  
> documentation. as a first step, i have read the code again (but not  
> necessarily understood it). find attached a diff that mainly  
> contains comments with open questions, currently only about pretty  
> superficial issues...
> after a while, i hope to have intelligent questions about the actual  
> guts of netjack as well :)

Hi Jörn

I think it is an excellent initiative, you could even start a wiki  
page on http://trac.jackaudio.org/ to add your comments or  
documentation?

>
> some questions may be pretty stupid and obvious - ignore those, i  
> can probably answer them myself once i've gained somme more  
> understanding as i go along. but others might be worth commenting on  
> *in the code*, so that new developers can get a foot in the door  
> more quickly.

Commenting is always wanted, if it can ultimately help better  
understanding the code, and clean the part that still need to be  
cleaned up....

>
> i understand that the cores of netjack1 for jack1 and netjack1 for  
> jack2 are identical, so i've based my questions on jack1 for now,  
> because i'm more familiar with that and use it daily. if preferred,  
> i can base future diffs on the netjack1/jack2 integration.

Well if the ultimate goal if to integrate netjack1 in JACK2, then yes  
it would be better to start from the GIT branch that just contain  
"netjack1 for JACK2 trunk".

http://repo.or.cz/w/jack2.git?a=shortlog;h=refs/heads/netone-isolateion

>
> the only part that actually deals with functionality is the auto-
> config feature. i may be missing something, but i don't see any use  
> for not auto-configuring the slave. it seems that autoconfig was  
> tacked on, but now that it exists, manual config should be  
> disallowed imho. that would get rid of quite a few lines of  
> redundant initialisation. your comments are appreciated.
>


I don't know exactly the situation for NetJack1, but in case the  
comparison would help, for NetJack2, the choice is to have the master  
send what is mandatory (sample rate, buffer size..) and the slave  
chose what it can chose. So we end up with something like:

MASTER: the "net_source side"

        -a, --multicast_ip Multicast Address (default: 225.3.19.154)
        -p, --udp_net_port UDP port (default: 19000)
        -c, --auto_connect Auto connect netmaster to system ports (default:  
false)

SLAVE:  the  "-d net" side

        -a, --multicast_ip Multicast Address (default: 225.3.19.154)
        -p, --udp_net_port UDP port (default: 19000)
        -M, --mtu MTU to the master (default: 1500)
        -C, --input_ports Number of audio input ports (default: 2)
        -P, --output_ports Number of audio output ports (default: 2)
        -i, --midi_in_ports Number of midi input ports (default: 0)
        -o, --midi_out_ports Number of midi output ports (default: 0)
        -n, --client_name Name of the jack client (default: 'hostname')
        -t, --transport_sync Sync transport with master's (default: 1)
        -m, --mode Slow, Normal or Fast mode. (default: slow)

(--multicast_ip is NetJack2 specific, and --mode is similar to the  
"latency" value of NetJack1)

So basically slave receive buffer size and SR and send back its number  
of wanted audio in/out, midi in/out.. etc.. so that the master can  
then create the correct "proxy" on master.

Stéphane


_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org

[torbenh]

On Tue, Oct 27, 2009 at 08:22:11PM +0100, Jörn Nettingsmeier wrote:

> hi torben, hi everyone!
>
>
> after our very instructive walk through berlin yesterday, i'm now trying to
> understand netjack to eventually be able to write some documentation. as a
> first step, i have read the code again (but not necessarily understood it).
> find attached a diff that mainly contains comments with open questions,
> currently only about pretty superficial issues...
> after a while, i hope to have intelligent questions about the actual guts
> of netjack as well :)
>
> some questions may be pretty stupid and obvious - ignore those, i can
> probably answer them myself once i've gained somme more understanding as i
> go along. but others might be worth commenting on *in the code*, so that
> new developers can get a foot in the door more quickly.
>
> i understand that the cores of netjack1 for jack1 and netjack1 for jack2
> are identical, so i've based my questions on jack1 for now, because i'm
> more familiar with that and use it daily. if preferred, i can base future
> diffs on the netjack1/jack2 integration.
>
> the only part that actually deals with functionality is the auto-config
> feature. i may be missing something, but i don't see any use for not
> auto-configuring the slave. it seems that autoconfig was tacked on, but now
> that it exists, manual config should be disallowed imho. that would get rid
> of quite a few lines of redundant initialisation. your comments are
> appreciated.

the problem with jack1 is that, it does not allow a client to connect to
the slave jack until the master configured it.
while waiting for the first packet, jackd is in a half-initialised
state.

it seems to work fine for qjackctl, but a real processing client will
fail.

also current state of autoconfig allows me to request 1G channels io
and crash your instance, filling all memory before.
this can of course be easily fixed with a firewall rule, but its still a
bit of a problem.

my plans were to handle the config stuff through an external frontend,
which just supplies the right options on both sides. which would render
autoconfig obsolete.

however i never realised it. so we currently depend on autoconfig.


conversation in patch form is a nice idea, using git, we could optimize
the effeciency even more.

we could just do this conversation on a separate branch. and it would be
nice to do it on the netone code.

gonna do it classically mail style now, but please either setup gitd to
export your local repository, or get an account on repo.or.cz
(setting up gitd in read-only mode is easy)

>
>
> best,
>
> jörn
>
>
>
>
> --
> Jörn Nettingsmeier
>
> Audio engineer
> Meister für Veranstaltungstechnik (Bühne/Studio)
>
> Lortzingstr. 11, D-45128 Essen, Germany
> [phone]
>
> http://stackingdwarves.net
>

> Index: netjack/net_driver.c
> ===================================================================
> --- netjack/net_driver.c (revision 3671)
> +++ netjack/net_driver.c (working copy)
> @@ -58,6 +58,8 @@
>  
>  #define MIN(x,y) ((x)<(y) ? (x) : (y))
>  
> +#define CELT_MODE 1000
> +
>  static int sync_state = TRUE;
>  static jack_transport_state_t last_transport_state;
>  
> @@ -547,15 +549,15 @@
>          driver->capture_ports =
>              jack_slist_append (driver->capture_ports, port);
>  
> - if( driver->bitdepth == 1000 ) {
> + if( driver->bitdepth == CELT_MODE ) {
>  #if HAVE_CELT
> -    celt_int32_t lookahead;
> +    celt_int32 lookahead;
>      // XXX: memory leak
> -    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, 1, driver->period_size, NULL );
> +    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, driver->period_size, NULL );
>      celt_mode_info( celt_mode, CELT_GET_LOOKAHEAD, &lookahead );
>      driver->codec_latency = 2*lookahead;
>  
> -    driver->capture_srcs = jack_slist_append(driver->capture_srcs, celt_decoder_create( celt_mode ) );
> +    driver->capture_srcs = jack_slist_append(driver->capture_srcs, celt_decoder_create( celt_mode, 1, NULL ) );
>  #endif
>   } else {
>  #if HAVE_SAMPLERATE
> @@ -594,11 +596,11 @@
>  
>          driver->playback_ports =
>              jack_slist_append (driver->playback_ports, port);
> - if( driver->bitdepth == 1000 ) {
> + if( driver->bitdepth == CELT_MODE ) {
>  #if HAVE_CELT
>      // XXX: memory leak
> -    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, 1, driver->period_size, NULL );
> -    driver->playback_srcs = jack_slist_append(driver->playback_srcs, celt_encoder_create( celt_mode ) );
> +    CELTMode *celt_mode = celt_mode_create( driver->sample_rate, driver->period_size, NULL );
> +    driver->playback_srcs = jack_slist_append(driver->playback_srcs, celt_encoder_create( celt_mode, 1, NULL ) );
>  #endif
>   } else {
>  #if HAVE_SAMPLERATE
> @@ -733,7 +735,7 @@
>      driver->engine = NULL;
>  
>  
> -    if ((bitdepth != 0) && (bitdepth != 8) && (bitdepth != 16) && (bitdepth != 1000))
> +    if ((bitdepth != 0) && (bitdepth != 8) && (bitdepth != 16) && (bitdepth != CELT_MODE))
>      {
>          jack_info ("Invalid bitdepth: %d (8, 16 or 0 for float) !!!", bitdepth);
>          return NULL;
> @@ -748,7 +750,7 @@
>      driver->sockfd = socket (PF_INET, SOCK_DGRAM, 0);
>      if (driver->sockfd == -1)
>      {
> -        jack_info ("socket error");
> +        jack_info ("incoming socket error");
>          return NULL;
>      }
>      address.sin_family = AF_INET;
> @@ -756,14 +758,14 @@
>      address.sin_addr.s_addr = htonl(INADDR_ANY);
>      if (bind (driver->sockfd, (struct sockaddr *) &address, sizeof (address)) < 0)
>      {
> -        jack_info("bind error");
> +        jack_info("incoming socket bind error");
>          return NULL;
>      }
>  
>      driver->outsockfd = socket (PF_INET, SOCK_DGRAM, 0);
>      if (driver->outsockfd == -1)
>      {
> -        jack_info ("socket error");
> +        jack_info ("outgoing socket error");
>          return NULL;
>      }
>      driver->srcaddress_valid = 0;
> @@ -778,8 +780,11 @@
>  
>   // XXX: netjack_poll polls forever.
>   //      thats ok here.
> +
> + //JN Looks like the first packet is special (autoconfig). what if it's lost?

no its not special.
every packet contains the autoconfig data.
its only special in the way, that its audio data will get dropped.

>   if (netjack_poll (driver->sockfd, 500))
> -    first_pack_len = recvfrom (driver->sockfd, first_packet, sizeof (jacknet_packet_header), 0, (struct sockaddr*) & driver->syncsource_address, &address_size);
> +    first_pack_len = recvfrom (driver->sockfd, first_packet, sizeof (jacknet_packet_header), 0,
> +                                (struct sockaddr*) & driver->syncsource_address, &address_size);
>   else
>      first_pack_len = 0;
>  
> @@ -789,6 +794,9 @@
>   {
>      packet_header_ntoh (first_packet);
>  
> +            //JN if slave config can be overridden, we can't depend on it for calculations and prepartions
> +            //JN before we are connected. and unless we are synced, we can't have clients connected anyways.
> +            //JN so what's the purpose of being able to configure the slave at all?

see above. but the question is still valid.

>      jack_info ("AutoConfig Override !!!");
>      if (driver->sample_rate != first_packet->sample_rate)
>      {
> @@ -835,12 +843,12 @@
>          (jack_time_t) floor ((((float) driver->period_size) / (float)driver->sample_rate)
>                               * 1000000.0f);
>  
> -    if( driver->bitdepth == 1000 ) {
> - // celt mode.
> +    if( driver->bitdepth == CELT_MODE ) {
>   // TODO: this is a hack. But i dont want to change the packet header.
>   driver->net_period_down = resample_factor;
>   driver->net_period_up = resample_factor_up;
>      } else {
> +        //JN what if the following terms yield non-integer period sizes?

good question. (undefined operation)
but downsampling on the wire is obsoleted by celt. so before i touch
this, id rather remove it.

>   driver->net_period_down = (float) driver->period_size / (float) resample_factor;
>   driver->net_period_up = (float) driver->period_size / (float) resample_factor_up;
>      }
> @@ -944,6 +952,7 @@
>              "The socket port we are listening on for sync packets");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: users would expect a target sample rate, not a factor
>      i++;
>      strcpy (params[i].name, "factor");
>      params[i].character  = 'f';
> @@ -953,6 +962,9 @@
>              "Factor for sample rate reduction");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: users would expect to set a target sample rate, not a factor
> +    //JN: better find names that are consistent, i.e.
> +    //JN: u for upstream sample rate, d for downstream sample rate.

these factors are only half-baked code. iirc the netsource doesnt even
support asymmetric downsampling on wire... this will get removed.

>      i++;
>      strcpy (params[i].name, "upstream-factor");
>      params[i].character  = 'u';
> @@ -962,6 +974,11 @@
>              "Factor for sample rate reduction on the upstream");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: number of bytes per channel is awkward.
> +    //JN: better have bandwidth in total kbit/s (for all channels).
> +    //JN: let the machine do the math instead.
> +    //JN: in case CELT can't handle that particular byte count,
> +    //JN: round it _down_ and inform the user via jack_info()
>      i++;
>      strcpy (params[i].name, "celt");
>      params[i].character  = 'c';
> @@ -989,6 +1006,12 @@
>              "Whether to slave the transport to the master transport");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: is that the reason for a local, non-auto config?
> +    //JN: that we can start right away?

yes.

> +    //JN: but then, what if autoconfig packet arrives anyway?
> +    //JN: then we should, in the absence of -a, ignore the autoconf
> +    //JN: data from the master. but if we do and important parameters
> +    //JN: don't match, we get garbage. so again: why not *always* autoconf?

see above :D

>      i++;
>      strcpy (params[i].name, "autoconf");
>      params[i].character  = 'a';
> @@ -998,13 +1021,18 @@
>              "Whether to use Autoconfig, or just start.");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: this parameter is not clear. is that the roundtrip latency?
> +    //JN: what is its unit? (i think periods).

exactly.

> +    //JN: maybe let the user specify maximum milliseconds and then round _down_
> +    //JN: to the nearest packets x periodsize / srate, and inform the user of
> +    //JN: the change with a jack_info()?

for LAN, there are 3 values, that make sense. 0,1,2
i prefer these values, over calculating which value will correspond to 1

for WAN we are dealing with larger values, and it would make sense to
specify it in ms... thats right.
so a second parameter ?  (also confusing... but maybe better)

>      i++;
>      strcpy (params[i].name, "latency");
>      params[i].character  = 'L';
>      params[i].type       = JackDriverParamUInt;
>      params[i].value.ui   = 5U;
>      strcpy (params[i].short_desc,
> -            "Latency setting");
> +            "Latency setting"); //JN: better "Roundtrip latency in periods"
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
>      i++;
> @@ -1016,6 +1044,9 @@
>              "Send packets N times");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: better: "assume identical byte order on master and slave (avoids conversion overhead)"
> +    //JN: because we don't really care for network byte order, only that master and slave endianness
> +    //JN: match...
>      i++;
>      strcpy (params[i].name, "no-htonl");
>      params[i].character  = 'H';
> @@ -1025,6 +1056,8 @@
>              "Dont convert samples to network byte order.");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: can you explain to the layman user what the deadline is in this context?
> +    //JN: does that imply an extra packet round trip of latency?

with lower latencies on LAN, it would probably mean an extra period of
latency. yes.
with higher latencies, it stabilises the time interval the process
cycle is executed, because a lost packet, will not result in the code
waiting for 5 periods, until the deadline is hit.

but when i introduced this option, the deadline calculation was still
buggy. and alsa_out was shit,
needs some testing, how much improvement this option yields.

>      i++;
>      strcpy (params[i].name, "deadline");
>      params[i].character  = 'D';
> @@ -1034,6 +1067,7 @@
>              "always wait for the deadline, more friendly to alsa_io");
>      strcpy (params[i].long_desc, params[i].short_desc);
>  
> +    //JN: what does this mean?

hmm... i dont remember.

>      i++;
>      strcpy (params[i].name, "jitter");
>      params[i].character  = 'j';
> @@ -1047,6 +1081,7 @@
>      return desc;
>  }
>  
> +//JN: "_pcm" is wrong now that CELT is an option. Just use "net"?
>  const char driver_client_name[] = "net_pcm";
>  
>  jack_driver_t *
> @@ -1129,7 +1164,7 @@
>  
>      case 'c':
>  #if HAVE_CELT
> - bitdepth = 1000;
> + bitdepth = CELT_MODE;
>   resample_factor = param->value.ui;
>  #else
>   printf( "not built with celt support\n" );
> @@ -1167,7 +1202,7 @@
>          }
>      }
>  
> -    return net_driver_new (client, "net_pcm", capture_ports, playback_ports,
> +    return net_driver_new (client, driver_client_name[], capture_ports, playback_ports,
>                             capture_ports_midi, playback_ports_midi,
>                             sample_rate, period_size,
>                             listen_port, handle_transport_sync,
> Index: netjack/net_driver.h
> ===================================================================
> --- netjack/net_driver.h (revision 3671)
> +++ netjack/net_driver.h (working copy)
> @@ -42,6 +42,7 @@
>      jack_nframes_t  bitdepth;
>      jack_nframes_t  period_size;
>      int    dont_htonl_floats;
> +//JN: can this typo ("ded") be fixed, or does it touch jackd core code?

can be fixed, easily.

>      int    always_wait_dedline;
>  
>      jack_nframes_t  codec_latency;


--
torben Hohn
_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org

[Stéphane Letz]

>>
>
> the problem with jack1 is that, it does not allow a client to  
> connect to
> the slave jack until the master configured it.
> while waiting for the first packet, jackd is in a half-initialised
> state.
>
> it seems to work fine for qjackctl, but a real processing client will
> fail.
>
> also current state of autoconfig allows me to request 1G channels io
> and crash your instance, filling all memory before.
> this can of course be easily fixed with a firewall rule, but its  
> still a
> bit of a problem.
>
> my plans were to handle the config stuff through an external frontend,
> which just supplies the right options on both sides. which would  
> render
> autoconfig obsolete.
>
> however i never realised it. so we currently depend on autoconfig.
>
>
> conversation in patch form is a nice idea, using git, we could  
> optimize
> the effeciency even more.
>
> we could just do this conversation on a separate branch. and it  
> would be
> nice to do it on the netone code.

Could I suggest to start from the "netone-isolateion" branch instead?

http://repo.or.cz/w/jack2.git?a=shortlog;h=refs/heads/netone-isolateion

Since the netone branch contains also the "adapter" code changes and  
some JackEngine changes also.

I think we should concentrate first on "pure NetJack1" stuff first and  
see how the other changes can be merged in a second step.

Stéphane

_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org

[Jörn Nettingsmeier-5]

torbenh@... wrote:
> On Tue, Oct 27, 2009 at 08:22:11PM +0100, Jörn Nettingsmeier wrote:

> the problem with jack1 is that, it does not allow a client to connect to
> the slave jack until the master configured it.
> while waiting for the first packet, jackd is in a half-initialised
> state.

which is not bad imho. you can't start the ffado backend either until
you've plugged in a device...
of course, ultimately, it would be cool to be able to set a local graph,
then wait for the master connection. but wouldn't that imply that *all*
clients can properly respond to period size and samplerate changes of
the jackd -net? what's the state of this?

> it seems to work fine for qjackctl, but a real processing client will
> fail.
>
> also current state of autoconfig allows me to request 1G channels io
> and crash your instance, filling all memory before.
> this can of course be easily fixed with a firewall rule, but its still a
> bit of a problem.

still, it's awkward, true. some sort of authentication eventually. for
now, i think that distracts from the real issues. an intermediate fix
could be to configure an upper bound of channels on the slave...

> my plans were to handle the config stuff through an external frontend,
> which just supplies the right options on both sides. which would render
> autoconfig obsolete.
>
> however i never realised it. so we currently depend on autoconfig.

understood.

> conversation in patch form is a nice idea, using git, we could optimize
> the effeciency even more.

certainly :)

> we could just do this conversation on a separate branch. and it would be
> nice to do it on the netone code.

i'd also prefer that, because i can test it more easily. but i'm also
with stephane that it would be good to work on the future "canonical"
jack repository...

stephane, since torben explained to me that the core code is identical,
would you be ok with us working on netjack1 for now and then do a bulk
patch?

> gonna do it classically mail style now, but please either setup gitd to
> export your local repository, or get an account on repo.or.cz
> (setting up gitd in read-only mode is easy)

will find out how to do that (haven't used git from a developer's POV, yet).

best,

jörn



--
Jörn Nettingsmeier

Meister für Veranstaltungstechnik

Audio and event engineer
Ambisonic surround recordings

http://stackingdwarves.net
[phone]

_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org

[Stéphane Letz]

we could just do this conversation on a separate branch. and it would be

nice to do it on the netone code.

i'd also prefer that, because i can test it more easily. but i'm also
with stephane that it would be good to work on the future "canonical"
jack repository...

stephane, since torben explained to me that the core code is identical,
would you be ok with us working on netjack1 for now and then do a bulk
patch?

Not sure to understand here.. do you mean netjack1 for JACK1?? Or netjack1 for JACK2? 

In this case (netjack1 for JACK2) the better way is to start from "netone-isolateion" git branch :

http://repo.or.cz/w/jack2.git?a=shortlog;h=refs/heads/netone-isolateion

(I've just synched with JACK2 SVN trunk, so the GIT netone-isolateion does now contain  netjack1 difference only, added to head of JACK2 SVN)

gonna do it classically mail style now, but please either setup gitd to

export your local repository, or get an account on repo.or.cz

(setting up gitd in read-only mode is easy)

will find out how to do that (haven't used git from a developer's POV, yet).

best,

jörn

Git is quite convenient for this kind of use IMHO.

Stéphane 

_______________________________________________
Jack-Devel mailing list
Jack-Devel@...
http://lists.jackaudio.org/listinfo.cgi/jack-devel-jackaudio.org