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

[David Henningsson]

On 01/05/2013 12:19 AM, poljar (Damir Jelić) wrote:
> The card-restore module now saves and restores the volume per port.
> This change includes a entry version bump.

No big comments here either, just minor stuff. Good work :-)

>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=55262
> ---
>   src/modules/module-card-restore.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index 643e074..1f1c89f 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -68,11 +68,12 @@ struct userdata {
>       pa_database *database;
>   };
>
> -#define ENTRY_VERSION 2
> +#define ENTRY_VERSION 3
>
>   struct port_info {
>       char *name;
>       int64_t offset;
> +    pa_cvolume volume;
>   };
>
>   struct entry {
> @@ -145,6 +146,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);
>       }
>
>       key.data = (char *) name;
> @@ -232,9 +234,11 @@ 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;
>
> +        pa_cvolume_init(&port_volume);
>           if (pa_tagstruct_getu32(t, &port_count) < 0)
>               goto fail;
>
> @@ -242,12 +246,14 @@ static struct entry* entry_read(struct userdata *u, const char *name) {
>               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 = pa_xnew(struct port_info, 1);
>               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);
>           }
> @@ -313,6 +319,7 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
>           p_info = pa_xnew(struct port_info, 1);
>           p_info->name = pa_xstrdup(p->name);
>           p_info->offset = p->latency_offset;
> +        p_info->volume = p->volume;
>
>           pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
>       }
> @@ -332,7 +339,8 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
>
>               PA_HASHMAP_FOREACH(old_p_info, old->ports, state) {
>                   if ((p_info = pa_hashmap_get(entry->ports, old_p_info->name))) {
> -                    if (p_info->offset != old_p_info->offset) {
> +                    if (p_info->offset != old_p_info->offset ||
> +                        !pa_cvolume_equal(&p_info->volume, &old_p_info->volume)) {
>                           dirty = true;
>                           break;
>                       }
> @@ -354,9 +362,9 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
>       }
>
>       if (card->save_profile)
> -        pa_log_info("Storing profile and port latency offsets for card %s.", card->name);
> +        pa_log_info("Storing profile and port volume and latency offsets for card %s.", card->name);

Nitpick:

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 volume and latency offsets for card %s.", card->name);

s/volume/volumes

>
>       if (entry_write(u, card->name, entry))
>           trigger_save(u);
> @@ -369,6 +377,7 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new
>       void *state;
>       pa_device_port *p;
>       struct port_info *p_info;
> +    char buf[PA_CVOLUME_SNPRINT_MAX];
>
>       pa_assert(new_data);
>
> @@ -385,14 +394,17 @@ 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 volume because their
> +     * initial value is always 0 and PA_VOLUME_INVALID*/

Nitpick:
* initial value is always 0 and PA_VOLUME_INVALID */

>
> -    pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
> +    pa_log_info("Restoring port volume and latency offsets for card %s.", new_data->name);
>
>       PA_HASHMAP_FOREACH(p_info, e->ports, state)
> -        if ((p = pa_hashmap_get(new_data->ports, p_info->name)))
> +        if ((p = pa_hashmap_get(new_data->ports, p_info->name))) {

I would typically prefer having the declaration as close as possible, i 
e here:

char buf[PA_CVOLUME_SNPRINT_MAX];

...but it's mostly a matter of taste I guess.

>               p->latency_offset = p_info->offset;
> +            p->volume = p_info->volume;
> +            pa_log_info("Restored volume: for port %s - %s", p->name, pa_cvolume_snprint(buf, PA_CVOLUME_SNPRINT_MAX, &p_info->volume));
> +        }
>
>       entry_free(e);
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic