[pulseaudio-discuss] PATCH 2/3: RAOP: Init dummy port and card for latency change

[Tanu Kaskinen]

On Sat, 2017-10-07 at 20:20 +0200, Colin Leroy wrote:
> From bd815a400ad280627083cbce3ef9b927f669faa0 Mon Sep 17 00:00:00 2001
> From: Colin Leroy <colin at colino.net>
> Date: Sat, 7 Oct 2017 20:10:21 +0200
> Subject: [PATCH 2/3] RAOP: Init dummy port and card for  latency change
> 
> This adds a port, card and profile to RAOP sinks to make it
> possible to change the latency at runtime (and have it persist)
> using pavucontrol or pactl set-port-latency-offset.

Since this is only a workaround for the lack of "pactl set-sink-
latency-offset", it's questionable whether I should accept this, but I
don't think this does much harm either, so I won't block this patch
because of that.

> ---
>  src/modules/raop/raop-sink.c | 110 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 107 insertions(+), 3 deletions(-)
> 
> diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
> index cd90e3d9..244f6951 100644
> --- a/src/modules/raop/raop-sink.c
> +++ b/src/modules/raop/raop-sink.c
> @@ -69,6 +69,7 @@ struct userdata {
>      pa_core *core;
>      pa_module *module;
>      pa_sink *sink;
> +    pa_card *card;
>  
>      pa_thread *thread;
>      pa_thread_mq thread_mq;
> @@ -122,9 +123,6 @@ 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->latency * PA_USEC_PER_MSEC;
> -
>      return latency;
>  }
>  
> @@ -465,6 +463,83 @@ finish:
>      pa_log_debug("Thread shutting down");
>  }
>  
> +static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
> +    return 0;
> +}
> +
> +static pa_device_port *raop_create_port(struct userdata *u, const char *server) {
> +    pa_device_port_new_data data;
> +    pa_device_port *port;
> +
> +    pa_device_port_new_data_init(&data);
> +
> +    pa_device_port_new_data_set_name(&data, "network-output");
> +    pa_device_port_new_data_set_description(&data, server);
> +    pa_device_port_new_data_set_direction(&data, PA_DIRECTION_OUTPUT);
> +
> +    port = pa_device_port_new(u->core, &data, 0);
> +
> +    pa_device_port_new_data_done(&data);
> +
> +    if (port == NULL)
> +        return NULL;
> +
> +    pa_device_port_ref(port);
> +    /* init the latency_offset directly instead of using
> +     * pa_device_port_set_latency_offset(), so that mode module-card-restore
> +     * can do its job when PulseAudio restarts and the user-set latency
> +     * is not overwritten.
> +     */
> +    port->latency_offset = u->latency * PA_USEC_PER_MSEC;

I don't think we should use the latency offset for this. The sink
implementation is expected to report as good latency as it can, and
only if that's not good enough, then the user can set a latency offset.
Normally the latency offset should be zero.

> +
> +    return port;
> +}
> +
> +static pa_card_profile *raop_create_profile() {
> +    pa_card_profile *profile;
> +
> +    profile = pa_card_profile_new("RAOP", _("RAOP standard profile"), 0);
> +    profile->priority = 10;
> +    profile->n_sinks = 1;
> +    profile->n_sources = 0;
> +    profile->max_sink_channels = 2;
> +    profile->max_source_channels = 0;
> +
> +    return profile;
> +}
> +
> +static pa_card *raop_create_card(pa_module *m, pa_device_port *port, pa_card_profile *profile, const char *server, const char *nicename) {
> +    pa_card_new_data data;
> +    pa_card *card;
> +    char *card_name;
> +
> +    pa_card_new_data_init(&data);
> +
> +    card_name = pa_sprintf_malloc("raop_client.%s", server);
> +
> +    pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, card_name);

To me just the server address would make more sense.
PA_PROP_DEVICE_STRING is usually something that is not PulseAudio
specific. (Nobody's going to have problems if the property is set to
the card name, though.)

> +    pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, nicename);
> +    data.driver = __FILE__;
> +
> +    pa_card_new_data_set_name(&data, card_name);
> +    pa_xfree(card_name);
> +
> +    pa_hashmap_put(data.ports, port->name, port);
> +    pa_hashmap_put(data.profiles, profile->name, profile);
> +
> +    card = pa_card_new(m->core, &data);
> +
> +    pa_card_new_data_done(&data);
> +
> +    if (card == NULL)
> +        return NULL;
> +
> +    card->active_profile = profile;

I think pa_card_choose_initial_profile() should be called instead of
setting the active profile directly (although it doesn't make much of a
difference as long as there's only one profile).

-- 
Tanu

https://www.patreon.com/tanuk