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
