[pulseaudio-discuss] [PATCH 1/3] sink_input: new volume_factor system

Tanu Kaskinen tanuk at iki.fi
Sat Nov 24 09:54:24 PST 2012


On Wed, 2012-11-14 at 15:19 -0200, Flavio Ceolin wrote:
> This patch makes possible to set more than one volume factor.  The
> real value of the volume_factor will be the multiplication of these
> values.

Please avoid breaking compilation between patches. Patches 1/3 and 2/3
should be squashed, otherwise module-position-event-sounds doesn't
compile after this patch.

One missing thing is remapping the volume_factor_sink entries in
pa_sink_input_start_move().

>  src/pulsecore/sink-input.c | 172 ++++++++++++++++++++++++++++++++++++++-------
>  src/pulsecore/sink-input.h |  19 +++--
>  2 files changed, 158 insertions(+), 33 deletions(-)
> 
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 7a7575a..22a462f 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -48,6 +48,34 @@
>  
>  PA_DEFINE_PUBLIC_CLASS(pa_sink_input, pa_msgobject);
>  
> +struct volume_factor_entry {
> +    char *key;
> +    pa_cvolume *volume;

The volume field can be just plain pa_cvolume instead of a pointer.

> +};
> +
> +static struct volume_factor_entry *volume_factor_entry_new(const char *key, const pa_cvolume *volume) {
> +    struct volume_factor_entry *entry;
> +

Here should be pa_assert(key) and pa_assert(volume).

> +    entry = pa_xnew(struct volume_factor_entry, 1);
> +    entry->key = pa_xstrdup(key);
> +
> +    entry->volume = pa_xnew(pa_cvolume, 1);
> +    *entry->volume = *volume;
> +
> +    return entry;
> +}
> +
> +static void volume_factor_entry_free(struct volume_factor_entry *volume_entry) {

Pointer assertion missing from here too.

> +    pa_xfree(volume_entry->key);
> +    pa_xfree(volume_entry->volume);
> +
> +    pa_xfree(volume_entry);
> +}
> +
> +static void volume_factor_entry_free2(struct volume_factor_entry *volume_entry, void *userdarta) {
> +    volume_factor_entry_free(volume_entry);
> +}

I would personally do an exception here and not have an assertion for
volume_entry, so this function doesn't need changing.

