From f07c16a6b96c8ed01812f623aa11721015199e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 15:37:54 +0000 Subject: [PATCH 01/48] Add lock-free ringbuffer. Preparation to make jack_stream_write_cb realtime safe. --- audio_jack.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/audio_jack.c b/audio_jack.c index c8a8b9ee..91abbe4b 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -31,6 +31,7 @@ #include #include +#include enum ift_type { IFT_frame_left_sample = 0, @@ -81,6 +82,8 @@ jack_client_t *client; jack_nframes_t sample_rate; jack_nframes_t jack_latency; +static jack_ringbuffer_t *jackbuf; + jack_latency_range_t latest_left_latency_range, latest_right_latency_range; int64_t time_of_latest_transfer; @@ -282,6 +285,9 @@ void jack_deinit() { if (open_client_if_necessary_thread) { pthread_cancel(*open_client_if_necessary_thread); free((char *)open_client_if_necessary_thread); + + jack_ringbuffer_free(jackbuf); + } } @@ -354,6 +360,9 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config.jack_right_channel_name == NULL) config.jack_right_channel_name = strdup("right"); + jackbuf = jack_ringbuffer_create(buffer_size); // make the jack ringbuffer the same size as audio_lmb + jack_ringbuffer_mlock(jackbuf); // lock buffer into memory so that it never gets paged out + jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); From 06504516f12573bd714d12ed143955d549502607 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 15:45:57 +0000 Subject: [PATCH 02/48] die() on failure to create ringbuffer. --- audio_jack.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/audio_jack.c b/audio_jack.c index 91abbe4b..88bec832 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -361,6 +361,8 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a config.jack_right_channel_name = strdup("right"); jackbuf = jack_ringbuffer_create(buffer_size); // make the jack ringbuffer the same size as audio_lmb + if (jackbuf == NULL) + die("Can't allocate %d bytes for the JACK ringbuffer.", buffer_size); jack_ringbuffer_mlock(jackbuf); // lock buffer into memory so that it never gets paged out jack_set_error_function(default_jack_error_callback); From e99837f878360b82363ede4f2358de47e01a1cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 20:18:05 +0000 Subject: [PATCH 03/48] First step of replacing existing buffer with jack_ringbuffer. Compiles, but does not work, of course. --- audio_jack.c | 108 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 89 insertions(+), 19 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 88bec832..1dc471ac 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -38,15 +38,20 @@ enum ift_type { IFT_frame_right_sample, } ift_type; +// Two-channel, 16bit audio: +static const int bytes_per_frame = 4; // Four seconds buffer -- should be plenty -#define buffer_size 44100 * 4 * 2 * 2 +#define buffer_size 44100 * 4 * bytes_per_frame static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; +/* char *audio_lmb, *audio_umb, *audio_toq, *audio_eoq; + size_t audio_occupancy; // this is in frames, not bytes. A frame is a left and // right sample, each 16 bits, hence 4 bytes +*/ pthread_t *open_client_if_necessary_thread = NULL; int jack_init(int, char **); @@ -83,6 +88,7 @@ jack_nframes_t sample_rate; jack_nframes_t jack_latency; static jack_ringbuffer_t *jackbuf; +static int flush_please = 0; jack_latency_range_t latest_left_latency_range, latest_right_latency_range; int64_t time_of_latest_transfer; @@ -90,7 +96,15 @@ int64_t time_of_latest_transfer; int play(void *buf, int samples) { // debug(1,"jack_play of %d samples.",samples); // copy the samples into the queue - size_t bytes_to_transfer = samples * 2 * 2; + size_t bytes_to_transfer, bytes_transferred; + bytes_to_transfer = samples * bytes_per_frame; + pthread_mutex_lock(&buffer_mutex); // it's ok to lock here since we're not in the realtime callback + bytes_transferred = jack_ringbuffer_write(jackbuf, buf, bytes_to_transfer); + pthread_mutex_unlock(&buffer_mutex); + if (bytes_transferred < bytes_to_transfer) { + debug(1, "JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); + } +/* size_t space_to_end_of_buffer = audio_umb - audio_eoq; if (space_to_end_of_buffer >= bytes_to_transfer) { memcpy(audio_eoq, buf, bytes_to_transfer); @@ -107,6 +121,7 @@ int play(void *buf, int samples) { audio_eoq = audio_lmb + bytes_to_transfer - space_to_end_of_buffer; pthread_mutex_unlock(&buffer_mutex); } +*/ return 0; } @@ -132,6 +147,24 @@ void deinterleave_and_convert_stream(const char *interleaved_frames, } } +static inline jack_default_audio_sample_t sample_conv(short sample) { + return ((sample < 0) ? (-1.0 * sample / SHRT_MIN) : (1.0 * sample / SHRT_MAX)); +} + +void deinterleave_and_convert(const char *interleaved_frames, + jack_default_audio_sample_t * const jack_buffer_left, + jack_default_audio_sample_t * const jack_buffer_right, + jack_nframes_t nframes) { + jack_nframes_t i; + short *ifp = (short *)interleaved_frames; // we're dealing with 16bit audio here + jack_default_audio_sample_t *fpl = jack_buffer_left; + jack_default_audio_sample_t *fpr = jack_buffer_right; + for (i=0; i nframes) { + frames_required = nframes; + } else { + frames_required = thisbuf; + } + deinterleave_and_convert(v[i].buf, &left_buffer[frames_written], &right_buffer[frames_written], frames_required); + frames_written = frames_required; + nframes -= frames_written; + } + jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); + + // debug(1,"transferring %u frames",written); + // Why are we doing this? The port latency should never change unless the JACK graph is reordered. Should happen on a graph reorder callback only. + // jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); + // jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); + + // now, if there are any more frames to put into the buffer, fill them with + // silence + while (nframes > 0) { + left_buffer[frames_written] = 0.0; + right_buffer[frames_written] = 0.0; + frames_written++; + nframes--; + } + time_of_latest_transfer = get_absolute_time_in_fp(); // this is the only writer, no locking necessary. + +/* size_t frames_we_can_transfer = nframes; // lock pthread_mutex_lock(&buffer_mutex); @@ -173,7 +241,6 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a IFT_frame_right_sample); audio_toq = audio_lmb + (frames_we_can_transfer - first_portion_to_write) * 2 * 2; } - // debug(1,"transferring %u frames",frames_we_can_transfer); audio_occupancy -= frames_we_can_transfer; jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); @@ -181,16 +248,12 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a pthread_mutex_unlock(&buffer_mutex); // unlock - // now, if there are any more frames to put into the buffer, fill them with - // silence - jack_nframes_t i; - for (i = frames_we_can_transfer; i < nframes; i++) { - left_buffer[i] = 0.0; - right_buffer[i] = 0.0; - } +*/ return 0; } +// FIXME: set_graph_order_callback(), recompute latencies here! + void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } @@ -207,6 +270,7 @@ void default_jack_set_latency_callback(jack_latency_callback_mode_t mode, b_latency = (right_latency_range.min + right_latency_range.max) / 2; // jack_latency = b_latency; // actually, we are not interested in the latency of the jack // devices connected... + // FIXME: yes, we are. jack_latency = 0; debug(1, "playback latency callback: %" PRIu32 ".", jack_latency); } @@ -368,6 +432,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); +/* // allocate space for the audio buffer audio_lmb = malloc(buffer_size); if (audio_lmb == NULL) @@ -375,6 +440,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a audio_toq = audio_eoq = audio_lmb; audio_umb = audio_lmb + buffer_size; audio_occupancy = 0; // frames +*/ client_is_open = 0; @@ -411,13 +477,16 @@ int jack_delay(long *the_delay) { // but then a transfer could occur and we would get the buffer occupancy after another transfer // had occurred // so we could "lose" a full transfer (e.g. 1024 frames @ 44,100 fps ~ 23.2 milliseconds) + pthread_mutex_lock(&buffer_mutex); - int64_t time_now = get_absolute_time_in_fp(); - int64_t delta = time_now - time_of_latest_transfer; // this is the time back to the last time data - // was transferred into a jack buffer - size_t audio_occupancy_now = audio_occupancy; // this is the buffer occupancy before any - // subsequent transfer because transfer is blocked - // by the mutex + int64_t time_now = get_absolute_time_in_fp(); + // this is the time back to the last time data + // was transferred into a jack buffer + int64_t delta = time_now - time_of_latest_transfer; + // this is the buffer occupancy before any + // subsequent transfer because transfer is blocked + // by the mutex + size_t audio_occupancy_now = jack_ringbuffer_read_space(jackbuf); pthread_mutex_unlock(&buffer_mutex); int64_t frames_processed_since_latest_latency_check = (delta * 44100) >> 32; @@ -427,17 +496,18 @@ int jack_delay(long *the_delay) { base_latency = (latest_right_latency_range.min + latest_right_latency_range.max) / 2; *the_delay = base_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; // debug(1,"reporting a delay of %d frames",*the_delay); - return 0; } void jack_flush() { + debug(1, "It is not possible to safely flush a lock-free ringbuffer. Asking the process callback to do it..."); + flush_please = 1; +/* // debug(1,"jack flush"); - pthread_mutex_lock(&buffer_mutex); audio_toq = audio_eoq = audio_lmb; audio_umb = audio_lmb + buffer_size; audio_occupancy = 0; // frames - pthread_mutex_unlock(&buffer_mutex); +*/ } void jack_stop(void) { From 83c8857cf8a4396ab3dfc4ec58103e0e429236a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 20:27:49 +0000 Subject: [PATCH 04/48] try a solution for flush(). Compiled but not tested. --- audio_jack.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 1dc471ac..7e950457 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -177,20 +177,23 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a int frames_written = 0; int frames_required = 0; - jack_ringbuffer_get_read_vector(jackbuf, v); // an array of two elements because of possible ringbuffer wrap-around - for (i=0; i<2; i++) { - thisbuf = v[i].len / bytes_per_frame; - if (thisbuf > nframes) { - frames_required = nframes; - } else { - frames_required = thisbuf; + if (flush_please) { + jack_ringbuffer_read_advance(jackbuf, jack_ringbuffer_read_space(jackbuf)); + } else { + jack_ringbuffer_get_read_vector(jackbuf, v); // an array of two elements because of possible ringbuffer wrap-around + for (i=0; i<2; i++) { + thisbuf = v[i].len / bytes_per_frame; + if (thisbuf > nframes) { + frames_required = nframes; + } else { + frames_required = thisbuf; + } + deinterleave_and_convert(v[i].buf, &left_buffer[frames_written], &right_buffer[frames_written], frames_required); + frames_written = frames_required; + nframes -= frames_written; } - deinterleave_and_convert(v[i].buf, &left_buffer[frames_written], &right_buffer[frames_written], frames_required); - frames_written = frames_required; - nframes -= frames_written; + jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); } - jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); - // debug(1,"transferring %u frames",written); // Why are we doing this? The port latency should never change unless the JACK graph is reordered. Should happen on a graph reorder callback only. // jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); From 3e6ef0194f1eb20c38fd669ec7dada1ec7a760b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 20:39:03 +0000 Subject: [PATCH 05/48] Unset flush_please after flushing. Tentatively re-enable latency queries. Same problems... --- audio_jack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 7e950457..eb00b666 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -179,6 +179,7 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a if (flush_please) { jack_ringbuffer_read_advance(jackbuf, jack_ringbuffer_read_space(jackbuf)); + flush_please = 0; } else { jack_ringbuffer_get_read_vector(jackbuf, v); // an array of two elements because of possible ringbuffer wrap-around for (i=0; i<2; i++) { @@ -196,8 +197,8 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a } // debug(1,"transferring %u frames",written); // Why are we doing this? The port latency should never change unless the JACK graph is reordered. Should happen on a graph reorder callback only. - // jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); - // jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); + jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); + jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); // now, if there are any more frames to put into the buffer, fill them with // silence From a238ac54bb0842eb532a69a06a1f14577a591077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Thu, 14 Feb 2019 20:50:02 +0000 Subject: [PATCH 06/48] audio_occupancy_now in delay() needs to be in frames, not bytes. Still no luck... --- audio_jack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/audio_jack.c b/audio_jack.c index eb00b666..525eb64b 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -490,7 +490,7 @@ int jack_delay(long *the_delay) { // this is the buffer occupancy before any // subsequent transfer because transfer is blocked // by the mutex - size_t audio_occupancy_now = jack_ringbuffer_read_space(jackbuf); + size_t audio_occupancy_now = jack_ringbuffer_read_space(jackbuf) / bytes_per_frame; pthread_mutex_unlock(&buffer_mutex); int64_t frames_processed_since_latest_latency_check = (delta * 44100) >> 32; From 7ab566960c54d94f5af4f588ebdfc352c54e3f06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 15:53:43 +0000 Subject: [PATCH 07/48] move setting of time_of_latest_transfer to play(), we're now looking at ringbuffer writes, not jack output buffers, as those can't be locked to be tested atomically. --- audio_jack.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 525eb64b..4f1edad4 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -100,6 +100,8 @@ int play(void *buf, int samples) { bytes_to_transfer = samples * bytes_per_frame; pthread_mutex_lock(&buffer_mutex); // it's ok to lock here since we're not in the realtime callback bytes_transferred = jack_ringbuffer_write(jackbuf, buf, bytes_to_transfer); + // semantics change: we now measure the last time audio was moved into the ringbuffer, not the jack output buffers. + time_of_latest_transfer = get_absolute_time_in_fp(); pthread_mutex_unlock(&buffer_mutex); if (bytes_transferred < bytes_to_transfer) { debug(1, "JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); @@ -208,7 +210,6 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a frames_written++; nframes--; } - time_of_latest_transfer = get_absolute_time_in_fp(); // this is the only writer, no locking necessary. /* size_t frames_we_can_transfer = nframes; @@ -477,6 +478,10 @@ void jack_start(__attribute__((unused)) int i_sample_rate, int jack_delay(long *the_delay) { + // semantics change: we now look at the last transfer into the lock-free ringbuffer, not + // into the jack buffers directly (because locking those would violate real-time constraints). + // on average, that should lead to just a constant additional latency. the old comment still applies: + // without the mutex, we could get the time of what is the last transfer of data to a jack buffer, // but then a transfer could occur and we would get the buffer occupancy after another transfer // had occurred @@ -504,7 +509,7 @@ int jack_delay(long *the_delay) { } void jack_flush() { - debug(1, "It is not possible to safely flush a lock-free ringbuffer. Asking the process callback to do it..."); + debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); flush_please = 1; /* // debug(1,"jack flush"); From aca15a889b456751bfa24381a3e72dc2e507ac5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 15:56:08 +0000 Subject: [PATCH 08/48] remove comment - the "rolling" status of jack transport is irrelevant to shairport-sync. --- audio_jack.c | 1 - 1 file changed, 1 deletion(-) diff --git a/audio_jack.c b/audio_jack.c index 4f1edad4..7344d86f 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -283,7 +283,6 @@ void default_jack_set_latency_callback(jack_latency_callback_mode_t mode, int jack_is_running() { int reply = -1; // meaning jack is not running - // if the client is open and initialised, see if the status is "rolling" if (client_is_open) { // check if the ports have a zero latency -- if they both have, then it's disconnected. From 8347223986efd8994a8c335ca44045c31ec8459a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 16:08:10 +0000 Subject: [PATCH 09/48] Remove dubious latency check from jack_is_running(). For now, we assume JACK is always running, which makes this function pointless, as it always returns possitive. FIXME: understand why it is needed. --- audio_jack.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 7344d86f..1731ac17 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -287,14 +287,24 @@ int jack_is_running() { // check if the ports have a zero latency -- if they both have, then it's disconnected. - jack_latency_range_t left_latency_range, right_latency_range; - jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); - jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); + // FIXME: this causes a segfault when shairport-sync is exited with CTRL-C, because + // the client_is_open flag is stale by then. Also, this test is not necessary. + // shairport-sync should not worry what's reading its ports. As long as jack is alive, + // deliver audio, even if nothing is connected. This behaviour probably stems from + // the wish to not hog an audio device if not needed, which is no longer an issue with + // jack. Moreover, don't "conserve" CPU this way, because in a realtime system you want + // deterministic CPU load more than anything else. + // jack_latency_range_t left_latency_range, right_latency_range; + // jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); + // jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); // if ((left_latency_range.min == 0) && (left_latency_range.max == 0) && // (right_latency_range.min == 0) && (right_latency_range.max == 0)) { // reply = -2; // meaning Shairport Sync is not connected // } else { + + // FIXME: For now, we assume JACK is always running, as it should. + // Still need to understand why shairport-sync needs this function. reply = 0; // meaning jack is open and Shairport Sync is connected to it // } } From 9c57fe43ccdafb7aa3cf2edf14cb16a9c90897c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 16:41:20 +0000 Subject: [PATCH 10/48] Do not re-check JACK graph latency every time delay() is called. Do not register a latency callback with jack. --- audio_jack.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 1731ac17..23331e53 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -263,6 +263,7 @@ void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } +/* void default_jack_set_latency_callback(jack_latency_callback_mode_t mode, __attribute__((unused)) void *arg) { if (mode == JackPlaybackLatency) { @@ -280,6 +281,7 @@ void default_jack_set_latency_callback(jack_latency_callback_mode_t mode, debug(1, "playback latency callback: %" PRIu32 ".", jack_latency); } } +*/ int jack_is_running() { int reply = -1; // meaning jack is not running @@ -325,16 +327,19 @@ int jack_client_open_if_needed(void) { sample_rate = jack_get_sample_rate(client); // debug(1, "jackaudio sample rate = %" PRId32 ".", sample_rate); if (sample_rate == 44100) { - if (jack_set_latency_callback(client, default_jack_set_latency_callback, NULL) == 0) { + // FIXME: shairport-sync has no need for a latency callback, it's a leaf node with only outputs. + // If in the future some jack video player wants to resync to shairplay-sync audio, then yes, but + // the current usage seems to suggest a misunderstanding. +// if (jack_set_latency_callback(client, default_jack_set_latency_callback, NULL) == 0) { if (jack_activate(client)) { debug(1, "jackaudio cannot activate client"); } else { debug(2, "jackaudio client opened."); client_is_open = 1; } - } else { - debug(1, "jackaudio cannot set latency callback"); - } +// } else { +// debug(1, "jackaudio cannot set latency callback"); +// } } else { inform( "jackaudio is running at the wrong speed (%d) for Shairport Sync, which must be 44100", @@ -505,15 +510,22 @@ int jack_delay(long *the_delay) { // subsequent transfer because transfer is blocked // by the mutex size_t audio_occupancy_now = jack_ringbuffer_read_space(jackbuf) / bytes_per_frame; + debug(1, "JN: audio_occupancy_now is %d.", audio_occupancy_now); pthread_mutex_unlock(&buffer_mutex); int64_t frames_processed_since_latest_latency_check = (delta * 44100) >> 32; + + // FIXME: this should only be done if there was an actual change, i.e. on the jack graph reorder + // callback, to update a static variable which can be checked here. For now, use a fixed arbitrary value: + jack_nframes_t base_latency = 0; + // debug(1,"delta: %" PRId64 " frames.",frames_processed_since_latest_latency_check); - jack_nframes_t base_latency = (latest_left_latency_range.min + latest_left_latency_range.max) / 2; - if (base_latency == 0) - base_latency = (latest_right_latency_range.min + latest_right_latency_range.max) / 2; + + // jack_nframes_t base_latency = (latest_left_latency_range.min + latest_left_latency_range.max) / 2; + // if (base_latency == 0) + // base_latency = (latest_right_latency_range.min + latest_right_latency_range.max) / 2; *the_delay = base_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; - // debug(1,"reporting a delay of %d frames",*the_delay); + debug(1,"reporting a delay of %d frames",*the_delay); return 0; } From 6104c580f8ba3db54b025fa27887e7076413765d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 16:51:17 +0000 Subject: [PATCH 11/48] NOP: comment flush implementation --- audio_jack.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/audio_jack.c b/audio_jack.c index 23331e53..919b0d6a 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -180,8 +180,10 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a int frames_required = 0; if (flush_please) { + // we just move the read pointer ahead without doing anything with the data. jack_ringbuffer_read_advance(jackbuf, jack_ringbuffer_read_space(jackbuf)); flush_please = 0; + // since we don't change nframes, the whole buffer will be zeroed later. } else { jack_ringbuffer_get_read_vector(jackbuf, v); // an array of two elements because of possible ringbuffer wrap-around for (i=0; i<2; i++) { From 23d4ec2beecfbe218779312f0b8d6d0cff7613b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 17:19:07 +0000 Subject: [PATCH 12/48] Fix logic error in counting the frames written and pending. RESTORES AUDIO! --- audio_jack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 919b0d6a..544255a8 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -194,8 +194,8 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a frames_required = thisbuf; } deinterleave_and_convert(v[i].buf, &left_buffer[frames_written], &right_buffer[frames_written], frames_required); - frames_written = frames_required; - nframes -= frames_written; + frames_written += frames_required; + nframes -= frames_required; } jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); } From 444c114231c20758efd289544fc6d58be20bf0ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 17:58:50 +0000 Subject: [PATCH 13/48] NOP: remove commented-out code segments that were re-implemented --- audio_jack.c | 127 --------------------------------------------------- 1 file changed, 127 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 544255a8..3d3fadda 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -106,49 +106,9 @@ int play(void *buf, int samples) { if (bytes_transferred < bytes_to_transfer) { debug(1, "JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); } -/* - size_t space_to_end_of_buffer = audio_umb - audio_eoq; - if (space_to_end_of_buffer >= bytes_to_transfer) { - memcpy(audio_eoq, buf, bytes_to_transfer); - pthread_mutex_lock(&buffer_mutex); - audio_occupancy += samples; - audio_eoq += bytes_to_transfer; - pthread_mutex_unlock(&buffer_mutex); - } else { - memcpy(audio_eoq, buf, space_to_end_of_buffer); - buf += space_to_end_of_buffer; - memcpy(audio_lmb, buf, bytes_to_transfer - space_to_end_of_buffer); - pthread_mutex_lock(&buffer_mutex); - audio_occupancy += samples; - audio_eoq = audio_lmb + bytes_to_transfer - space_to_end_of_buffer; - pthread_mutex_unlock(&buffer_mutex); - } -*/ return 0; } -void deinterleave_and_convert_stream(const char *interleaved_frames, - const jack_default_audio_sample_t *jack_frame_buffer, - jack_nframes_t number_of_frames, enum ift_type side) { - jack_nframes_t i; - short *ifp = (short *)interleaved_frames; - jack_default_audio_sample_t *fp = (jack_default_audio_sample_t *)jack_frame_buffer; - if (side == IFT_frame_right_sample) - ifp++; - for (i = 0; i < number_of_frames; i++) { - short sample = *ifp; - jack_default_audio_sample_t converted_value; - if (sample >= 0) - converted_value = (1.0 * sample) / SHRT_MAX; - else - converted_value = -(1.0 * sample) / SHRT_MIN; - *fp = converted_value; - ifp++; - ifp++; - fp++; - } -} - static inline jack_default_audio_sample_t sample_conv(short sample) { return ((sample < 0) ? (-1.0 * sample / SHRT_MIN) : (1.0 * sample / SHRT_MAX)); } @@ -212,50 +172,6 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a frames_written++; nframes--; } - -/* - size_t frames_we_can_transfer = nframes; - // lock - pthread_mutex_lock(&buffer_mutex); - if (audio_occupancy < frames_we_can_transfer) { - frames_we_can_transfer = audio_occupancy; - // This means we effectively have underflow from the Shairport Sync source. - // In fact, it may be that there is nothing at all coming from the source, - // but the Shairport Sync client is open and active, so it must continue to output something. - } - - if (frames_we_can_transfer * 2 * 2 <= (size_t)(audio_umb - audio_toq)) { - // the bytes are all in a row in the audio buffer - deinterleave_and_convert_stream(audio_toq, &left_buffer[0], frames_we_can_transfer, - IFT_frame_left_sample); - deinterleave_and_convert_stream(audio_toq, &right_buffer[0], frames_we_can_transfer, - IFT_frame_right_sample); - audio_toq += frames_we_can_transfer * 2 * 2; - } else { - // the bytes are in two places in the audio buffer - size_t first_portion_to_write = (audio_umb - audio_toq) / (2 * 2); - if (first_portion_to_write != 0) { - deinterleave_and_convert_stream(audio_toq, &left_buffer[0], first_portion_to_write, - IFT_frame_left_sample); - deinterleave_and_convert_stream(audio_toq, &right_buffer[0], first_portion_to_write, - IFT_frame_right_sample); - } - deinterleave_and_convert_stream(audio_lmb, &left_buffer[first_portion_to_write], - frames_we_can_transfer - first_portion_to_write, - IFT_frame_left_sample); - deinterleave_and_convert_stream(audio_lmb, &right_buffer[first_portion_to_write], - frames_we_can_transfer - first_portion_to_write, - IFT_frame_right_sample); - audio_toq = audio_lmb + (frames_we_can_transfer - first_portion_to_write) * 2 * 2; - } - audio_occupancy -= frames_we_can_transfer; - jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); - jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); - time_of_latest_transfer = get_absolute_time_in_fp(); - pthread_mutex_unlock(&buffer_mutex); - // unlock - -*/ return 0; } @@ -265,26 +181,6 @@ void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } -/* -void default_jack_set_latency_callback(jack_latency_callback_mode_t mode, - __attribute__((unused)) void *arg) { - if (mode == JackPlaybackLatency) { - jack_latency_range_t left_latency_range, right_latency_range; - jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); - jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); - - jack_nframes_t b_latency = (left_latency_range.min + left_latency_range.max) / 2; - if (b_latency == 0) - b_latency = (right_latency_range.min + right_latency_range.max) / 2; - // jack_latency = b_latency; // actually, we are not interested in the latency of the jack - // devices connected... - // FIXME: yes, we are. - jack_latency = 0; - debug(1, "playback latency callback: %" PRIu32 ".", jack_latency); - } -} -*/ - int jack_is_running() { int reply = -1; // meaning jack is not running if (client_is_open) { @@ -329,19 +225,12 @@ int jack_client_open_if_needed(void) { sample_rate = jack_get_sample_rate(client); // debug(1, "jackaudio sample rate = %" PRId32 ".", sample_rate); if (sample_rate == 44100) { - // FIXME: shairport-sync has no need for a latency callback, it's a leaf node with only outputs. - // If in the future some jack video player wants to resync to shairplay-sync audio, then yes, but - // the current usage seems to suggest a misunderstanding. -// if (jack_set_latency_callback(client, default_jack_set_latency_callback, NULL) == 0) { if (jack_activate(client)) { debug(1, "jackaudio cannot activate client"); } else { debug(2, "jackaudio client opened."); client_is_open = 1; } -// } else { -// debug(1, "jackaudio cannot set latency callback"); -// } } else { inform( "jackaudio is running at the wrong speed (%d) for Shairport Sync, which must be 44100", @@ -453,16 +342,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); -/* - // allocate space for the audio buffer - audio_lmb = malloc(buffer_size); - if (audio_lmb == NULL) - die("Can't allocate %d bytes for jackaudio buffer.", buffer_size); - audio_toq = audio_eoq = audio_lmb; - audio_umb = audio_lmb + buffer_size; - audio_occupancy = 0; // frames -*/ - client_is_open = 0; // now, if selected, start a thread to automatically open a client when there is a server. @@ -534,12 +413,6 @@ int jack_delay(long *the_delay) { void jack_flush() { debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); flush_please = 1; -/* - // debug(1,"jack flush"); - audio_toq = audio_eoq = audio_lmb; - audio_umb = audio_lmb + buffer_size; - audio_occupancy = 0; // frames -*/ } void jack_stop(void) { From 120e95fc662cb316a8c5853c45f18e6ae2a4733e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:06:36 +0000 Subject: [PATCH 14/48] Add parentheses around macro to avoid breakage with stronger-binding operators. --- audio_jack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/audio_jack.c b/audio_jack.c index 3d3fadda..667ace96 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -41,7 +41,7 @@ enum ift_type { // Two-channel, 16bit audio: static const int bytes_per_frame = 4; // Four seconds buffer -- should be plenty -#define buffer_size 44100 * 4 * bytes_per_frame +#define buffer_size (44100 * 4 * bytes_per_frame) static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; From aefd7cdb3314fc07f3296163192ee2a17d6a244e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:09:36 +0000 Subject: [PATCH 15/48] remove more dead code and obsolete comments --- audio_jack.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 667ace96..5f1a4adc 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -46,12 +46,6 @@ static const int bytes_per_frame = 4; static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; -/* -char *audio_lmb, *audio_umb, *audio_toq, *audio_eoq; - -size_t audio_occupancy; // this is in frames, not bytes. A frame is a left and - // right sample, each 16 bits, hence 4 bytes -*/ pthread_t *open_client_if_necessary_thread = NULL; int jack_init(int, char **); @@ -334,7 +328,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config.jack_right_channel_name == NULL) config.jack_right_channel_name = strdup("right"); - jackbuf = jack_ringbuffer_create(buffer_size); // make the jack ringbuffer the same size as audio_lmb + jackbuf = jack_ringbuffer_create(buffer_size); if (jackbuf == NULL) die("Can't allocate %d bytes for the JACK ringbuffer.", buffer_size); jack_ringbuffer_mlock(jackbuf); // lock buffer into memory so that it never gets paged out From ec13a2bb3a7fbb0b272bcdd7ae4da79195605f8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:19:44 +0000 Subject: [PATCH 16/48] Do not query the JACK port latency on every process() call. It can only ever change when the graph is reordered. --- audio_jack.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 5f1a4adc..c0c4fab1 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -153,11 +153,6 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a } jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); } - // debug(1,"transferring %u frames",written); - // Why are we doing this? The port latency should never change unless the JACK graph is reordered. Should happen on a graph reorder callback only. - jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); - jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); - // now, if there are any more frames to put into the buffer, fill them with // silence while (nframes > 0) { From a3201ff290373a1720fa1ef30122e05d9b140cf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:22:33 +0000 Subject: [PATCH 17/48] Declare deinterleaving function static, it's nobody else's business. --- audio_jack.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index c0c4fab1..5cde2213 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -107,10 +107,10 @@ static inline jack_default_audio_sample_t sample_conv(short sample) { return ((sample < 0) ? (-1.0 * sample / SHRT_MIN) : (1.0 * sample / SHRT_MAX)); } -void deinterleave_and_convert(const char *interleaved_frames, - jack_default_audio_sample_t * const jack_buffer_left, - jack_default_audio_sample_t * const jack_buffer_right, - jack_nframes_t nframes) { +static void deinterleave_and_convert(const char *interleaved_frames, + jack_default_audio_sample_t * const jack_buffer_left, + jack_default_audio_sample_t * const jack_buffer_right, + jack_nframes_t nframes) { jack_nframes_t i; short *ifp = (short *)interleaved_frames; // we're dealing with 16bit audio here jack_default_audio_sample_t *fpl = jack_buffer_left; From d7c7365bb11c4d8e47f4244986d46ae9b603c87a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:31:23 +0000 Subject: [PATCH 18/48] comment out some debug statements not relevant to users --- audio_jack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 5cde2213..f5aa7cea 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -380,7 +380,7 @@ int jack_delay(long *the_delay) { // subsequent transfer because transfer is blocked // by the mutex size_t audio_occupancy_now = jack_ringbuffer_read_space(jackbuf) / bytes_per_frame; - debug(1, "JN: audio_occupancy_now is %d.", audio_occupancy_now); + // debug(1, "audio_occupancy_now is %d.", audio_occupancy_now); pthread_mutex_unlock(&buffer_mutex); int64_t frames_processed_since_latest_latency_check = (delta * 44100) >> 32; @@ -395,12 +395,12 @@ int jack_delay(long *the_delay) { // if (base_latency == 0) // base_latency = (latest_right_latency_range.min + latest_right_latency_range.max) / 2; *the_delay = base_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; - debug(1,"reporting a delay of %d frames",*the_delay); + // debug(1,"reporting a delay of %d frames",*the_delay); return 0; } void jack_flush() { - debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); + // debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); flush_please = 1; } From 9520e891836eaeaed41127a1601980e4a72a0d28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:39:20 +0000 Subject: [PATCH 19/48] Declare all functions static that are not callbacks and not in audio_jack API struct. If there are any calls from the outside I haven't thought of, that should make them fail. --- audio_jack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index f5aa7cea..7bd513b0 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -200,7 +200,7 @@ int jack_is_running() { return reply; } -int jack_client_open_if_needed(void) { +static int jack_client_open_if_needed(void) { pthread_mutex_lock(&client_mutex); if (client_is_open == 0) { jack_status_t status; @@ -231,7 +231,7 @@ int jack_client_open_if_needed(void) { return client_is_open; } -void jack_close(void) { +static void jack_close(void) { pthread_mutex_lock(&client_mutex); if (client_is_open) { if (jack_deactivate(client)) @@ -254,7 +254,7 @@ void jack_deinit() { } } -void *open_client_if_necessary_thread_function(void *arg) { +static void *open_client_if_necessary_thread_function(void *arg) { int *interval = (int *)arg; while (*interval != 0) { if (client_is_open == 0) { From 91a8553a02cf0451ba1b9884660a4691b7bba2a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:43:51 +0000 Subject: [PATCH 20/48] Declare all JACK callbacks static (they are accessed via function pointers only). --- audio_jack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 7bd513b0..680ce8cb 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -121,7 +121,7 @@ static void deinterleave_and_convert(const char *interleaved_frames, } } -int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *arg) { +static int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *arg) { jack_default_audio_sample_t *left_buffer = (jack_default_audio_sample_t *)jack_port_get_buffer(left_port, nframes); @@ -166,9 +166,9 @@ int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *a // FIXME: set_graph_order_callback(), recompute latencies here! -void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } +static void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } -void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } +static void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } int jack_is_running() { int reply = -1; // meaning jack is not running From 89a8add7af36d5525dbaf1eebbc38e715431952b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 18:57:15 +0000 Subject: [PATCH 21/48] NOP: move external API functions to the end, ordered by struct audio_output --- audio_jack.c | 130 ++++++++++++++++++++++++++------------------------- 1 file changed, 66 insertions(+), 64 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 680ce8cb..e4d97439 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -87,21 +87,6 @@ static int flush_please = 0; jack_latency_range_t latest_left_latency_range, latest_right_latency_range; int64_t time_of_latest_transfer; -int play(void *buf, int samples) { - // debug(1,"jack_play of %d samples.",samples); - // copy the samples into the queue - size_t bytes_to_transfer, bytes_transferred; - bytes_to_transfer = samples * bytes_per_frame; - pthread_mutex_lock(&buffer_mutex); // it's ok to lock here since we're not in the realtime callback - bytes_transferred = jack_ringbuffer_write(jackbuf, buf, bytes_to_transfer); - // semantics change: we now measure the last time audio was moved into the ringbuffer, not the jack output buffers. - time_of_latest_transfer = get_absolute_time_in_fp(); - pthread_mutex_unlock(&buffer_mutex); - if (bytes_transferred < bytes_to_transfer) { - debug(1, "JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); - } - return 0; -} static inline jack_default_audio_sample_t sample_conv(short sample) { return ((sample < 0) ? (-1.0 * sample / SHRT_MIN) : (1.0 * sample / SHRT_MAX)); @@ -170,35 +155,6 @@ static void default_jack_error_callback(const char *desc) { debug(2, "jackd erro static void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } -int jack_is_running() { - int reply = -1; // meaning jack is not running - if (client_is_open) { - - // check if the ports have a zero latency -- if they both have, then it's disconnected. - - // FIXME: this causes a segfault when shairport-sync is exited with CTRL-C, because - // the client_is_open flag is stale by then. Also, this test is not necessary. - // shairport-sync should not worry what's reading its ports. As long as jack is alive, - // deliver audio, even if nothing is connected. This behaviour probably stems from - // the wish to not hog an audio device if not needed, which is no longer an issue with - // jack. Moreover, don't "conserve" CPU this way, because in a realtime system you want - // deterministic CPU load more than anything else. - // jack_latency_range_t left_latency_range, right_latency_range; - // jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); - // jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); - - // if ((left_latency_range.min == 0) && (left_latency_range.max == 0) && - // (right_latency_range.min == 0) && (right_latency_range.max == 0)) { - // reply = -2; // meaning Shairport Sync is not connected - // } else { - - // FIXME: For now, we assume JACK is always running, as it should. - // Still need to understand why shairport-sync needs this function. - reply = 0; // meaning jack is open and Shairport Sync is connected to it - // } - } - return reply; -} static int jack_client_open_if_needed(void) { pthread_mutex_lock(&client_mutex); @@ -243,17 +199,6 @@ static void jack_close(void) { pthread_mutex_unlock(&client_mutex); } -void jack_deinit() { - jack_close(); - if (open_client_if_necessary_thread) { - pthread_cancel(*open_client_if_necessary_thread); - free((char *)open_client_if_necessary_thread); - - jack_ringbuffer_free(jackbuf); - - } -} - static void *open_client_if_necessary_thread_function(void *arg) { int *interval = (int *)arg; while (*interval != 0) { @@ -351,6 +296,17 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a return 0; } +void jack_deinit() { + jack_close(); + if (open_client_if_necessary_thread) { + pthread_cancel(*open_client_if_necessary_thread); + free((char *)open_client_if_necessary_thread); + + jack_ringbuffer_free(jackbuf); + + } +} + void jack_start(__attribute__((unused)) int i_sample_rate, __attribute__((unused)) int i_sample_format) { // debug(1, "jack start"); @@ -360,6 +316,47 @@ void jack_start(__attribute__((unused)) int i_sample_rate, debug(1, "cannot open a jack client for a play session"); } +void jack_stop(void) { + // debug(1, "jack stop"); + if (config.jack_auto_client_disconnect) + jack_close(); +} + +int jack_is_running() { + int reply = -1; // meaning jack is not running + if (client_is_open) { + + // check if the ports have a zero latency -- if they both have, then it's disconnected. + + // FIXME: this causes a segfault when shairport-sync is exited with CTRL-C, because + // the client_is_open flag is stale by then. Also, this test is not necessary. + // shairport-sync should not worry what's reading its ports. As long as jack is alive, + // deliver audio, even if nothing is connected. This behaviour probably stems from + // the wish to not hog an audio device if not needed, which is no longer an issue with + // jack. Moreover, don't "conserve" CPU this way, because in a realtime system you want + // deterministic CPU load more than anything else. + // jack_latency_range_t left_latency_range, right_latency_range; + // jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); + // jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); + + // if ((left_latency_range.min == 0) && (left_latency_range.max == 0) && + // (right_latency_range.min == 0) && (right_latency_range.max == 0)) { + // reply = -2; // meaning Shairport Sync is not connected + // } else { + + // FIXME: For now, we assume JACK is always running, as it should. + // Still need to understand why shairport-sync needs this function. + reply = 0; // meaning jack is open and Shairport Sync is connected to it + // } + } + return reply; +} + +void jack_flush() { + // debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); + flush_please = 1; +} + int jack_delay(long *the_delay) { // semantics change: we now look at the last transfer into the lock-free ringbuffer, not @@ -399,13 +396,18 @@ int jack_delay(long *the_delay) { return 0; } -void jack_flush() { - // debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); - flush_please = 1; -} - -void jack_stop(void) { - // debug(1, "jack stop"); - if (config.jack_auto_client_disconnect) - jack_close(); +int play(void *buf, int samples) { + // debug(1,"jack_play of %d samples.",samples); + // copy the samples into the queue + size_t bytes_to_transfer, bytes_transferred; + bytes_to_transfer = samples * bytes_per_frame; + pthread_mutex_lock(&buffer_mutex); // it's ok to lock here since we're not in the realtime callback + bytes_transferred = jack_ringbuffer_write(jackbuf, buf, bytes_to_transfer); + // semantics change: we now measure the last time audio was moved into the ringbuffer, not the jack output buffers. + time_of_latest_transfer = get_absolute_time_in_fp(); + pthread_mutex_unlock(&buffer_mutex); + if (bytes_transferred < bytes_to_transfer) { + debug(1, "JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); + } + return 0; } From 0b589cbe6d13d43faa252d70156c7a2e1323734c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 19:29:24 +0000 Subject: [PATCH 22/48] Remove option to configure port names. This is very unusual for a JACK client, nothing is gained by offering this, and it might confuse users. --- audio_jack.c | 16 ++-------------- common.h | 2 +- scripts/shairport-sync.conf | 2 -- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index e4d97439..15f1f49d 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -163,9 +163,9 @@ static int jack_client_open_if_needed(void) { client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); if (client) { jack_set_process_callback(client, jack_stream_write_cb, 0); - left_port = jack_port_register(client, config.jack_left_channel_name, JACK_DEFAULT_AUDIO_TYPE, + left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); - right_port = jack_port_register(client, config.jack_right_channel_name, + right_port = jack_port_register(client, "out_R", JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); sample_rate = jack_get_sample_rate(client); // debug(1, "jackaudio sample rate = %" PRId32 ".", sample_rate); @@ -235,14 +235,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config_lookup_string(config.cfg, "jack.client_name", &str)) { config.jack_client_name = (char *)str; } - /* Get the Left Channel Name. */ - if (config_lookup_string(config.cfg, "jack.left_channel_name", &str)) { - config.jack_left_channel_name = (char *)str; - } - /* Get the Right Channel Name. */ - if (config_lookup_string(config.cfg, "jack.right_channel_name", &str)) { - config.jack_right_channel_name = (char *)str; - } /* See if we should attempt to connect to the jack server automatically, and, if so, how often * we should try. */ @@ -263,10 +255,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config.jack_client_name == NULL) config.jack_client_name = strdup("Shairport Sync"); - if (config.jack_left_channel_name == NULL) - config.jack_left_channel_name = strdup("left"); - if (config.jack_right_channel_name == NULL) - config.jack_right_channel_name = strdup("right"); jackbuf = jack_ringbuffer_create(buffer_size); if (jackbuf == NULL) diff --git a/common.h b/common.h index 78c9ddda..a43c2b85 100644 --- a/common.h +++ b/common.h @@ -232,7 +232,7 @@ typedef struct { double diagnostic_drop_packet_fraction; // pseudo randomly drop this fraction of packets, for // debugging. Currently audio packets only... #ifdef CONFIG_JACK - char *jack_client_name, *jack_left_channel_name, *jack_right_channel_name; + char *jack_client_name; int jack_auto_client_open_interval; // will try to open a client automatically every second int jack_auto_client_disconnect; // will disconnect from the server on end of session if set, // normally clear. diff --git a/scripts/shairport-sync.conf b/scripts/shairport-sync.conf index bf6ce4ba..cb927380 100644 --- a/scripts/shairport-sync.conf +++ b/scripts/shairport-sync.conf @@ -109,8 +109,6 @@ pa = jack = { // client_name = "Shairport Sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. -// left_channel_name = "left"; //Set this to the name of the left output port that should appear in "Connections" when Shairport Sync is active. -// right_channel_name = "right"; //Set this to the name of the right output port that should appear in "Connections" when Shairport Sync is active. // auto_client_open_interval = 1; // Set this to the interval in seconds between automatic attempts to open a client on the jack server. Set to zero to disable this behaviour. // auto_client_disconnect = "no"; // Set this to "yes" to close the jack client automatically at then end of a play session. Default is no -- the client stays connected. }; From adefcc698ccdd0164f834b233135b07ccb6f46c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 19:34:23 +0000 Subject: [PATCH 23/48] remove unused globals --- audio_jack.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 15f1f49d..d825cacc 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -33,11 +33,6 @@ #include #include -enum ift_type { - IFT_frame_left_sample = 0, - IFT_frame_right_sample, -} ift_type; - // Two-channel, 16bit audio: static const int bytes_per_frame = 4; // Four seconds buffer -- should be plenty @@ -74,8 +69,6 @@ audio_output audio_jack = {.name = "jack", jack_port_t *left_port; jack_port_t *right_port; -long offset = 0; - int client_is_open; jack_client_t *client; jack_nframes_t sample_rate; From 63b3381a27906859e7a2932e6bebe3979e7e075d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 22:59:31 +0000 Subject: [PATCH 24/48] Fail early and fail deadly (AFAIK there is no fallback to another audio backend, so any severe JACK issue should be fatal). --- audio_jack.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index d825cacc..20e3b37f 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -148,32 +148,29 @@ static void default_jack_error_callback(const char *desc) { debug(2, "jackd erro static void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } - static int jack_client_open_if_needed(void) { pthread_mutex_lock(&client_mutex); if (client_is_open == 0) { jack_status_t status; client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); - if (client) { - jack_set_process_callback(client, jack_stream_write_cb, 0); - left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - right_port = jack_port_register(client, "out_R", - JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); - sample_rate = jack_get_sample_rate(client); - // debug(1, "jackaudio sample rate = %" PRId32 ".", sample_rate); - if (sample_rate == 44100) { - if (jack_activate(client)) { - debug(1, "jackaudio cannot activate client"); - } else { - debug(2, "jackaudio client opened."); - client_is_open = 1; - } - } else { - inform( - "jackaudio is running at the wrong speed (%d) for Shairport Sync, which must be 44100", - sample_rate); - } + if (!client) { + die("Could not start JACK server. JackStatus is %x", status); + } + sample_rate = jack_get_sample_rate(client); + if (sample_rate != 44100) { + die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", + sample_rate); + } + jack_set_process_callback(client, jack_stream_write_cb, 0); + left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, 0); + right_port = jack_port_register(client, "out_R", + JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); + if (jack_activate(client)) { + die("Could not activate %s JACK client.", config.jack_client_name); + } else { + debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); + client_is_open = 1; } } pthread_mutex_unlock(&client_mutex); From edd699dfbce4e82d84b2891a28f7e1af034acd2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:08:04 +0000 Subject: [PATCH 25/48] Remove auto_client_open_interval option, always open JACK connection immediately or fail. Remove client creation thread and wrapper function. --- audio_jack.c | 49 ++----------------------------------- common.h | 1 - scripts/shairport-sync.conf | 1 - 3 files changed, 2 insertions(+), 49 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 20e3b37f..4071c39e 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -189,25 +189,12 @@ static void jack_close(void) { pthread_mutex_unlock(&client_mutex); } -static void *open_client_if_necessary_thread_function(void *arg) { - int *interval = (int *)arg; - while (*interval != 0) { - if (client_is_open == 0) { - debug(1, "Try to open the jack client"); - jack_client_open_if_needed(); - } - sleep(*interval); - } - pthread_exit(NULL); -} - int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { config.audio_backend_latency_offset = 0; config.audio_backend_buffer_desired_length = 0.500; config.audio_backend_buffer_interpolation_threshold_in_seconds = 0.25; // below this, soxr interpolation will not occur -- it'll be basic interpolation // instead. - config.jack_auto_client_open_interval = 1; // check every second // get settings from settings file first, allow them to be overridden by // command line options @@ -220,24 +207,11 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a // now the specific options if (config.cfg != NULL) { const char *str; - int value; /* Get the Client Name. */ if (config_lookup_string(config.cfg, "jack.client_name", &str)) { config.jack_client_name = (char *)str; } - /* See if we should attempt to connect to the jack server automatically, and, if so, how often - * we should try. */ - if (config_lookup_int(config.cfg, "jack.auto_client_open_interval", &value)) { - if ((value < 0) || (value > 300)) - debug(1, - "Invalid jack auto_client_open_interval \"%sd\". It should be between 0 and 300, " - "default is %d.", - value, config.jack_auto_client_open_interval); - else - config.jack_auto_client_open_interval = value; - } - /* See if we should close the client at then end of a play session. */ config_set_lookup_bool(config.cfg, "jack.auto_client_disconnect", &config.jack_auto_client_disconnect); @@ -256,33 +230,14 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a client_is_open = 0; - // now, if selected, start a thread to automatically open a client when there is a server. - if (config.jack_auto_client_open_interval != 0) { - open_client_if_necessary_thread = malloc(sizeof(pthread_t)); - if (open_client_if_necessary_thread == NULL) { - debug(1, "Couldn't allocate space for jack server scanner thread"); - jack_client_open_if_needed(); - } else { - pthread_create(open_client_if_necessary_thread, NULL, - open_client_if_necessary_thread_function, - &config.jack_auto_client_open_interval); - } - } else { - jack_client_open_if_needed(); - } + jack_client_open_if_needed(); return 0; } void jack_deinit() { jack_close(); - if (open_client_if_necessary_thread) { - pthread_cancel(*open_client_if_necessary_thread); - free((char *)open_client_if_necessary_thread); - - jack_ringbuffer_free(jackbuf); - - } + jack_ringbuffer_free(jackbuf); } void jack_start(__attribute__((unused)) int i_sample_rate, diff --git a/common.h b/common.h index a43c2b85..b91b3a00 100644 --- a/common.h +++ b/common.h @@ -233,7 +233,6 @@ typedef struct { // debugging. Currently audio packets only... #ifdef CONFIG_JACK char *jack_client_name; - int jack_auto_client_open_interval; // will try to open a client automatically every second int jack_auto_client_disconnect; // will disconnect from the server on end of session if set, // normally clear. #endif diff --git a/scripts/shairport-sync.conf b/scripts/shairport-sync.conf index cb927380..cfddd414 100644 --- a/scripts/shairport-sync.conf +++ b/scripts/shairport-sync.conf @@ -109,7 +109,6 @@ pa = jack = { // client_name = "Shairport Sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. -// auto_client_open_interval = 1; // Set this to the interval in seconds between automatic attempts to open a client on the jack server. Set to zero to disable this behaviour. // auto_client_disconnect = "no"; // Set this to "yes" to close the jack client automatically at then end of a play session. Default is no -- the client stays connected. }; From 8c5e85c87c2c65e5c1305cd22388253d51d04508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:14:31 +0000 Subject: [PATCH 26/48] Remove audio_client_disconnect option. This is not good behaviour for a JACK client, and adds nothing. --- audio_jack.c | 6 ------ common.h | 2 -- scripts/shairport-sync.conf | 1 - 3 files changed, 9 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 4071c39e..e9730184 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -211,10 +211,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config_lookup_string(config.cfg, "jack.client_name", &str)) { config.jack_client_name = (char *)str; } - - /* See if we should close the client at then end of a play session. */ - config_set_lookup_bool(config.cfg, "jack.auto_client_disconnect", - &config.jack_auto_client_disconnect); } if (config.jack_client_name == NULL) @@ -251,8 +247,6 @@ void jack_start(__attribute__((unused)) int i_sample_rate, void jack_stop(void) { // debug(1, "jack stop"); - if (config.jack_auto_client_disconnect) - jack_close(); } int jack_is_running() { diff --git a/common.h b/common.h index b91b3a00..9a824cbf 100644 --- a/common.h +++ b/common.h @@ -233,8 +233,6 @@ typedef struct { // debugging. Currently audio packets only... #ifdef CONFIG_JACK char *jack_client_name; - int jack_auto_client_disconnect; // will disconnect from the server on end of session if set, - // normally clear. #endif } shairport_cfg; diff --git a/scripts/shairport-sync.conf b/scripts/shairport-sync.conf index cfddd414..c06c32e7 100644 --- a/scripts/shairport-sync.conf +++ b/scripts/shairport-sync.conf @@ -109,7 +109,6 @@ pa = jack = { // client_name = "Shairport Sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. -// auto_client_disconnect = "no"; // Set this to "yes" to close the jack client automatically at then end of a play session. Default is no -- the client stays connected. }; // Parameters for the "pipe" audio back end, a back end that directs raw CD-style audio output to a pipe. No interpolation is done. From ec68c677e0bd80bd29058b4c01462611f8e10011 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:17:50 +0000 Subject: [PATCH 27/48] Remove jack_stop() function pointer from API struct, since it does nothing. Checked, no segfault on program termination. --- audio_jack.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index e9730184..0221d043 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -57,7 +57,7 @@ audio_output audio_jack = {.name = "jack", .init = &jack_init, .deinit = &jack_deinit, .start = &jack_start, - .stop = &jack_stop, + .stop = NULL, .is_running = &jack_is_running, .flush = &jack_flush, .delay = &jack_delay, @@ -245,10 +245,6 @@ void jack_start(__attribute__((unused)) int i_sample_rate, debug(1, "cannot open a jack client for a play session"); } -void jack_stop(void) { - // debug(1, "jack stop"); -} - int jack_is_running() { int reply = -1; // meaning jack is not running if (client_is_open) { From 1906bfd727c2348aba71e1e8b7c22cd4264b71f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:38:01 +0000 Subject: [PATCH 28/48] Remove indirections open_if_needed() and close(), can now be integrated into API calls init and deinit. --- audio_jack.c | 83 ++++++++++++++++++++++------------------------------ 1 file changed, 35 insertions(+), 48 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 0221d043..b67e16ca 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -148,47 +148,6 @@ static void default_jack_error_callback(const char *desc) { debug(2, "jackd erro static void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } -static int jack_client_open_if_needed(void) { - pthread_mutex_lock(&client_mutex); - if (client_is_open == 0) { - jack_status_t status; - client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); - if (!client) { - die("Could not start JACK server. JackStatus is %x", status); - } - sample_rate = jack_get_sample_rate(client); - if (sample_rate != 44100) { - die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", - sample_rate); - } - jack_set_process_callback(client, jack_stream_write_cb, 0); - left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - right_port = jack_port_register(client, "out_R", - JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); - if (jack_activate(client)) { - die("Could not activate %s JACK client.", config.jack_client_name); - } else { - debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); - client_is_open = 1; - } - } - pthread_mutex_unlock(&client_mutex); - return client_is_open; -} - -static void jack_close(void) { - pthread_mutex_lock(&client_mutex); - if (client_is_open) { - if (jack_deactivate(client)) - debug(1, "Error deactivating jack client"); - if (jack_client_close(client)) - debug(1, "Error closing jack client"); - client_is_open = 0; - } - pthread_mutex_unlock(&client_mutex); -} - int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { config.audio_backend_latency_offset = 0; config.audio_backend_buffer_desired_length = 0.500; @@ -226,23 +185,51 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a client_is_open = 0; - jack_client_open_if_needed(); + pthread_mutex_lock(&client_mutex); + if (client_is_open == 0) { + jack_status_t status; + client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); + if (!client) { + die("Could not start JACK server. JackStatus is %x", status); + } + sample_rate = jack_get_sample_rate(client); + if (sample_rate != 44100) { + die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", + sample_rate); + } + jack_set_process_callback(client, jack_stream_write_cb, 0); + left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, 0); + right_port = jack_port_register(client, "out_R", + JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); + if (jack_activate(client)) { + die("Could not activate %s JACK client.", config.jack_client_name); + } else { + debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); + client_is_open = 1; + } + } + pthread_mutex_unlock(&client_mutex); return 0; } void jack_deinit() { - jack_close(); + pthread_mutex_lock(&client_mutex); + if (client_is_open) { + if (jack_deactivate(client)) + debug(1, "Error deactivating jack client"); + if (jack_client_close(client)) + debug(1, "Error closing jack client"); + client_is_open = 0; + } + pthread_mutex_unlock(&client_mutex); jack_ringbuffer_free(jackbuf); } void jack_start(__attribute__((unused)) int i_sample_rate, __attribute__((unused)) int i_sample_format) { - // debug(1, "jack start"); - // see if the client is running. If not, try to open and initialise it - - if (jack_client_open_if_needed() == 0) - debug(1, "cannot open a jack client for a play session"); + // nothing to do, JACK client has already been set up at jack_init() } int jack_is_running() { From 753cc07777abed3f7953720b55458b7e80bf3fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:39:53 +0000 Subject: [PATCH 29/48] Remove optional is_running() API call, since it doesn't do anything useful. We are either already running along just fine, or we are throughly dead. --- audio_jack.c | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index b67e16ca..c04853f0 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -58,7 +58,7 @@ audio_output audio_jack = {.name = "jack", .deinit = &jack_deinit, .start = &jack_start, .stop = NULL, - .is_running = &jack_is_running, + .is_running = NULL, .flush = &jack_flush, .delay = &jack_delay, .play = &play, @@ -232,36 +232,6 @@ void jack_start(__attribute__((unused)) int i_sample_rate, // nothing to do, JACK client has already been set up at jack_init() } -int jack_is_running() { - int reply = -1; // meaning jack is not running - if (client_is_open) { - - // check if the ports have a zero latency -- if they both have, then it's disconnected. - - // FIXME: this causes a segfault when shairport-sync is exited with CTRL-C, because - // the client_is_open flag is stale by then. Also, this test is not necessary. - // shairport-sync should not worry what's reading its ports. As long as jack is alive, - // deliver audio, even if nothing is connected. This behaviour probably stems from - // the wish to not hog an audio device if not needed, which is no longer an issue with - // jack. Moreover, don't "conserve" CPU this way, because in a realtime system you want - // deterministic CPU load more than anything else. - // jack_latency_range_t left_latency_range, right_latency_range; - // jack_port_get_latency_range(left_port, JackPlaybackLatency, &left_latency_range); - // jack_port_get_latency_range(right_port, JackPlaybackLatency, &right_latency_range); - - // if ((left_latency_range.min == 0) && (left_latency_range.max == 0) && - // (right_latency_range.min == 0) && (right_latency_range.max == 0)) { - // reply = -2; // meaning Shairport Sync is not connected - // } else { - - // FIXME: For now, we assume JACK is always running, as it should. - // Still need to understand why shairport-sync needs this function. - reply = 0; // meaning jack is open and Shairport Sync is connected to it - // } - } - return reply; -} - void jack_flush() { // debug(1, "Only the consumer can safely flush a lock-free ringbuffer. Asking the process callback to do it..."); flush_please = 1; From 2826756938aeccbdff8cb602c8383d022604eaa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:43:00 +0000 Subject: [PATCH 30/48] Remove global client_is_open flag, always execute former conditional statements. We know we're open. --- audio_jack.c | 56 +++++++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index c04853f0..f6f8b799 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -48,7 +48,6 @@ void jack_deinit(void); void jack_start(int, int); int play(void *, int); void jack_stop(void); -int jack_is_running(void); int jack_delay(long *); void jack_flush(void); @@ -69,7 +68,6 @@ audio_output audio_jack = {.name = "jack", jack_port_t *left_port; jack_port_t *right_port; -int client_is_open; jack_client_t *client; jack_nframes_t sample_rate; jack_nframes_t jack_latency; @@ -183,31 +181,26 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); - client_is_open = 0; - pthread_mutex_lock(&client_mutex); - if (client_is_open == 0) { - jack_status_t status; - client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); - if (!client) { - die("Could not start JACK server. JackStatus is %x", status); - } - sample_rate = jack_get_sample_rate(client); - if (sample_rate != 44100) { - die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", - sample_rate); - } - jack_set_process_callback(client, jack_stream_write_cb, 0); - left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - right_port = jack_port_register(client, "out_R", - JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); - if (jack_activate(client)) { - die("Could not activate %s JACK client.", config.jack_client_name); - } else { - debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); - client_is_open = 1; - } + jack_status_t status; + client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); + if (!client) { + die("Could not start JACK server. JackStatus is %x", status); + } + sample_rate = jack_get_sample_rate(client); + if (sample_rate != 44100) { + die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", + sample_rate); + } + jack_set_process_callback(client, jack_stream_write_cb, 0); + left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, 0); + right_port = jack_port_register(client, "out_R", + JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); + if (jack_activate(client)) { + die("Could not activate %s JACK client.", config.jack_client_name); + } else { + debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); } pthread_mutex_unlock(&client_mutex); @@ -216,13 +209,10 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a void jack_deinit() { pthread_mutex_lock(&client_mutex); - if (client_is_open) { - if (jack_deactivate(client)) - debug(1, "Error deactivating jack client"); - if (jack_client_close(client)) - debug(1, "Error closing jack client"); - client_is_open = 0; - } + if (jack_deactivate(client)) + debug(1, "Error deactivating jack client"); + if (jack_client_close(client)) + debug(1, "Error closing jack client"); pthread_mutex_unlock(&client_mutex); jack_ringbuffer_free(jackbuf); } From 8e46c727d59334b87ea0f639160e92349f5c81d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:45:12 +0000 Subject: [PATCH 31/48] Remove client_mutex, since JACK opening and closing is now single-threaded. --- audio_jack.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index f6f8b799..95759536 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -39,7 +39,6 @@ static const int bytes_per_frame = 4; #define buffer_size (44100 * 4 * bytes_per_frame) static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; -static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; pthread_t *open_client_if_necessary_thread = NULL; @@ -181,7 +180,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); - pthread_mutex_lock(&client_mutex); jack_status_t status; client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); if (!client) { @@ -202,18 +200,15 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a } else { debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); } - pthread_mutex_unlock(&client_mutex); return 0; } void jack_deinit() { - pthread_mutex_lock(&client_mutex); if (jack_deactivate(client)) debug(1, "Error deactivating jack client"); if (jack_client_close(client)) debug(1, "Error closing jack client"); - pthread_mutex_unlock(&client_mutex); jack_ringbuffer_free(jackbuf); } From 55797c83877c0209f8a0ea666d6f755805d7cd64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Fri, 15 Feb 2019 23:51:11 +0000 Subject: [PATCH 32/48] Remove unused headers. will come back once we implement command line parsing, the others may well be gone for good. --- audio_jack.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 95759536..8d176759 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -20,17 +20,11 @@ #include "audio.h" #include "common.h" #include -#include #include -#include #include -#include -#include #include -#include #include -#include #include // Two-channel, 16bit audio: @@ -215,6 +209,9 @@ void jack_deinit() { void jack_start(__attribute__((unused)) int i_sample_rate, __attribute__((unused)) int i_sample_format) { // nothing to do, JACK client has already been set up at jack_init() + // also, we have no say over the sample rate or sample format of JACK + // we convert the 16bit samples to float, and die if the sample rate is != 44k1. + // FIXME: later, resampling would be nice. Fold into soxr if possible } void jack_flush() { From 0009fcddf6c3f9c64c629e4bf086a1677ff2db1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 00:53:04 +0000 Subject: [PATCH 33/48] Revert "Remove client_mutex, since JACK opening and closing is now single-threaded." That was a bit too optimistic, I guess. Must understand more about the rest of shairport-sync. Fixes spurious segfault on program termination with CTRL-C that does not happen in gdb :) This reverts commit 8e46c727d59334b87ea0f639160e92349f5c81d1. --- audio_jack.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/audio_jack.c b/audio_jack.c index 8d176759..ec004861 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -33,6 +33,7 @@ static const int bytes_per_frame = 4; #define buffer_size (44100 * 4 * bytes_per_frame) static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; pthread_t *open_client_if_necessary_thread = NULL; @@ -174,6 +175,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(default_jack_error_callback); jack_set_info_function(default_jack_info_callback); + pthread_mutex_lock(&client_mutex); jack_status_t status; client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); if (!client) { @@ -194,15 +196,18 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a } else { debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); } + pthread_mutex_unlock(&client_mutex); return 0; } void jack_deinit() { + pthread_mutex_lock(&client_mutex); if (jack_deactivate(client)) debug(1, "Error deactivating jack client"); if (jack_client_close(client)) debug(1, "Error closing jack client"); + pthread_mutex_unlock(&client_mutex); jack_ringbuffer_free(jackbuf); } From f1ba07ff2bebb56f1258ee16994e69bfe6d72f4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 01:02:30 +0000 Subject: [PATCH 34/48] Make callback names simpler and more consistent. Always use &callback in setter functions to make the function pointer explicit, even though it's not syntactically necessary. --- audio_jack.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index ec004861..9ccc4be7 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -91,7 +91,7 @@ static void deinterleave_and_convert(const char *interleaved_frames, } } -static int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) void *arg) { +static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { jack_default_audio_sample_t *left_buffer = (jack_default_audio_sample_t *)jack_port_get_buffer(left_port, nframes); @@ -136,9 +136,9 @@ static int jack_stream_write_cb(jack_nframes_t nframes, __attribute__((unused)) // FIXME: set_graph_order_callback(), recompute latencies here! -static void default_jack_error_callback(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } +static void error(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } -static void default_jack_info_callback(const char *desc) { inform("jackd information: \"%s\"", desc); } +static void info(const char *desc) { inform("jackd information: \"%s\"", desc); } int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { config.audio_backend_latency_offset = 0; @@ -172,8 +172,8 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a die("Can't allocate %d bytes for the JACK ringbuffer.", buffer_size); jack_ringbuffer_mlock(jackbuf); // lock buffer into memory so that it never gets paged out - jack_set_error_function(default_jack_error_callback); - jack_set_info_function(default_jack_info_callback); + jack_set_error_function(&error); + jack_set_info_function(&info); pthread_mutex_lock(&client_mutex); jack_status_t status; @@ -186,7 +186,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", sample_rate); } - jack_set_process_callback(client, jack_stream_write_cb, 0); + jack_set_process_callback(client, &process, 0); left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); right_port = jack_port_register(client, "out_R", From bda8b99cdaa2190cac916194422c0f6badf0e1b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 01:53:49 +0000 Subject: [PATCH 35/48] Add graph reorder callback, this is where we now compute the output latencies. --- audio_jack.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 9ccc4be7..80c357bf 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -134,7 +134,15 @@ static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { return 0; } -// FIXME: set_graph_order_callback(), recompute latencies here! +static int graph(__attribute__((unused)) void * arg) { + jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); + jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); + debug(1, "JACK graph reorder callback called. Current latencies to terminal downstream port:\n" + "\tLmin = %d, Lmax = %d, Rmin = %d, Rmax = %d", + latest_left_latency_range.min, latest_left_latency_range.max, + latest_right_latency_range.min, latest_right_latency_range.max); + return 0; +} static void error(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } @@ -172,9 +180,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a die("Can't allocate %d bytes for the JACK ringbuffer.", buffer_size); jack_ringbuffer_mlock(jackbuf); // lock buffer into memory so that it never gets paged out - jack_set_error_function(&error); - jack_set_info_function(&info); - pthread_mutex_lock(&client_mutex); jack_status_t status; client = jack_client_open(config.jack_client_name, JackNoStartServer, &status); @@ -187,6 +192,10 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a sample_rate); } jack_set_process_callback(client, &process, 0); + jack_set_graph_order_callback(client, &graph, 0); + jack_set_error_function(&error); + jack_set_info_function(&info); + left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); right_port = jack_port_register(client, "out_R", @@ -225,7 +234,7 @@ void jack_flush() { } int jack_delay(long *the_delay) { - + jack_nframes_t base_latency; // semantics change: we now look at the last transfer into the lock-free ringbuffer, not // into the jack buffers directly (because locking those would violate real-time constraints). // on average, that should lead to just a constant additional latency. the old comment still applies: @@ -234,7 +243,6 @@ int jack_delay(long *the_delay) { // but then a transfer could occur and we would get the buffer occupancy after another transfer // had occurred // so we could "lose" a full transfer (e.g. 1024 frames @ 44,100 fps ~ 23.2 milliseconds) - pthread_mutex_lock(&buffer_mutex); int64_t time_now = get_absolute_time_in_fp(); // this is the time back to the last time data @@ -248,16 +256,10 @@ int jack_delay(long *the_delay) { pthread_mutex_unlock(&buffer_mutex); int64_t frames_processed_since_latest_latency_check = (delta * 44100) >> 32; - - // FIXME: this should only be done if there was an actual change, i.e. on the jack graph reorder - // callback, to update a static variable which can be checked here. For now, use a fixed arbitrary value: - jack_nframes_t base_latency = 0; - // debug(1,"delta: %" PRId64 " frames.",frames_processed_since_latest_latency_check); - - // jack_nframes_t base_latency = (latest_left_latency_range.min + latest_left_latency_range.max) / 2; - // if (base_latency == 0) - // base_latency = (latest_right_latency_range.min + latest_right_latency_range.max) / 2; + // use the average of the left and right maximum latencies. if max is really different from min, + // there is an anomaly in the graph that we don't have any hope of fixing anyways + base_latency = (latest_left_latency_range.max + latest_right_latency_range.max) / 2; *the_delay = base_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; // debug(1,"reporting a delay of %d frames",*the_delay); return 0; From 4751a4e1bcd555bb6eb28cd99f2cfbb226236aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:03:15 +0000 Subject: [PATCH 36/48] Move jack base latency computation out of delay() into graph() to save cycles. --- audio_jack.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 80c357bf..5eb8f285 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -137,16 +137,22 @@ static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { static int graph(__attribute__((unused)) void * arg) { jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); + jack_latency = (latest_left_latency_range.max + latest_right_latency_range.max) / 2; debug(1, "JACK graph reorder callback called. Current latencies to terminal downstream port:\n" - "\tLmin = %d, Lmax = %d, Rmin = %d, Rmax = %d", + "\tLmin = %d, Lmax = %d, Rmin = %d, Rmax = %d; jack base latency = %d", latest_left_latency_range.min, latest_left_latency_range.max, - latest_right_latency_range.min, latest_right_latency_range.max); + latest_right_latency_range.min, latest_right_latency_range.max, + jack_latency); return 0; } -static void error(const char *desc) { debug(2, "jackd error: \"%s\"", desc); } +static void error(const char *desc) { + debug(2, "JACK error: \"%s\"", desc); +} -static void info(const char *desc) { inform("jackd information: \"%s\"", desc); } +static void info(const char *desc) { + inform("JACK information: \"%s\"", desc); +} int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { config.audio_backend_latency_offset = 0; @@ -234,7 +240,6 @@ void jack_flush() { } int jack_delay(long *the_delay) { - jack_nframes_t base_latency; // semantics change: we now look at the last transfer into the lock-free ringbuffer, not // into the jack buffers directly (because locking those would violate real-time constraints). // on average, that should lead to just a constant additional latency. the old comment still applies: @@ -259,8 +264,7 @@ int jack_delay(long *the_delay) { // debug(1,"delta: %" PRId64 " frames.",frames_processed_since_latest_latency_check); // use the average of the left and right maximum latencies. if max is really different from min, // there is an anomaly in the graph that we don't have any hope of fixing anyways - base_latency = (latest_left_latency_range.max + latest_right_latency_range.max) / 2; - *the_delay = base_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; + *the_delay = jack_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; // debug(1,"reporting a delay of %d frames",*the_delay); return 0; } From 95246dcd84698216692aab31a4df99d27acb9479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:05:05 +0000 Subject: [PATCH 37/48] Use more conservative JACK client name (a simple token without whitespace). --- audio_jack.c | 2 +- scripts/shairport-sync.conf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 5eb8f285..1e848678 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -179,7 +179,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a } if (config.jack_client_name == NULL) - config.jack_client_name = strdup("Shairport Sync"); + config.jack_client_name = strdup("shairport-sync"); jackbuf = jack_ringbuffer_create(buffer_size); if (jackbuf == NULL) diff --git a/scripts/shairport-sync.conf b/scripts/shairport-sync.conf index c06c32e7..dded9294 100644 --- a/scripts/shairport-sync.conf +++ b/scripts/shairport-sync.conf @@ -108,7 +108,7 @@ pa = // Parameters for the "jack" JACK Audio Connection Kit backend. jack = { -// client_name = "Shairport Sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. +// client_name = "shairport-sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. }; // Parameters for the "pipe" audio back end, a back end that directs raw CD-style audio output to a pipe. No interpolation is done. From c00199d664a1042af78ede253f45799e8bca3aa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:15:35 +0000 Subject: [PATCH 38/48] Always use NULL instead of 0 for pointers for clarity. --- audio_jack.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 1e848678..736992f1 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -92,12 +92,10 @@ static void deinterleave_and_convert(const char *interleaved_frames, } static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { - jack_default_audio_sample_t *left_buffer = (jack_default_audio_sample_t *)jack_port_get_buffer(left_port, nframes); jack_default_audio_sample_t *right_buffer = (jack_default_audio_sample_t *)jack_port_get_buffer(right_port, nframes); - jack_ringbuffer_data_t v[2] = { 0 }; jack_nframes_t i, thisbuf; int frames_written = 0; @@ -177,7 +175,6 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a config.jack_client_name = (char *)str; } } - if (config.jack_client_name == NULL) config.jack_client_name = strdup("shairport-sync"); @@ -197,15 +194,15 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a die("The JACK server is running at the wrong sample rate (%d) for Shairport Sync. Must be 44100 Hz.", sample_rate); } - jack_set_process_callback(client, &process, 0); - jack_set_graph_order_callback(client, &graph, 0); + jack_set_process_callback(client, &process, NULL); + jack_set_graph_order_callback(client, &graph, NULL); jack_set_error_function(&error); jack_set_info_function(&info); left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, 0); - right_port = jack_port_register(client, "out_R", - JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); + JackPortIsOutput, NULL); + right_port = jack_port_register(client, "out_R", JACK_DEFAULT_AUDIO_TYPE, + JackPortIsOutput, NULL); if (jack_activate(client)) { die("Could not activate %s JACK client.", config.jack_client_name); } else { From 90f24c62dfe5ecf56450419663bb8dd90849a614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:20:21 +0000 Subject: [PATCH 39/48] Add myself to (C)omplain-to tag :-D --- audio_jack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/audio_jack.c b/audio_jack.c index 736992f1..30403f62 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -1,6 +1,7 @@ /* * jack output driver. This file is part of Shairport Sync. - * Copyright (c) 2018 Mike Brady + * Copyright (c) 2019 Mike Brady , + * Jörn Nettingsmeier * * All rights reserved. * From 3a4deb38a2f82fc0890723201850738318f0268b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:26:36 +0000 Subject: [PATCH 40/48] Declare all file-global variables as static, remove unused thread pointer. --- audio_jack.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 30403f62..7095a31b 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -36,8 +36,6 @@ static const int bytes_per_frame = 4; static pthread_mutex_t buffer_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t client_mutex = PTHREAD_MUTEX_INITIALIZER; -pthread_t *open_client_if_necessary_thread = NULL; - int jack_init(int, char **); void jack_deinit(void); void jack_start(int, int); @@ -60,18 +58,18 @@ audio_output audio_jack = {.name = "jack", .parameters = NULL, .mute = NULL}; -jack_port_t *left_port; -jack_port_t *right_port; +static jack_port_t *left_port; +static jack_port_t *right_port; -jack_client_t *client; -jack_nframes_t sample_rate; -jack_nframes_t jack_latency; +static jack_client_t *client; +static jack_nframes_t sample_rate; +static jack_nframes_t jack_latency; static jack_ringbuffer_t *jackbuf; static int flush_please = 0; -jack_latency_range_t latest_left_latency_range, latest_right_latency_range; -int64_t time_of_latest_transfer; +static jack_latency_range_t latest_left_latency_range, latest_right_latency_range; +static int64_t time_of_latest_transfer; static inline jack_default_audio_sample_t sample_conv(short sample) { From 5c1769f87c6bef17607731a1ba0532ac751ddc92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 02:28:59 +0000 Subject: [PATCH 41/48] Partly revert c00199d664a1042af78ede253f45799e8bca3aa0, some 0 are actually longs. It's late :( --- audio_jack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 7095a31b..2e89ec95 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -199,9 +199,9 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_info_function(&info); left_port = jack_port_register(client, "out_L", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, NULL); + JackPortIsOutput, 0); right_port = jack_port_register(client, "out_R", JACK_DEFAULT_AUDIO_TYPE, - JackPortIsOutput, NULL); + JackPortIsOutput, 0); if (jack_activate(client)) { die("Could not activate %s JACK client.", config.jack_client_name); } else { From 28ea6972af185ef44674a55512f943a32e57fb04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 15:33:29 +0000 Subject: [PATCH 42/48] Add dummy option jack_autoconnect_option. --- audio_jack.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/audio_jack.c b/audio_jack.c index 2e89ec95..cd90c796 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -173,6 +173,10 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config_lookup_string(config.cfg, "jack.client_name", &str)) { config.jack_client_name = (char *)str; } + /* Get the autoconnect pattern. */ + if (config_lookup_string(config.cfg, "jack.autoconnect_pattern", &str)) { + config.jack_autoconnect_pattern = (char *)str; + } } if (config.jack_client_name == NULL) config.jack_client_name = strdup("shairport-sync"); @@ -207,6 +211,17 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a } else { debug(2, "JACK client %s activated sucessfully.", config.jack_client_name); } + + if (config.jack_autoconnect_pattern != NULL) { + debug(1, "config.jack_autoconnect_pattern is %s.", config.jack_autoconnect_pattern); + const char** port_list = jack_get_ports(client, config.jack_autoconnect_pattern, JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput); + while (*port_list != NULL) { + debug(1, "Found matching JACK port %s.", *port_list); + port_list++; + // FIXME: implement actual connection, warn user if more than 2 hits. + } + } + pthread_mutex_unlock(&client_mutex); return 0; From bd5cfd3583fe75d17f5b44a4af8efff15f6fd54e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 16:08:40 +0000 Subject: [PATCH 43/48] forgot to add new option autoconnect_pattern to default conf and common.h --- common.h | 1 + scripts/shairport-sync.conf | 1 + 2 files changed, 2 insertions(+) diff --git a/common.h b/common.h index 9a824cbf..b06fd52d 100644 --- a/common.h +++ b/common.h @@ -233,6 +233,7 @@ typedef struct { // debugging. Currently audio packets only... #ifdef CONFIG_JACK char *jack_client_name; + char *jack_autoconnect_pattern; #endif } shairport_cfg; diff --git a/scripts/shairport-sync.conf b/scripts/shairport-sync.conf index dded9294..addbd1ee 100644 --- a/scripts/shairport-sync.conf +++ b/scripts/shairport-sync.conf @@ -109,6 +109,7 @@ pa = jack = { // client_name = "shairport-sync"; //Set this to the name of the client that should appear in "Connections" when Shairport Sync is active. +// autoconnect_pattern = ""; //Set this to a pattern that describes the ports you would like to connect to automatically when we start up, like "system:playback_?" }; // Parameters for the "pipe" audio back end, a back end that directs raw CD-style audio output to a pipe. No interpolation is done. From 4f39e0d83073513e4bbfd85648fda932d056c717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 16:09:29 +0000 Subject: [PATCH 44/48] Move away from hardcoded channels. Centralizes information, simplifies the code, and might be helpful for future surround support. --- audio_jack.c | 62 ++++++++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index cd90c796..6f4e3416 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -58,8 +58,10 @@ audio_output audio_jack = {.name = "jack", .parameters = NULL, .mute = NULL}; -static jack_port_t *left_port; -static jack_port_t *right_port; +// This also affects deinterlacing, so make it exactly the number of incoming audio channels! +#define NPORTS 2 +static jack_port_t *port[NPORTS]; +static const char* port_name[NPORTS] = { "out_L", "out_R" }; static jack_client_t *client; static jack_nframes_t sample_rate; @@ -68,7 +70,7 @@ static jack_nframes_t jack_latency; static jack_ringbuffer_t *jackbuf; static int flush_please = 0; -static jack_latency_range_t latest_left_latency_range, latest_right_latency_range; +static jack_latency_range_t latest_latency_range[NPORTS]; static int64_t time_of_latest_transfer; @@ -77,24 +79,23 @@ static inline jack_default_audio_sample_t sample_conv(short sample) { } static void deinterleave_and_convert(const char *interleaved_frames, - jack_default_audio_sample_t * const jack_buffer_left, - jack_default_audio_sample_t * const jack_buffer_right, + jack_default_audio_sample_t* jack_buffer[], + jack_nframes_t offset, jack_nframes_t nframes) { - jack_nframes_t i; + jack_nframes_t f; short *ifp = (short *)interleaved_frames; // we're dealing with 16bit audio here - jack_default_audio_sample_t *fpl = jack_buffer_left; - jack_default_audio_sample_t *fpr = jack_buffer_right; - for (i=0; i 0) { - left_buffer[frames_written] = 0.0; - right_buffer[frames_written] = 0.0; + for (int i=0; i < NPORTS; i++) { + buffer[i][frames_written] = 0.0; + } frames_written++; nframes--; } @@ -132,14 +134,16 @@ static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { } static int graph(__attribute__((unused)) void * arg) { - jack_port_get_latency_range(left_port, JackPlaybackLatency, &latest_left_latency_range); - jack_port_get_latency_range(right_port, JackPlaybackLatency, &latest_right_latency_range); - jack_latency = (latest_left_latency_range.max + latest_right_latency_range.max) / 2; - debug(1, "JACK graph reorder callback called. Current latencies to terminal downstream port:\n" - "\tLmin = %d, Lmax = %d, Rmin = %d, Rmax = %d; jack base latency = %d", - latest_left_latency_range.min, latest_left_latency_range.max, - latest_right_latency_range.min, latest_right_latency_range.max, - jack_latency); + int latency = 0; + debug(1, "JACK graph reorder callback called. Current latencies to terminal downstream port:"); + for (int i=0; i Date: Sat, 16 Feb 2019 18:45:51 +0000 Subject: [PATCH 45/48] Actually implement auto-connection, and document it. --- audio_jack.c | 42 ++++++++++++++++++++++++++++++------- scripts/shairport-sync.conf | 10 +++++++-- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 6f4e3416..d98be546 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -20,6 +20,7 @@ #include "audio.h" #include "common.h" +#include #include #include #include @@ -156,6 +157,7 @@ static void info(const char *desc) { } int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { + int i; config.audio_backend_latency_offset = 0; config.audio_backend_buffer_desired_length = 0.500; config.audio_backend_buffer_interpolation_threshold_in_seconds = @@ -206,7 +208,7 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a jack_set_error_function(&error); jack_set_info_function(&info); - for (int i=0; i < NPORTS; i++) { + for (i=0; i < NPORTS; i++) { port[i] = jack_port_register(client, port_name[i], JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0); } @@ -218,14 +220,40 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a if (config.jack_autoconnect_pattern != NULL) { debug(1, "config.jack_autoconnect_pattern is %s.", config.jack_autoconnect_pattern); - const char** port_list = jack_get_ports(client, config.jack_autoconnect_pattern, JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput); - while (*port_list != NULL) { - debug(1, "Found matching JACK port %s.", *port_list); - port_list++; - // FIXME: implement actual connection, warn user if more than 2 hits. + const char** port_list = jack_get_ports(client, config.jack_autoconnect_pattern, + JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput); + for (i=0; i Date: Sat, 16 Feb 2019 19:01:37 +0000 Subject: [PATCH 46/48] Use appropriate warn/inform/debug levels. --- audio_jack.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index d98be546..f34e8899 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -136,20 +136,20 @@ static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { static int graph(__attribute__((unused)) void * arg) { int latency = 0; - debug(1, "JACK graph reorder callback called. Current latencies to terminal downstream port:"); + debug(2, "JACK graph reorder callback called."); for (int i=0; i Date: Sat, 16 Feb 2019 19:57:28 +0000 Subject: [PATCH 47/48] NOP. Reformat code consistently, improve comments. --- audio_jack.c | 143 ++++++++++++++++++++++++++++----------------------- 1 file changed, 79 insertions(+), 64 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index f34e8899..28172833 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -59,7 +59,8 @@ audio_output audio_jack = {.name = "jack", .parameters = NULL, .mute = NULL}; -// This also affects deinterlacing, so make it exactly the number of incoming audio channels! +// This also affects deinterlacing. +// So make it exactly the number of incoming audio channels! #define NPORTS 2 static jack_port_t *port[NPORTS]; static const char* port_name[NPORTS] = { "out_L", "out_R" }; @@ -76,40 +77,55 @@ static int64_t time_of_latest_transfer; static inline jack_default_audio_sample_t sample_conv(short sample) { + // It sounds correct, but I don't understand it. + // Zero int needs to be zero float. Check. + // Plus 32767 int is 1.0. Check. + // Minus 32767 int is -0.99997. And here my brain shuts down. + // In my head, it should be 1.0, and we should tolerate an overflow + // at minus 32768. But I'm sure there's a textbook explanation somewhere. return ((sample < 0) ? (-1.0 * sample / SHRT_MIN) : (1.0 * sample / SHRT_MAX)); } -static void deinterleave_and_convert(const char *interleaved_frames, - jack_default_audio_sample_t* jack_buffer[], +static void deinterleave_and_convert(const char *interleaved_input_buffer, + jack_default_audio_sample_t* jack_output_buffer[], jack_nframes_t offset, jack_nframes_t nframes) { jack_nframes_t f; - short *ifp = (short *)interleaved_frames; // we're dealing with 16bit audio here - for (f=offset; f<(nframes + offset); f++) { - for (int i=0; i nframes) { frames_required = nframes; @@ -122,24 +138,28 @@ static int process(jack_nframes_t nframes, __attribute__((unused)) void *arg) { } jack_ringbuffer_read_advance(jackbuf, frames_written * bytes_per_frame); } - // now, if there are any more frames to put into the buffer, fill them with - // silence + // If there are any more frames to put into the buffer, fill them with + // silence. This is a critical underflow situation. Let's at least keep the JACK + // graph humming along while preventing the motorboat sound of a repeating buffer. while (nframes > 0) { - for (int i=0; i < NPORTS; i++) { + for (i = 0; i < NPORTS; i++) { buffer[i][frames_written] = 0.0; } frames_written++; nframes--; } - return 0; + return 0; // Tell JACK that all is well. } +// This is the JACK graph reorder callback. Now we know some JACK connections +// have changed, so we recompute the latency. static int graph(__attribute__((unused)) void * arg) { int latency = 0; debug(2, "JACK graph reorder callback called."); for (int i=0; i> 32; // debug(1,"delta: %" PRId64 " frames.",frames_processed_since_latest_latency_check); - // use the average of the left and right maximum latencies. if max is really different from min, - // there is an anomaly in the graph that we don't have any hope of fixing anyways + // jack_latency is set by the graph() callback, it's the average of the maximum + // latencies of all our output ports. Adjust this constant baseline delay according + // to the buffer fill level: *the_delay = jack_latency + audio_occupancy_now - frames_processed_since_latest_latency_check; // debug(1,"reporting a delay of %d frames",*the_delay); return 0; @@ -320,13 +334,14 @@ int play(void *buf, int samples) { // copy the samples into the queue size_t bytes_to_transfer, bytes_transferred; bytes_to_transfer = samples * bytes_per_frame; - pthread_mutex_lock(&buffer_mutex); // it's ok to lock here since we're not in the realtime callback + // It's ok to lock here since we're not in the realtime callback: + pthread_mutex_lock(&buffer_mutex); bytes_transferred = jack_ringbuffer_write(jackbuf, buf, bytes_to_transfer); - // semantics change: we now measure the last time audio was moved into the ringbuffer, not the jack output buffers. time_of_latest_transfer = get_absolute_time_in_fp(); pthread_mutex_unlock(&buffer_mutex); if (bytes_transferred < bytes_to_transfer) { - warn("JACK ringbuffer overrun. Only wrote %d of %d bytes.", bytes_transferred, bytes_to_transfer); + warn("JACK ringbuffer overrun. Only wrote %d of %d bytes.", + bytes_transferred, bytes_to_transfer); } return 0; } From e59da78348903a4c31f1064e56beac214fcc10de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Nettingsmeier?= Date: Sat, 16 Feb 2019 21:26:32 +0000 Subject: [PATCH 48/48] Fix segfault due to missing null check... --- audio_jack.c | 58 +++++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/audio_jack.c b/audio_jack.c index 28172833..7b00d1d2 100644 --- a/audio_jack.c +++ b/audio_jack.c @@ -242,37 +242,39 @@ int jack_init(__attribute__((unused)) int argc, __attribute__((unused)) char **a // I've reported it to the jack-devel mailing list, they're in a better place to fix it. const char** port_list = jack_get_ports(client, config.jack_autoconnect_pattern, JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput); - for (i = 0; i < NPORTS ; i++) { - char* full_port_name[NPORTS]; - full_port_name[i] = malloc(sizeof(char) * jack_port_name_size()); - sprintf(full_port_name[i], "%s:%s", config.jack_client_name, port_name[i]); - if (port_list[i] != NULL) { - int err; - debug(2, "Connecting %s to %s.", full_port_name[i], port_list[i]); - err = jack_connect(client, full_port_name[i], port_list[i]); - switch (err) { - case EEXIST: - inform("The requested connection from %s to %s already exists.", - full_port_name[i], port_list[i]); - break; - case 0: - // success - break; - default: - warn("JACK error no. %d occured while trying to connect %s to %s.", - err, full_port_name[i], port_list[i]); - break; + if (port_list != NULL) { + for (i = 0; i < NPORTS ; i++) { + char* full_port_name[NPORTS]; + full_port_name[i] = malloc(sizeof(char) * jack_port_name_size()); + sprintf(full_port_name[i], "%s:%s", config.jack_client_name, port_name[i]); + if (port_list[i] != NULL) { + int err; + debug(2, "Connecting %s to %s.", full_port_name[i], port_list[i]); + err = jack_connect(client, full_port_name[i], port_list[i]); + switch (err) { + case EEXIST: + inform("The requested connection from %s to %s already exists.", + full_port_name[i], port_list[i]); + break; + case 0: + // success + break; + default: + warn("JACK error no. %d occured while trying to connect %s to %s.", + err, full_port_name[i], port_list[i]); + break; + } + } else { + inform("No matching port found in %s to connect %s to. You may not hear audio.", + config.jack_autoconnect_pattern, full_port_name[i]); } - } else { - inform("No matching port found in %s to connect %s to. You may not hear audio.", - config.jack_autoconnect_pattern, full_port_name[i]); + free(full_port_name[i]); } - free(full_port_name[i]); + while (port_list[i++] != NULL) { + inform("Additional matching port %s found. Check that the connections are what you intended."); + } + jack_free(port_list); } - while (port_list[i++] != NULL) { - inform("Additional matching port %s found. Check that the connections are what you intended."); - } - jack_free(port_list); } pthread_mutex_unlock(&client_mutex);