> From e9ac9ae71f9ef134f69757bbe481983374c08b04 Mon Sep 17 00:00:00 2001
> From: Colin Leroy <colin at colino.net>
> Date: Sat, 22 Jul 2017 14:50:40 +0200
> Subject: [PATCH] RAOP: Announce real latency
>
> Use predefined values depending on the server, and make it configurable.
> AirPlay is supposed to have 2s of latency. With my hardware, this is
> more 2.352 seconds after numerous tests.
> ---
> src/modules/raop/module-raop-discover.c | 42 +++++++++++++++++++++++++++++++++
> src/modules/raop/module-raop-sink.c | 4 +++-
> src/modules/raop/raop-sink.c | 12 ++++++++++
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
> index c1becb17..7ec72dfd 100644
> --- a/src/modules/raop/module-raop-discover.c
> +++ b/src/modules/raop/module-raop-discover.c
> @@ -49,6 +49,8 @@ PA_MODULE_AUTHOR("Colin Guthrie");
> PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
> PA_MODULE_VERSION(PACKAGE_VERSION);
> PA_MODULE_LOAD_ONCE(true);
> +PA_MODULE_USAGE(
> + "force_latency=<audio latency in usec - applies to all devices> ");
We have two modules (module-loopback and module-rtp-recv) that have a
module argument for configuring the latency. Those module arguments
have a "_msec" suffix, which is good, because it makes it more obvious
what units are expected. Having a "_usec" suffix would be good here
too.
Working with milliseconds rather than microseconds would be nicer for
users, though, so I suggest changing the module argument unit.
I'd also like just plain "latency_msec" as the argument name, without
the "force_" prefix.
>
> #define SERVICE_TYPE_SINK "_raop._tcp"
>
> @@ -61,9 +63,11 @@ struct userdata {
> AvahiServiceBrowser *sink_browser;
>
> pa_hashmap *tunnels;
> + uint32_t force_latency;
Nitpicking: if the unit is microseconds, pa_usec_t would be a more
appropriate type.
> };
>
> static const char* const valid_modargs[] = {
> + "force_latency",
> NULL
> };
>
> @@ -127,6 +131,25 @@ static void tunnel_free(struct tunnel *t) {
> pa_xfree(t);
> }
>
> +/* This functions returns RAOP audio latency as guessed by the
> + * device model header.
> + * Feel free to complete the possible values after testing with
> + * your hardware.
> + */
> +static uint32_t guess_latency_from_device(const char *model) {
> + uint32_t default_latency = 2000000;
> +
> + if (pa_streq(model, "AppleTV2,1")) {
> + default_latency = 2000000;
If I understood correctly, you haven't tested this device. I think it's
best to override the default latency only for tested devices. Here the
value doesn't change anyway, but having the special case suggests that
somebody has actually tested this hardware and found out that 2000000
is the best value, which may discourage fixing the value later.
> + } else if (pa_streq(model, "PIONEER,1")) {
> + /* Pioneer N-30 */
> + default_latency = 2352000;
> + }
> +
> + pa_log_debug("Default latency is %u for device model %s.", default_latency, model);
The log message should say in which unit the latency value is.
> + return default_latency;
> +}
> +
> static void resolver_cb(
> AvahiServiceResolver *r,
> AvahiIfIndex interface, AvahiProtocol protocol,
> @@ -145,6 +168,7 @@ static void resolver_cb(
> char at[AVAHI_ADDRESS_STR_MAX];
> AvahiStringList *l;
> pa_module *m;
> + uint32_t latency = 0;
>
> pa_assert(u);
>
> @@ -226,6 +250,9 @@ static void resolver_cb(
> /* Sample rate */
> pa_xfree(sr);
> sr = pa_xstrdup(value);
> + } else if (pa_streq(key, "am")) {
> + /* Device model */
> + latency = guess_latency_from_device(value);
Now there are two default latencies: 0 for the case when the device
model is known and 2000000 when the device model isn't known. The
default latency should be defined in only one place.
Since module-raop-sink.c also needs to have a default value for the
latency, I think it would be best to put the default latency constant
to some header.
> }
>
> avahi_free(key);
> @@ -308,6 +335,16 @@ static void resolver_cb(
> pa_xfree(t);
> }
>
> + if (u->force_latency > 0) {
> + t = args;
> + args = pa_sprintf_malloc("%s latency=%u", args, u->force_latency);
> + pa_xfree(t);
> + } else if (latency > 0) {
> + t = args;
> + args = pa_sprintf_malloc("%s latency=%u", args, latency);
> + pa_xfree(t);
> + }
Treating 0 as a special "not set" value is not ideal, because it makes
it impossible to configure the latency to 0. Either add a "latency_set"
boolean flag, or use negative values to indicate that the variable is
not set.
Also, this would be simpler:
if (u->force_latency_set)
latency = u->force_latency;
t = args;
args = pa_sprintf_malloc("%s latency=%u", args, latency);
pa_xfree(t);
> +
> pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
>
> if (pa_module_load(&m, u->core, "module-raop-sink", args) == PA_OK) {
> @@ -426,6 +463,7 @@ int pa__init(pa_module *m) {
> struct userdata *u;
> pa_modargs *ma = NULL;
> int error;
> + uint32_t l;
>
> if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
> pa_log("Failed to parse module arguments.");
> @@ -437,6 +475,10 @@ int pa__init(pa_module *m) {
> u->module = m;
> u->sink_browser = NULL;
>
> + if (pa_modargs_get_value_u32(ma, "force_latency", &l) == 0) {
> + u->force_latency = l;
> + }
u->force_latency remains uninitialized if the "force_latency" argument
is not given. I recommend fixing this by using pa_xnew0() instead of
pa_xnew() when allocating the userdata struct. (If you choose to use
negative values for indicating that the variable is not set, though,
then pa_xnew0() won't help with this particular problem.)
> +
> u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
>
> u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
> diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
> index 82fa48d9..bc1d0d67 100644
> --- a/src/modules/raop/module-raop-sink.c
> +++ b/src/modules/raop/module-raop-sink.c
> @@ -46,7 +46,8 @@ PA_MODULE_USAGE(
> "rate=<sample rate> "
> "channels=<number of channels> "
> "username=<authentication user name, default: \"iTunes\"> "
> - "password=<authentication password>");
> + "password=<authentication password> "
> + "latency=<audio latency in usec>");
I'd prefer "latency_msec".
>
> static const char* const valid_modargs[] = {
> "name",
> @@ -61,6 +62,7 @@ static const char* const valid_modargs[] = {
> "channels",
> "username",
> "password",
> + "latency",
> NULL
> };
>
> diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
> index e5d219e8..61bb93c2 100644
> --- a/src/modules/raop/raop-sink.c
> +++ b/src/modules/raop/raop-sink.c
> @@ -87,6 +87,8 @@ struct userdata {
> pa_usec_t start;
> pa_smoother *smoother;
> uint64_t write_count;
> +
> + uint32_t audio_latency;
> };
>
> enum {
> @@ -119,6 +121,9 @@ static int64_t sink_get_latency(const struct userdata *u) {
>
> latency = pa_bytes_to_usec(u->write_count, &u->sink->sample_spec) - (int64_t) now;
>
> + /* RAOP default latency */
> + latency += u->audio_latency;
> +
> return latency;
> }
>
> @@ -163,6 +168,7 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse
> pa_log_debug("RAOP: RUNNING");
>
> now = pa_rtclock_now();
> + pa_smoother_reset(u->smoother, now, true);
> pa_smoother_resume(u->smoother, now, true);
The pa_smoother_pause() and pa_smoother_resume() calls can be removed.
>
> if (!pa_raop_client_is_alive(u->raop)) {
> @@ -461,6 +467,7 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
> const char /* *username, */ *password;
> pa_sink_new_data data;
> const char *name = NULL;
> + uint32_t l;
>
> pa_assert(m);
> pa_assert(ma);
> @@ -488,6 +495,11 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
> u->rtpoll = pa_rtpoll_new();
> u->rtpoll_item = NULL;
>
> + if (pa_modargs_get_value_u32(ma, "latency", &l) == 0) {
> + u->audio_latency = l;
> + }
u->audio_latency should be initialized to 2000000 if the "latency"
argument is not given.
--
Tanu
https://www.patreon.com/tanuk
