[PATCH 1/6] Implement the "volume sharing" feature.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hiya,

Some quick comments but these are to go on top rather than change this
as I've already merged it :)

'Twas brillig, and Tanu Kaskinen at 24/02/11 14:16 did gyre and gimble:
> When we have a filter sink that does some processing, currently the
> benefits of the flat volume feature are not really available. That's
> because if you have a music player that is connected to the filter sink,
> the hardware sink doesn't have any idea of the music player's stream
> volume.
> 
> This problem is solved by this "volume sharing" feature. The volume
> sharing feature works so that the filter sinks that want to avoid the
> previously described problem declare that they don't want to have
> independent volume, but they follow the master sink volume instead.
> The PA_SINK_SHARE_VOLUME_WITH_MASTER sink flag is used for that
> declaration. Then the volume logic is changed so that the hardware
> sink calculates its real volume using also the streams connected to the
> filter sink in addition to the streams that are connected directly to
> the hardware sink. Basically we're trying to create an illusion that
> from volume point of view all streams are connected directly to the
> hardware sink.
> 
> For that illusion to work, the volumes of the filter sinks and their
> virtual streams have to be managed carefully according to a set of
> rules:
> 
> If a filter sink follows the hardware sink volume, then the filter sink's
>  * reference_volume always equals the hw sink's reference_volume
>  * real_volume always equals the hw sink's real_volume
>  * soft_volume is always 0dB (ie. no soft volume)
> 
> If a filter sink doesn't follow the hardware sink volume, then the filter
> sink's
>  * reference_volume can be whatever (completely independent from the hw sink)
>  * real_volume always equals reference_volume
>  * soft_volume always equals real_volume (and reference_volume)
> 
> If a filter sink follows the hardware sink volume, and the hardware sink
> supports flat volume, then the filter sink's virtual stream's
>  * volume always equals the hw sink's real_volume
>  * reference_ratio is calculated normally from the stream volume and the hw
>    sink's reference_volume
>  * real_ratio always equals 0dB (follows from the first point)
>  * soft_volume always equals volume_factor (follows from the previous point)
> 
> If a filter sink follows the hardware sink volume, and the hardware sink
> doesn't support flat volume, then the filter sink's virtual stream's
>  * volume is always 0dB
>  * reference_ratio is always 0dB
>  * real_ratio is always 0dB
>  * soft_volume always equals volume_factor
> 
> If a filter sink doesn't follow the hardware sink volume, then the filter
> sink's virtual stream is handled as a regular stream.
> 
> Since the volumes of the virtual streams are controlled by a set of rules,
> the user is not allowed to change the virtual streams' volumes. It would
> probably also make sense to forbid changing the filter sinks' volume, but
> that's not strictly necessary, and currently changing a filter sink's volume
> changes actually the hardware sink's volume, and from there it propagates to
> all filter sinks ("funny" effects are expected when adjusting a single
> channel in cases where all sinks don't have the same channel maps).
> ---
>  src/pulse/def.h                 |   11 +
>  src/pulsecore/protocol-native.c |    2 +-
>  src/pulsecore/sink-input.c      |  204 +++++++++++++--
>  src/pulsecore/sink.c            |  555 ++++++++++++++++++++++++++++-----------
>  src/pulsecore/sink.h            |    4 +
>  5 files changed, 591 insertions(+), 185 deletions(-)
> 
> diff --git a/src/pulse/def.h b/src/pulse/def.h
> index 4a8da13..f6de7b3 100644
> --- a/src/pulse/def.h
> +++ b/src/pulse/def.h
> @@ -749,6 +749,16 @@ typedef enum pa_sink_flags {
>      /**< The HW volume changes are syncronized with SW volume.
>       * \since 1.0 */
>  
> +/** \cond fulldocs */
> +    /* PRIVATE: Server-side values -- do not try to use these at client-side.
> +     * The server will filter out these flags anyway, so you should never see
> +     * these flags in sinks. */
> +
> +    PA_SINK_SHARE_VOLUME_WITH_MASTER = 0x0400U,
> +    /**< This sink shares the volume with the master sink (used by some filter
> +     * sinks). */
> +/** \endcond */


Out of curiosity, is there any problem letting clients "see" these
flags. I can understand not letting them adjust/set them, but do we
really want to hide them away?

>  } pa_sink_flags_t;
>  
>  /** \cond fulldocs */
> @@ -762,6 +772,7 @@ typedef enum pa_sink_flags {
>  #define PA_SINK_DYNAMIC_LATENCY PA_SINK_DYNAMIC_LATENCY
>  #define PA_SINK_PASSTHROUGH PA_SINK_PASSTHROUGH
>  #define PA_SINK_SYNC_VOLUME PA_SINK_SYNC_VOLUME
> +#define PA_SINK_SHARE_VOLUME_WITH_MASTER PA_SINK_SHARE_VOLUME_WITH_MASTER

Could we have a PA_SINK_SERVER_SIDE_FLAGS define here that current is just:

#define PA_SINK_SERVER_SIDE_FLAGS PA_SINK_SHARE_VOLUME_WITH_MASTER

but could extend to:

#define PA_SINK_SERVER_SIDE_FLAGS
PA_SINK_SHARE_VOLUME_WITH_MASTER|PA_SINK_SOME_SECRET_FLAG


in the future.....


>  /** \endcond */
>  
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index 9257524..c812a3e 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -2902,7 +2902,7 @@ static void sink_fill_tagstruct(pa_native_connection *c, pa_tagstruct *t, pa_sin
>          PA_TAG_STRING, sink->monitor_source ? sink->monitor_source->name : NULL,
>          PA_TAG_USEC, pa_sink_get_latency(sink),
>          PA_TAG_STRING, sink->driver,
> -        PA_TAG_U32, sink->flags,
> +        PA_TAG_U32, sink->flags & ~PA_SINK_SHARE_VOLUME_WITH_MASTER,
>          PA_TAG_INVALID);


.... and then use PA_SINK_SERVER_SIDE_FLAGS here instead?

It'll just make grepping the code easier and aid future readability I think.

> @@ -1282,6 +1300,156 @@ int pa_sink_input_start_move(pa_sink_input *i) {
>      return 0;
>  }
>  
> +/* Called from main context. If i has an origin sink that uses volume sharing,
> + * then also the origin sink and all streams connected to it need to update
> + * their volume - this function does all that by using recursion. */
> +static void update_volume_due_to_moving(pa_sink_input *i, pa_sink *dest) {
> +    pa_cvolume old_volume;
> +
> +    pa_assert(i);
> +    pa_assert(dest);
> +    pa_assert(i->sink); /* The destination sink should already be set. */

If it should already be set, do you mean that dest==i->sink (in which
case that's what the assert should say) or should the comment read "The
previous sink should already be set"?

> +    /* If i->sink == dest, then recursion has finished, and we can finally call
> +     * pa_sink_set_volume(), which will do the rest of the updates. */
> +    if ((i->sink == dest) && pa_sink_flat_volume_enabled(i->sink))
> +        pa_sink_set_volume(i->sink, NULL, FALSE, i->save_volume);
> +}


Judging from this comment at the tail of that function, I'm guessing
that the comment in the above hunk needs a little clarification. The
rest of the comments in this function are awesomely detailed! :)

>  /* Called from main context */
>  int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, pa_bool_t save) {
>      pa_resampler *new_resampler;
> @@ -1355,17 +1523,7 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, pa_bool_t save) {
>      }
>      pa_sink_update_status(dest);
>  
> -    if (i->sink->flags & PA_SINK_FLAT_VOLUME) {
> -        pa_cvolume remapped;
> -
> -        /* Make relative volumes absolute */
> -        remapped = dest->reference_volume;
> -        pa_cvolume_remap(&remapped, &dest->channel_map, &i->channel_map);
> -        pa_sw_cvolume_multiply(&i->volume, &i->reference_ratio, &remapped);
> -
> -        /* We might need to update the sink's volume if we are in flat volume mode. */
> -        pa_sink_set_volume(i->sink, NULL, FALSE, i->save_volume);
> -    }
> +    update_volume_due_to_moving(i, dest);
>  
>      pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i->sink), PA_SINK_MESSAGE_FINISH_MOVE, i, 0, NULL) == 0);
>  
> @@ -1374,9 +1532,6 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, pa_bool_t save) {
>      /* Notify everyone */
>      pa_hook_fire(&i->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_FINISH], i);
>  
> -    if (i->volume_changed)
> -        i->volume_changed(i);
> -
>      pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_CHANGE, i->index);
>  
>      return 0;

