[pulseaudio-discuss] [PATCH 1/6] Implement the "volume sharing" feature.
Tanu Kaskinen
tanu.kaskinen at digia.com
Mon Feb 28 04:49:56 PST 2011
On Mon, 2011-02-28 at 13:38 +0200, Colin Guthrie wrote:
> 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?
I guess there would be no big problems with adding the flag to the
public API. I just wanted to avoid adding stuff to the public API that
isn't needed there.
> > } 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.
Yes, your proposal seems good to me.
> > @@ -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"?
"dest == i->sink" would be a better assertion, if the function wasn't
recursive. "dest == i->sink" is valid only for the input that is
actually being moved, not for any inputs that move with it.
But maybe this would be better than the current assertion:
pa_assert(i->sink == dest || (i->sink->flags & PA_SINK_SHARE_VOLUME_WITH_MASTER));
That will segfault if i->sink is NULL, though. The reason why I put the
assertion there in the first place was that I wanted to document that
this function is not called at that phase of sink input moving where
i->sink is NULL, but after that. I don't know what would be the best
choice here. Maybe the entire assertion should be removed, I'm sure it
won't catch anything anyway :)
> > + /* 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! :)
What questions were you left wondering about by the comment?
> > /* 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).
Yes, I hope it won't matter. Besides, at least to me it seems more
logical anyway to fire the hook only after the move operation has really
finished.
> > @@ -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?
You mean some other thread would read i->volume while the calculation is
unfinished? No, that can't happen, unless the other thread is buggy.
pa_sw_cvolume_multiply isn't atomic, so the same problem would be with
the previous implementation too.
> > @@ -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?
I have actually seen a bug in an out-of-tree module where the virtual
sink input was reinitialized (ie. replaced with a new one), and the
input_to_master pointer wasn't updated. So root_sink->input_to_master
pointed to a dead sink input that had its sink set to NULL as part of
the unlinking process, and this code then segfaulted...
So, I'm not opposed of having an assertion for
root_sink->input_to_master or input_to_master->sink or for both. It's
not necessarily redundant.
> > /* 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?
Yes, they probably should be squashed. I believe there was at some point
something between the code blocks, which prevented having just one "if
(volume)" block, but obviously there's nothing anymore.
> > @@ -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.
Ok.
> 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 :)
Thank you!
--
Tanu
More information about the pulseaudio-discuss
mailing list