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/]
