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

[Tanu Kaskinen]

On Thu, 2012-11-29 at 11:04 -0200, Flavio Ceolin wrote:
> Implement setting of more than one volume factor.  The
> real value of the volume_factor will be the multiplication of these
> values.

Thanks, applied to my "next" branch. I did a few small fixes and
cosmetic changes, see below:

> +static pa_cvolume volume_factor_from_hashmap_get(pa_hashmap *items, uint8_t channels) {

I didn't like the "get" in the function name, so I removed that. Also,
to avoid copying, I added a pa_cvolume pointer as the first function
parameter, so the function now operates directly on the target
pa_cvolume struct.

> +void pa_sink_input_add_volume_factor(pa_sink_input *i, const char *key, const pa_cvolume *volume_factor) {
> +    struct volume_factor_entry *v;
> +
> +    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));
> +
> +    v = volume_factor_entry_new(key, volume_factor);
> +    if (!pa_cvolume_compatible(volume_factor, &i->sample_spec))
> +        pa_cvolume_set(&v->volume, i->sample_spec.channels, volume_factor->values[0]);
> +
> +    pa_assert_se(pa_hashmap_put(i->volume_factor_items, key, v) >= 0);

Changed "key" to "v->key".

> +    if (pa_hashmap_size(i->volume_factor_items) == 1)
> +        i->volume_factor = v->volume;
> +    else
> +        pa_sw_cvolume_multiply(&i->volume_factor, &i->volume_factor, &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_sink_input_assert_ref(i);
> +    pa_assert(key);
> +    pa_assert_ctl_context();
> +    pa_assert(PA_SINK_INPUT_IS_LINKED(i->state));
> +    pa_assert(v = pa_hashmap_remove(i->volume_factor_items, key));

If assertions are disabled, the pa_assert() condition is not evaluated
at all, so if the condition has side effects (like it does here)
pa_assert_se() must be used.

> +
> +    volume_factor_entry_free(v);
> +    switch (pa_hashmap_size(i->volume_factor_items)) {
> +        case 0:
> +            pa_cvolume_reset(&i->volume_factor, i->sample_spec.channels);
> +            break;
> +        case 1:
> +            v = (struct volume_factor_entry *)pa_hashmap_first(i->volume_factor_items);

I don't see the point of the cast here, so I removed it.

> @@ -1442,6 +1547,8 @@ pa_bool_t pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) {
>  /* Called from main context */
>  int pa_sink_input_start_move(pa_sink_input *i) {
>      pa_source_output *o, *p = NULL;
> +    struct volume_factor_entry *v;
> +    void *state = NULL;
>      int r;
>  
>      pa_sink_input_assert_ref(i);
> @@ -1479,7 +1586,12 @@ int pa_sink_input_start_move(pa_sink_input *i) {
>      pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i->sink), PA_SINK_MESSAGE_START_MOVE, i, 0, NULL) == 0);
>  
>      pa_sink_update_status(i->sink);
> -    pa_cvolume_remap(&i->volume_factor_sink, &i->sink->channel_map, &i->channel_map);
> +    PA_HASHMAP_FOREACH(v, i->volume_factor_items, state) {
> +        pa_cvolume_remap(&v->volume, &i->sink->channel_map, &i->channel_map);
> +    }
> +
> +    pa_cvolume_remap(&i->volume_factor, &i->sink->channel_map, &i->channel_map);

The remapping needs to be done for volume_factor_sink, not for
volume_factor. And the remapping needs to be done also in
pa_sink_input_finish_move() (I didn't realize that when I mentioned the
remapping issue in the last mail).

Also, blocks that have only one line inside them (like the
PA_HASHMAP_FOREACH block here) should not have the curly braces around
them.

>      i->sink = NULL;
>  
>      pa_sink_input_unref(i);
> diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
> index af2177a..10f19da 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. Its value is derived from the individual volume items in the volume_factor_items hashmap */
> +    pa_hashmap *volume_factor_items; /* it consists of volume items that are merged into one volume. This hashmap contains those individual items */

I was still not happy with the wording, and the volume_factor comment
line was excessively long, so I rewrote the comments as a block comment
above volume_factor.

-- 
Tanu