> +
>  static void sink_input_free(pa_object *o);
>  static void set_real_ratio(pa_sink_input *i, const pa_cvolume *v);
>  
> @@ -74,6 +102,9 @@ pa_sink_input_new_data* pa_sink_input_new_data_init(pa_sink_input_new_data *data
>      data->proplist = pa_proplist_new();
>      data->volume_writable = TRUE;
>  
> +    data->volume_factor_items = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> +    data->volume_factor_sink_items = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> +
>      return data;
>  }
>  
> @@ -111,28 +142,28 @@ void pa_sink_input_new_data_set_volume(pa_sink_input_new_data *data, const pa_cv
>          data->volume = *volume;
>  }
>  
> -void pa_sink_input_new_data_apply_volume_factor(pa_sink_input_new_data *data, const pa_cvolume *volume_factor) {
> +void pa_sink_input_new_data_add_volume_factor(pa_sink_input_new_data *data, const pa_cvolume *volume_factor, const char *key) {
> +    struct volume_factor_entry *v;
> +
>      pa_assert(data);
> +    pa_assert(key);
>      pa_assert(volume_factor);
>  
> -    if (data->volume_factor_is_set)
> -        pa_sw_cvolume_multiply(&data->volume_factor, &data->volume_factor, volume_factor);
> -    else {
> -        data->volume_factor_is_set = TRUE;
> -        data->volume_factor = *volume_factor;
> -    }
> +    v = volume_factor_entry_new(key, volume_factor);
> +    if (pa_hashmap_put(data->volume_factor_items, key, v) < 0)
> +        volume_factor_entry_free(v);

The key that is inserted to the hashmap has to be v->key, because we
don't have guarantees for the lifetime of the key that is passed as the
function argument.

Also, if pa_hashmap_put() fails here, that's a bug, so you can replace
the if with

pa_assert_se(pa_hashmap_put(data->volume_factor_items, v->key, v) >= 0);

>  }
>  
> -void pa_sink_input_new_data_apply_volume_factor_sink(pa_sink_input_new_data *data, const pa_cvolume *volume_factor) {
> +void pa_sink_input_new_data_add_volume_factor_sink(pa_sink_input_new_data *data, const pa_cvolume *volume_factor, const char *key) {
> +    struct volume_factor_entry *v;
> +
>      pa_assert(data);
> +    pa_assert(key);
>      pa_assert(volume_factor);
>  
> -    if (data->volume_factor_sink_is_set)
> -        pa_sw_cvolume_multiply(&data->volume_factor_sink, &data->volume_factor_sink, volume_factor);
> -    else {
> -        data->volume_factor_sink_is_set = TRUE;
> -        data->volume_factor_sink = *volume_factor;
> -    }
> +    v = volume_factor_entry_new(key, volume_factor);
> +    if (pa_hashmap_put(data->volume_factor_sink_items, key, v) < 0)
> +        volume_factor_entry_free(v);

Same comment as above.

>  }
>  
>  void pa_sink_input_new_data_set_muted(pa_sink_input_new_data *data, pa_bool_t mute) {
> @@ -204,6 +235,16 @@ void pa_sink_input_new_data_done(pa_sink_input_new_data *data) {
>      if (data->format)
>          pa_format_info_free(data->format);
>  
> +    if (data->volume_factor_items) {
> +        pa_hashmap_free(data->volume_factor_items, (pa_free2_cb_t) volume_factor_entry_free2, NULL);
> +        data->volume_factor_items = NULL;
> +    }
> +
> +    if (data->volume_factor_sink_items) {
> +        pa_hashmap_free(data->volume_factor_sink_items, (pa_free2_cb_t) volume_factor_entry_free2, NULL);
> +        data->volume_factor_sink_items = NULL;
> +    }

The other pointers aren't set to NULL either in this function, so I
think you shouldn't set these hashmaps to NULL either, for consistency.
Generally it's a good idea to set pointers to NULL after freeing, but
the convention is to not do it in free() and done() functions.

> +
>      pa_proplist_free(data->proplist);
>  }
>  
> @@ -247,6 +288,8 @@ int pa_sink_input_new(
>      char *memblockq_name;
>      pa_sample_spec ss;
>      pa_channel_map map;
> +    pa_cvolume *volume_factor;
> +    void *state = NULL;
>  
>      pa_assert(_i);
>      pa_assert(core);
> @@ -335,16 +378,6 @@ int pa_sink_input_new(
>  
>      pa_return_val_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec), -PA_ERR_INVALID);
>  
> -    if (!data->volume_factor_is_set)
> -        pa_cvolume_reset(&data->volume_factor, data->sample_spec.channels);
> -
> -    pa_return_val_if_fail(pa_cvolume_compatible(&data->volume_factor, &data->sample_spec), -PA_ERR_INVALID);
> -
> -    if (!data->volume_factor_sink_is_set)
> -        pa_cvolume_reset(&data->volume_factor_sink, data->sink->sample_spec.channels);
> -
> -    pa_return_val_if_fail(pa_cvolume_compatible(&data->volume_factor_sink, &data->sink->sample_spec), -PA_ERR_INVALID);
> -
>      if (!data->muted_is_set)
>          data->muted = FALSE;
>  
> @@ -450,8 +483,19 @@ int pa_sink_input_new(
>      } else
>          i->volume = data->volume;
>  
> -    i->volume_factor = data->volume_factor;
> -    i->volume_factor_sink = data->volume_factor_sink;
> +
> +    i->volume_factor_items = data->volume_factor_items;
> +    data->volume_factor_items = NULL;
> +    pa_cvolume_reset(&i->volume_factor, PA_CHANNELS_MAX);

While using PA_CHANNELS_MAX probably doesn't cause any visible bugs, you
can remove any doubt about that by passing i->sample_spec.channels
instead.

> +    PA_HASHMAP_FOREACH(volume_factor , i->volume_factor_items, state)
> +        pa_sw_cvolume_multiply(&i->volume_factor, &i->volume_factor, volume_factor);
> +
> +    i->volume_factor_sink_items = data->volume_factor_sink_items;
> +    data->volume_factor_sink_items = NULL;
> +    pa_cvolume_reset(&i->volume_factor_sink, PA_CHANNELS_MAX);
> +    PA_HASHMAP_FOREACH(volume_factor , i->volume_factor_sink_items, state)
> +        pa_sw_cvolume_multiply(&i->volume_factor_sink, &i->volume_factor_sink, volume_factor);

Maybe the hashmap->cvolume conversion could be done in a helper
function? It would be useful also in
pa_sink_input_remove_volume_factor().

> +
>      i->real_ratio = i->reference_ratio = data->volume;
>      pa_cvolume_reset(&i->soft_volume, i->sample_spec.channels);
>      pa_cvolume_reset(&i->real_ratio, i->sample_spec.channels);
> @@ -700,6 +744,11 @@ static void sink_input_free(pa_object *o) {
>      if (i->thread_info.direct_outputs)
>          pa_hashmap_free(i->thread_info.direct_outputs, NULL, NULL);
>  
> +    if (i->volume_factor_items)
> +        pa_hashmap_free(i->volume_factor_items, (pa_free2_cb_t) volume_factor_entry_free2, NULL);
> +    if (i->volume_factor_sink_items)
> +        pa_hashmap_free(i->volume_factor_sink_items, (pa_free2_cb_t) volume_factor_entry_free2, NULL);
> +
>      pa_xfree(i->driver);
>      pa_xfree(i);
>  }
> @@ -1209,6 +1258,77 @@ void pa_sink_input_set_volume(pa_sink_input *i, const pa_cvolume *volume, pa_boo
>      pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_CHANGE, i->index);
>  }
>  
> +void pa_sink_input_add_volume_factor(pa_sink_input *i, const pa_cvolume *volume_factor, const char *key) {
> +    struct volume_factor_entry *v;
> +    pa_cvolume aux;
> +
> +    pa_sink_input_assert_ref(i);
> +    pa_assert_ctl_context();
> +    pa_assert(PA_SINK_INPUT_IS_LINKED(i->state));
> +    pa_assert(volume_factor);
> +    pa_assert(key);
> +    pa_assert(pa_cvolume_valid(volume_factor));
> +    pa_assert(volume_factor->channels == 1 || pa_cvolume_compatible(volume_factor, &i->sample_spec));
> +
> +    if (!pa_cvolume_compatible(volume_factor, &i->sample_spec)) {
> +        aux = i->volume;
> +        volume_factor = pa_cvolume_scale(&aux, pa_cvolume_max(volume_factor));

This is not correct. i->volume isn't relevant here at all. The aux
variable isn't needed, if you create v first. Then you can just call
pa_cvolume_set(&v->volume, i->sample_spec.channels,
volume_factor.values[0]).

> +    }
> +
> +    v = volume_factor_entry_new(key, volume_factor);
> +    if (pa_hashmap_put(i->volume_factor_items, key, v) < 0) {
> +        volume_factor_entry_free(v);
> +        return;
> +    }

Same as before: if pa_hashmap_put() fails, that's a bug, so you can use
pa_assert_se() here.

> +
> +    if (pa_hashmap_size(i->volume_factor_items) == 1)
> +        i->volume_factor = *volume_factor;

Note that if you do the conversion from one channel to many in the way
that I suggest above, this needs to be changed to "i->volume_factor =
v->volume"...

> +    else
> +        pa_sw_cvolume_multiply(&i->volume_factor, &i->volume_factor, volume_factor);

...and here too the last parameter needs to be replaced with
"&v->volume".

> +
> +    pa_sw_cvolume_multiply(&i->soft_volume, &i->real_ratio, &i->volume_factor);
> +
> +    /* Copy the new soft_volume to the thread_info struct */
> +    pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_SOFT_VOLUME, NULL, 0, NULL) == 0);
> +}
> +
> +void pa_sink_input_remove_volume_factor(pa_sink_input *i, const char *key) {
> +    struct volume_factor_entry *v;
> +    pa_cvolume volume_factor;

This appears to be a redundant variable. You can use i->volume_factor
directly.

> +    void *state = NULL;
> +
> +    pa_sink_input_assert_ref(i);
> +    pa_assert(key);
> +    pa_assert_ctl_context();
> +    pa_assert(PA_SINK_INPUT_IS_LINKED(i->state));
> +
> +    v = pa_hashmap_remove(i->volume_factor_items, key);
> +    if (!v)
> +        return;

Assertion can be used here too.

> +
> +    volume_factor_entry_free(v);
> +    switch (pa_hashmap_size(i->volume_factor_items)) {
> +        case 0:
> +            pa_cvolume_reset(&volume_factor, PA_CHANNELS_MAX);

Replace "PA_CHANNELS_MAX" with "i->sample_spec.channels".

> +            break;
> +        case 1:
> +            v = (struct volume_factor_entry *)pa_hashmap_first(i->volume_factor_items);
> +            volume_factor = *v->volume;
> +            break;
> +        default:
> +            pa_cvolume_reset(&volume_factor, i->volume_factor.channels);
> +            PA_HASHMAP_FOREACH(v, i->volume_factor_items, state)
> +                pa_sw_cvolume_multiply(&volume_factor, &volume_factor, v->volume);
> +    }
> +
> +    i->volume_factor = volume_factor;
> +
> +    pa_sw_cvolume_multiply(&i->soft_volume, &i->real_ratio, &i->volume_factor);
> +
> +    /* Copy the new soft_volume to the thread_info struct */
> +    pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_SOFT_VOLUME, NULL, 0, NULL) == 0);
> +}
> +
>  /* Called from main context */
>  static void set_real_ratio(pa_sink_input *i, const pa_cvolume *v) {
>      pa_sink_input_assert_ref(i);
> diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
> index af2177a..31eedc1 100644
> --- a/src/pulsecore/sink-input.h
> +++ b/src/pulsecore/sink-input.h
> @@ -100,10 +100,12 @@ struct pa_sink_input {
>      pa_cvolume volume;             /* The volume clients are informed about */
>      pa_cvolume reference_ratio;    /* The ratio of the stream's volume to the sink's reference volume */
>      pa_cvolume real_ratio;         /* The ratio of the stream's volume to the sink's real volume */
> -    pa_cvolume volume_factor;      /* An internally used volume factor that can be used by modules to apply effects and suchlike without having that visible to the outside */
> -    pa_cvolume soft_volume;        /* The internal software volume we apply to all PCM data while it passes through. Usually calculated as real_ratio * volume_factor */
> +    pa_cvolume volume_factor;        /* An internally used volume factor that can be used by modules to apply effects and suchlike without having that visible to the outside */
> +    pa_hashmap *volume_factor_items; /* An internally used set of volumes factor that can be used by modules to apply effects and suchlike without having that visible to the outside */

I think the description for volume_factor_items could be more useful.
For example: "volume_factor can consist of volume items that are merged
into one volume. This hashmap contains those individual items."

> +    pa_cvolume soft_volume;          /* The internal software volume we apply to all PCM data while it passes through. Usually calculated as real_ratio * volume_factor */
>  
> -    pa_cvolume volume_factor_sink; /* A second volume factor in format of the sink this stream is connected to */
> +    pa_cvolume volume_factor_sink;        /* A second volume factor in format of the sink this stream is connected to */
> +    pa_hashmap *volume_factor_sink_items; /* A second set of volume factors in format of the sink this stream is connected to */

As above, this description could also say that volume_factor_sink is
derived from the individual volume items in this hashmap.

>      pa_bool_t volume_writable:1;
>  
> @@ -284,13 +286,14 @@ typedef struct pa_sink_input_new_data {
>      pa_idxset *req_formats;
>      pa_idxset *nego_formats;
>  
> -    pa_cvolume volume, volume_factor, volume_factor_sink;
> +    pa_cvolume volume;
>      pa_bool_t muted:1;
> +    pa_hashmap *volume_factor_items, *volume_factor_sink_items;
>  
>      pa_bool_t sample_spec_is_set:1;
>      pa_bool_t channel_map_is_set:1;
>  
> -    pa_bool_t volume_is_set:1, volume_factor_is_set:1, volume_factor_sink_is_set:1;
> +    pa_bool_t volume_is_set:1;
>      pa_bool_t muted_is_set:1;
>  
>      pa_bool_t volume_is_absolute:1;
> @@ -305,8 +308,8 @@ void pa_sink_input_new_data_set_sample_spec(pa_sink_input_new_data *data, const
>  void pa_sink_input_new_data_set_channel_map(pa_sink_input_new_data *data, const pa_channel_map *map);
>  pa_bool_t pa_sink_input_new_data_is_passthrough(pa_sink_input_new_data *data);
>  void pa_sink_input_new_data_set_volume(pa_sink_input_new_data *data, const pa_cvolume *volume);
> -void pa_sink_input_new_data_apply_volume_factor(pa_sink_input_new_data *data, const pa_cvolume *volume_factor);
> -void pa_sink_input_new_data_apply_volume_factor_sink(pa_sink_input_new_data *data, const pa_cvolume *volume_factor);
> +void pa_sink_input_new_data_add_volume_factor(pa_sink_input_new_data *data, const pa_cvolume *volume_factor, const char *key);

I would prefer having the volume_factor and key arguments the other way
around: when setting a key-value pair, usually the key is given first.

-- 
Tanu



More information about the pulseaudio-discuss mailing list