The volume_changed now fires before the hook. I don't think this will
really matter tho' (especially considering the current usage of that hook).




> @@ -1395,9 +1549,9 @@ static void propagate_reference_volume(pa_sink *s) {
>           *
>           * i->volume := s->reference_volume * i->reference_ratio  */
>  
> -        remapped = s->reference_volume;
> -        pa_cvolume_remap(&remapped, &s->channel_map, &i->channel_map);
> -        pa_sw_cvolume_multiply(&i->volume, &remapped, &i->reference_ratio);
> +        i->volume = s->reference_volume;
> +        pa_cvolume_remap(&i->volume, &s->channel_map, &i->channel_map);
> +        pa_sw_cvolume_multiply(&i->volume, &i->volume, &i->reference_ratio);
>  
>          /* The volume changed, let's tell people so */
>          if (!pa_cvolume_equal(&old_volume, &i->volume)) {

I presume that removing the intermediate "remapped" variable here cannot
introduce any crazy race conditions?



> @@ -1445,76 +1647,82 @@ void pa_sink_set_volume(
>          }
>      }
>  
> +    /* In case of volume sharing, the volume is set for the root sink first,
> +     * from which it's then propagated to the sharing sinks. */
> +    while (root_sink->flags & PA_SINK_SHARE_VOLUME_WITH_MASTER)
> +        root_sink = root_sink->input_to_master->sink;
> +

I guess there is no need for a pa_assert_se() here? We know this will be
OK from setup checks etc?


>      /* As a special exception we accept mono volumes on all sinks --
>       * even on those with more complex channel maps */
>  
> +    if (volume) {
> +        if (pa_cvolume_compatible(volume, &s->sample_spec))
> +            new_reference_volume = *volume;
> +        else {
> +            new_reference_volume = s->reference_volume;
> +            pa_cvolume_scale(&new_reference_volume, pa_cvolume_max(volume));
> +        }
> +
> +        pa_cvolume_remap(&new_reference_volume, &s->channel_map, &root_sink->channel_map);
> +    }
> +
>      /* If volume is NULL we synchronize the sink's real and reference
>       * volumes with the stream volumes. If it is not NULL we update
>       * the reference_volume with it. */
>  
> -    old_reference_volume = s->reference_volume;
> -
>      if (volume) {

Smells like these two "if (volume)" blocks could be squashed
together.... unless this is for readability?


> @@ -1572,9 +1780,9 @@ static void propagate_real_volume(pa_sink *s, const pa_cvolume *old_real_volume)
>               * i->volume = s->reference_volume * i->reference_ratio
>               *
>               * This is identical to propagate_reference_volume() */
> -            remapped = s->reference_volume;
> -            pa_cvolume_remap(&remapped, &s->channel_map, &i->channel_map);
> -            pa_sw_cvolume_multiply(&i->volume, &remapped, &i->reference_ratio);
> +            i->volume = s->reference_volume;
> +            pa_cvolume_remap(&i->volume, &s->channel_map, &i->channel_map);
> +            pa_sw_cvolume_multiply(&i->volume, &i->volume, &i->reference_ratio);

Again "remapped" intermediate variable removed. Same race possibility
question as above.



> +/* Called from the IO thread. Only called for the root sink in volume sharing
> + * cases, except for internal recursive calls. */
> +static void set_shared_volume_within_thread(pa_sink *s) {
> +    pa_sink_input *i = NULL;
> +    void *state = NULL;
> +
> +    pa_sink_assert_ref(s);
> +
> +    PA_MSGOBJECT(s)->process_msg(PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_VOLUME_SYNCED, NULL, 0, NULL);
> +
> +    PA_HASHMAP_FOREACH(i, s->thread_info.inputs, state) {
> +        if (i->origin_sink && (i->origin_sink->flags & PA_SINK_SHARE_VOLUME_WITH_MASTER))
> +            set_shared_volume_within_thread(i->origin_sink);
> +    }
> +}
> +

Is PA_SINK_MESSAGE_SET_VOLUME_SYNCED rather than
PA_SINK_MESSAGE_SET_SHARED_VOLUME correct here?

I'm guessing from the below hunks that it is but it's getting a bit
confusing... with the message defines here...

>  /* Called from IO thread, except when it is not */
>  int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offset, pa_memchunk *chunk) {
>      pa_sink *s = PA_SINK(o);
> @@ -1913,7 +2143,7 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
>  
>              /* In flat volume mode we need to update the volume as
>               * well */
> -            return o->process_msg(o, PA_SINK_MESSAGE_SET_VOLUME_SYNCED, NULL, 0, NULL);
> +            return o->process_msg(o, PA_SINK_MESSAGE_SET_SHARED_VOLUME, NULL, 0, NULL);
>          }
>  
>          case PA_SINK_MESSAGE_REMOVE_INPUT: {
> @@ -1956,7 +2186,7 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
>  
>              /* In flat volume mode we need to update the volume as
>               * well */
> -            return o->process_msg(o, PA_SINK_MESSAGE_SET_VOLUME_SYNCED, NULL, 0, NULL);
> +            return o->process_msg(o, PA_SINK_MESSAGE_SET_SHARED_VOLUME, NULL, 0, NULL);
>          }
>  
>          case PA_SINK_MESSAGE_START_MOVE: {
> @@ -2001,7 +2231,7 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
>  
>              /* In flat volume mode we need to update the volume as
>               * well */
> -            return o->process_msg(o, PA_SINK_MESSAGE_SET_VOLUME_SYNCED, NULL, 0, NULL);
> +            return o->process_msg(o, PA_SINK_MESSAGE_SET_SHARED_VOLUME, NULL, 0, NULL);
>          }
>  
>          case PA_SINK_MESSAGE_FINISH_MOVE: {
> @@ -2042,9 +2272,17 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
>                  pa_sink_request_rewind(s, nbytes);
>              }
>  
> -            /* In flat volume mode we need to update the volume as
> -             * well */
> -            return o->process_msg(o, PA_SINK_MESSAGE_SET_VOLUME_SYNCED, NULL, 0, NULL);
> +            return o->process_msg(o, PA_SINK_MESSAGE_SET_SHARED_VOLUME, NULL, 0, NULL);
> +        }
> +
> +        case PA_SINK_MESSAGE_SET_SHARED_VOLUME: {
> +            pa_sink *root_sink = s;
> +
> +            while (root_sink->flags & PA_SINK_SHARE_VOLUME_WITH_MASTER)
> +                root_sink = root_sink->input_to_master->sink;
> +
> +            set_shared_volume_within_thread(root_sink);
> +            return 0;
>          }

The above changes make me think my previous comment does not require any
action. Just included for context.



This wasn't a massively thorough review, but it's the best I can likely
do without properly getting a handle on all the volume code - so sorry
about that.

In the mean time I've pushed all the changes I have and we can all go
mental on fixing it up :)

Cheers

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]




[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux