[pulseaudio-discuss] [PATCH 3/5] pacmd: Add functions to handle the latency offset

[Tanu Kaskinen]

On Fri, 2012-06-22 at 20:55 +0200, poljar (Damir Jelic) wrote:
> From: poljar <poljarinho at gmail.com>
> 
> pacmd was extended so it can handle the new latency offset.
> 
> A new function was added so we can set the latency also the list
> commands were extended to print the latency offset on the ports.

Thanks, I've done a few changes (see below), and I'll push this soon.

> @@ -168,6 +169,7 @@ static const struct command commands[] = {
>      { "set-card-profile",        pa_cli_command_card_profile,       "Change the profile of a card (args: index|name, profile-name)", 3},
>      { "set-sink-port",           pa_cli_command_sink_port,          "Change the port of a sink (args: index|name, port-name)", 3},
>      { "set-source-port",         pa_cli_command_source_port,        "Change the port of a source (args: index|name, port-name)", 3},
> +    { "set-port-latency-offset", pa_cli_command_port_offset, "Change the latency of a port (args: card-index|card-name, port-name, latency-offset)", 4},

The description string is misaligned.

>      { "suspend-sink",            pa_cli_command_suspend_sink,       "Suspend sink (args: index|name, bool)", 3},
>      { "suspend-source",          pa_cli_command_suspend_source,     "Suspend source (args: index|name, bool)", 3},
>      { "suspend",                 pa_cli_command_suspend,            "Suspend all sinks and all sources (args: bool)", 2},
> @@ -1723,6 +1725,52 @@ static int pa_cli_command_source_port(pa_core *c, pa_tokenizer *t, pa_strbuf *bu
>      return 0;
>  }
>  
> +static int pa_cli_command_port_offset(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, pa_bool_t *fail) {
> +    const char *n, *p, *l;
> +    pa_device_port *port;
> +    pa_card *card;
> +    double offset;
> +
> +    pa_core_assert_ref(c);
> +    pa_assert(t);
> +    pa_assert(buf);
> +    pa_assert(fail);
> +
> +    if (!(n = pa_tokenizer_get(t, 1))) {
> +        pa_strbuf_puts(buf, "You need to specify a card either by its name or its index.\n");
> +        return -1;
> +    }
> +
> +    if (!(p = pa_tokenizer_get(t, 2))) {
> +        pa_strbuf_puts(buf, "You need to specify a port by its name.\n");
> +        return -1;
> +    }
> +
> +    if (!(l = pa_tokenizer_get(t, 3))) {
> +        pa_strbuf_puts(buf, "You need to specify a latency offset.\n");
> +        return -1;
> +    }
> +
> +    if (pa_atod(l, &offset) < 0) {
> +        pa_strbuf_puts(buf, "Failed to parse latency.\n");
> +        return -1;
> +    }

I guess you use pa_atod() because there's no pa_atoi64(), and therefore
the full range of pa_usec_t is not available if you just use pa_atoi().
But the range is still roughly from -2000 seconds to 2000 seconds, which
should be enough for everybody. In my opinion it's better to accept a
reduced range than to accept floating point numbers. If you accept
floating point numbers, you should check also that the offset is between
INT64_MIN and INT64_MAX.

In conclusion: I have changed the offset parsing to use pa_atoi().

> +
> +    if (!(card = pa_namereg_get(c, n, PA_NAMEREG_CARD))) {
> +        pa_strbuf_puts(buf, "No card found by this name or index.\n");
> +        return -1;
> +    }
> +
> +    if (!(port = pa_hashmap_get(card->ports, p))) {
> +        pa_strbuf_puts(buf, "No port found by this name.\n");
> +        return -1;
> +    }
> +
> +    pa_device_port_set_latency_offset(port, (pa_usec_t) offset);
> +
> +    return 0;
> +}
> +
>  static int pa_cli_command_dump(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, pa_bool_t *fail) {
>      pa_module *m;
>      pa_sink *sink;
> diff --git a/src/pulsecore/cli-text.c b/src/pulsecore/cli-text.c
> index d234b96..d4c6660 100644
> --- a/src/pulsecore/cli-text.c
> +++ b/src/pulsecore/cli-text.c
> @@ -126,8 +126,9 @@ static void append_port_list(pa_strbuf *s, pa_hashmap *ports)
>      pa_strbuf_puts(s, "\tports:\n");
>      PA_HASHMAP_FOREACH(p, ports, state) {
>          char *t = pa_proplist_to_string_sep(p->proplist, "\n\t\t\t\t");
> -        pa_strbuf_printf(s, "\t\t%s: %s (priority %u, available: %s)\n",
> -            p->name, p->description, p->priority, port_available_to_string(p->available));
> +        pa_strbuf_printf(s, "\t\t%s: %s (priority %u, offset %.2f usec, available: %s)\n",
> +            p->name, p->description, p->priority, (double) p->latency_offset,
> +            port_available_to_string(p->available));

Why cast the offset to floating point? I'd understand it if you'd change
the units to milliseconds or something, but if you don't change the
units, what's the point?

Also, I think it's better to write "latency offset" than "offset" in the
output, since I think it's not obvious to the user what just "offset"
means in this context.

-- 
Tanu