[pulseaudio-discuss] [PATCH v1 2/4] card-restore: Save/restore volume for the ports.

[Tanu Kaskinen]

On Sun, 2013-03-17 at 21:48 +0100, poljar (Damir Jelić) wrote:
> The card-restore module now saves and restores the volume per port.
> This change includes a entry version bump.
> 
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=55262
> ---
>  src/modules/module-card-restore.c | 68 +++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index 3b061b6..b7ff7bc 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -67,16 +67,18 @@ struct userdata {
>      pa_hook_slot *card_profile_hook_slot;
>      pa_hook_slot *port_new_hook_slot;
>      pa_hook_slot *port_offset_hook_slot;
> +    pa_hook_slot *port_volume_hook_slot;
>      pa_time_event *save_time_event;
>      pa_database *database;
>      bool hooks_connected;
>  };
>  
> -#define ENTRY_VERSION 2
> +#define ENTRY_VERSION 3
>  
>  struct port_info {
>      char *name;
>      int64_t offset;
> +    pa_cvolume volume;

I think a volume_is_set field is needed too.

>  };
>  
>  struct entry {
> @@ -121,6 +123,7 @@ static struct port_info *port_info_new(pa_device_port *port) {
>          p_info = pa_xnew(struct port_info, 1);
>          p_info->name = pa_xstrdup(port->name);
>          p_info->offset = port->latency_offset;
> +        p_info->volume = port->volume;

The volume should be saved only if that has been requested. I didn't
realize this when reviewing the previous patch, but we need a
save_volume field in pa_device_port (and pa_device_port_new_data).

>      } else
>          p_info = pa_xnew0(struct port_info, 1);
>  
> @@ -134,6 +137,7 @@ static void port_info_update(struct port_info *p_info, pa_device_port *port) {
>      pa_xfree(p_info->name);
>      p_info->name = pa_xstrdup(port->name);
>      p_info->offset = port->latency_offset;
> +    p_info->volume = port->volume;

Same as above: the volume should be saved only if that has been
requested.

>  }
>  
>  static void port_info_free(struct port_info *p_info) {
> @@ -185,7 +189,10 @@ static bool entrys_equal(struct entry *a, struct entry *b) {
>  
>      PA_HASHMAP_FOREACH(Ap_info, a->ports, state) {
>          if ((Bp_info = pa_hashmap_get(b->ports, Ap_info->name))) {
> -            if (Ap_info->offset != Bp_info->offset)
> +            if (Ap_info->offset != Bp_info->offset ||
> +                !pa_cvolume_valid(&Ap_info->volume) ||
> +                !pa_cvolume_valid(&Bp_info->volume) ||

If both volumes are unset, that should not make the entries unequal.

> +                !pa_cvolume_equal(&Ap_info->volume, &Bp_info->volume))
>                  return false;
>          } else
>              return false;
> @@ -213,6 +220,7 @@ static pa_bool_t entry_write(struct userdata *u, const char *name, const struct
>      PA_HASHMAP_FOREACH(p_info, e->ports, state) {
>          pa_tagstruct_puts(t, p_info->name);
>          pa_tagstruct_puts64(t, p_info->offset);
> +        pa_tagstruct_put_cvolume(t, &p_info->volume);

If the volume is unset, I think it shouldn't be written to the database.
The tagstruct functions for putting and getting cvolumes seem to handle
invalid volumes in a way that prevents serious issues even if the
volumes are invalid, but I still think it's cleaner not to write invalid
values (at least module-stream-restore seems to write invalid volumes
too, though).

>      }
>  
>      key.data = (char *) name;
> @@ -300,22 +308,26 @@ static struct entry* entry_read(struct userdata *u, const char *name) {
>          uint32_t port_count = 0;
>          const char *port_name = NULL;
>          int64_t port_offset = 0;
> +        pa_cvolume port_volume;
>          struct port_info *p_info;
>          unsigned i;
>  
>          if (pa_tagstruct_getu32(t, &port_count) < 0)
>              goto fail;
>  
> +        pa_cvolume_init(&port_volume);
>          for (i = 0; i < port_count; i++) {
>              if (pa_tagstruct_gets(t, &port_name) < 0 ||
>                  !port_name ||
>                  pa_hashmap_get(e->ports, port_name) ||
> -                pa_tagstruct_gets64(t, &port_offset) < 0)
> +                pa_tagstruct_gets64(t, &port_offset) < 0 ||
> +                (e->version >= 3 && pa_tagstruct_get_cvolume(t, &port_volume) < 0))
>                  goto fail;
>  
>              p_info = port_info_new(NULL);
>              p_info->name = pa_xstrdup(port_name);
>              p_info->offset = port_offset;
> +            p_info->volume = port_volume;
>  
>              pa_assert_se(pa_hashmap_put(e->ports, p_info->name, p_info) >= 0);
>          }
> @@ -358,9 +370,9 @@ static void show_full_info(pa_card *card) {
>      pa_assert(card);
>  
>      if (card->save_profile)
> -        pa_log_info("Storing profile and port latency offsets for card %s.", card->name);
> +        pa_log_info("Storing profile, port volumes and latency offsets for card %s.", card->name);
>      else
> -        pa_log_info("Storing port latency offsets for card %s.", card->name);
> +        pa_log_info("Storing port volumes and latency offsets for card %s.", card->name);
>  }
>  
>  static pa_hook_result_t card_put_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
> @@ -475,6 +487,37 @@ static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *
>      return PA_HOOK_OK;
>  }
>  
> +static pa_hook_result_t port_volume_change_callback(pa_core *c, pa_device_port *port, struct userdata *u) {
> +    struct entry *entry;
> +    pa_card *card;
> +
> +    pa_assert(port);
> +    card = port->card;
> +
> +    if ((entry = entry_read(u, card->name))) {
> +        struct port_info *p_info;
> +
> +        if ((p_info = pa_hashmap_get(entry->ports, port->name)))
> +            p_info->volume = port->volume;
> +        else {
> +            p_info = port_info_new(port);
> +            pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> +        }
> +
> +        pa_log_info("Storing volumes for port %s on card %s.", port->name, card->name);
> +
> +    } else {
> +        entry_from_card(card);
> +        show_full_info(card);
> +    }
> +
> +    if (entry_write(u, card->name, entry))
> +        trigger_save(u);
> +
> +    entry_free(entry);
> +    return PA_HOOK_OK;
> +}

The volume should be saved only if saving has been requested.

> +
>  static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
>      struct entry *e;
>      void *state;
> @@ -496,14 +539,19 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new
>              pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
>      }
>  
> -    /* Always restore the latency offsets because their
> -     * initial value is always 0 */
> +    /* Always restore the latency offsets and volumes because their
> +     * initial values are always 0 and PA_VOLUME_INVALID */

I don't think this makes sense. The volume should be restored only if no
volume has yet been set for the port.

This might apply to the latency offset too, or maybe not. The difference
between latency offset and volume is that volume is clearly a user
preference, while latency offset can be considered to be additional
hardware information that we just don't have means to query from the
hardware. Even if the offset is considered hardware information, there's
the argument that why should module-card-restore have the monopoly for
restoring the offset. Anyway, restoring the offset unconditionally in
module-card-restore doesn't currently have any practical problems, so
there's no real need to change anything about it now. Just change the
volume handling so that module-card-restore sets the volume if it's not
already set. And change the comment (or maybe remove it), because the
current rationale for always restoring the latency offset doesn't make
sense.

-- 
Tanu