[pulseaudio-discuss] [PATCH 1/5] core: Add infrastructure for synchronizing HW and SW volume changes
Colin Guthrie
gmane at colin.guthr.ie
Thu Oct 14 12:16:00 PDT 2010
Hi,
Review below:
'Twas brillig, and oku at iki.fi at 13/10/10 17:40 did gyre and gimble:
> + PA_SINK_SYNC_VOLUME = 0x0200U,
> + /**< The HW volume changes are syncronized with SW volume.
> + * \since X.X.XX */
> +
> } pa_sink_flags_t;
Can you put 0.9.22 here? If needed I'll do a global find and replace on
any \since 0.9.22's if/when we release that version from stable queue
instead, but I'll almost certainly forget to fix up x.x.xx replacements.
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index ff4cc17..0f2af4f 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> +struct sink_message_set_port {
> + pa_device_port *port;
> + int ret;
> +};
> +
I'll refer to this below[1]
> @@ -1459,7 +1493,8 @@ void pa_sink_set_volume(
> * apply one to s->soft_volume */
>
> pa_cvolume_reset(&s->soft_volume, s->sample_spec.channels);
> - s->set_volume(s);
> + if (!(s->flags & PA_SINK_SYNC_VOLUME))
> + s->set_volume(s);
>
> } else
> /* If we have no function set_volume(), then the soft volume
Hmm, should this "if" have an "else" that sets send_msg to TRUE
(send_msg is used just below where the context ends) otherwise if
send_msg is set to false, then the message will not be pushed to the
asyncq and thus the set_volume() function will never be called in
pa_sink_process_msg()[2]
> @@ -1474,18 +1509,22 @@ void pa_sink_set_volume(
> pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK|PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
> }
>
> -/* Called from main thread. Only to be called by sink implementor */
> +/* Called from the io thread if sync volume is used, otherwise from the main thread.
> + * Only to be called by sink implementor */
> void pa_sink_set_soft_volume(pa_sink *s, const pa_cvolume *volume) {
> pa_sink_assert_ref(s);
> - pa_assert_ctl_context();
> + if (s->flags & PA_SINK_SYNC_VOLUME)
> + pa_sink_assert_io_context(s);
> + else
> + pa_assert_ctl_context();
>
> if (!volume)
> pa_cvolume_reset(&s->soft_volume, s->sample_spec.channels);
> else
> s->soft_volume = *volume;
>
> - if (PA_SINK_IS_LINKED(s->state))
> - pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_VOLUME, NULL, 0, NULL) == 0);
> + if (PA_SINK_IS_LINKED(s->state) && !(s->flags & PA_SINK_SYNC_VOLUME))
> + pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_SOFT_VOLUME, NULL, 0, NULL) == 0);
> else
> s->thread_info.soft_volume = s->soft_volume;
> }
This is a little concerning. If I have a sink (e.g. an roap-sink) whcih
does not support PA_SINK_SYNC_VOLUME flag, then you are now sending me a
different message to the one I got before.
While there is nothing in the tree that does this, there may be external
uses of this in private trees and I don't think there is any need to
change the semantics of PA_SINK_MESSAGE_SET_VOLUME.
Would it be better to instead define:
PA_SINK_MESSAGE_SET_VOLUME_SYNCED to go before
PA_SINK_MESSAGE_SET_VOLUME, and not change the message here and instead
change the o->process_msg() calls that use PA_SINK_MESSAGE_SET_VOLUME to
use PA_SINK_MESSAGE_SET_VOLUME_SYNCED?
I think this would be semantically the same with regards to your
requirements and keeps any out-of-tree reliance on
PA_SINK_MESSAGE_SET_VOLUME message semantically unchanged.[3]
> @@ -1555,6 +1594,16 @@ static void propagate_real_volume(pa_sink *s, const pa_cvolume *old_real_volume)
> pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK|PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
> }
>
> +/* Called from io thread */
> +void pa_sink_update_volume_and_mute(pa_sink *s) {
> + pa_assert(s);
> + pa_sink_assert_io_context(s);
> +
> + pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s),
> + PA_SINK_MESSAGE_UPDATE_VOLUME_AND_MUTE,
> + NULL, 0, NULL, NULL);
> +}
> +
While the limiting of line length is appreciated, I think most of the
other calls are just long lines... so probably worth leaving it on one
line for consistency.
> @@ -1605,7 +1654,7 @@ void pa_sink_set_mute(pa_sink *s, pa_bool_t mute, pa_bool_t save) {
> + if (!(s->flags & PA_SINK_SYNC_VOLUME) && s->set_mute)
> @@ -1624,7 +1673,7 @@ pa_bool_t pa_sink_get_mute(pa_sink *s, pa_bool_t force_refresh) {
> + if (!(s->flags & PA_SINK_SYNC_VOLUME) && s->get_mute)
These two, as above.
> @@ -2000,6 +2049,14 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
>
> case PA_SINK_MESSAGE_SET_VOLUME:
>
> + if (s->flags & PA_SINK_SYNC_VOLUME) {
> + s->set_volume(s);
> + pa_sink_volume_change_push(s);
> + }
> + /* Fall through ... */
[2]: Just for reference, here is where the above could result in
s->set_volume() not being called.
> @@ -2127,6 +2204,21 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
> pa_sink_set_max_request_within_thread(s, (size_t) offset);
> return 0;
>
> + case PA_SINK_MESSAGE_SET_PORT:
> +
> + pa_assert(userdata);
> + if (s->set_port)
> + ((struct sink_message_set_port *) userdata)->ret = s->set_port(s, ((struct sink_message_set_port *) userdata)->port);
> + return 0;
The casting is a bug ugly here. Can you use a variable?
Comment about the ->ret assignment below too.
> @@ -2568,7 +2660,7 @@ size_t pa_sink_get_max_request(pa_sink *s) {
> /* Called from main context */
> int pa_sink_set_port(pa_sink *s, const char *name, pa_bool_t save) {
> pa_device_port *port;
> -
> + int ret;
> pa_sink_assert_ref(s);
> pa_assert_ctl_context();
>
> @@ -2588,7 +2680,14 @@ int pa_sink_set_port(pa_sink *s, const char *name, pa_bool_t save) {
> return 0;
> }
>
> - if ((s->set_port(s, port)) < 0)
> + if (s->flags & PA_SINK_SYNC_VOLUME) {
> + struct sink_message_set_port msg = { .port = port, ret = 0 };
> + pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_PORT, &msg, 0, NULL) == 0);
[1]: This looks wrong, but perhaps it's some magic syntax I don't
appreciate.
AFAICT, ret (the local var) is assigned to 0 in all cases. The actual
value of msg.ret is never inspected after calling sending the message.
i.e. the value set in the previous hunk above is never actually
inspected and pulled back into our local ret value, which leads to...
> + }
> + else
> + ret = s->set_port(s, port);
> +
> + if (ret < 0)
> return -PA_ERR_NOENTITY;
...ret always being 0 in the case of (s->flags & PA_SINK_SYNC_VOLUME)
AFAICT. If this is intended then it is better to do the check of ret
inside the else itself, but I suspect you want to get the ret value back
after sending the message...
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index ba547fc..6b55778 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -201,6 +263,7 @@ typedef enum pa_sink_message {
> PA_SINK_MESSAGE_REMOVE_INPUT,
> PA_SINK_MESSAGE_GET_VOLUME,
> PA_SINK_MESSAGE_SET_VOLUME,
> + PA_SINK_MESSAGE_SET_SOFT_VOLUME,
> PA_SINK_MESSAGE_SYNC_VOLUMES,
> PA_SINK_MESSAGE_GET_MUTE,
> PA_SINK_MESSAGE_SET_MUTE,
[3]: I mentioned this above, but to reinforce that, perhaps this
_SOFT_VOLUME should be dropped and a SET_VOLUME_SYNCED inserted above
SET_VOLUME.... wont describe why again here, just a reminder in the
right place to what I said above :)
I think that is all I have to say about this one. Thanks a lot of this
patch. I can see it's all very confusing to work with, but I think this
is an important fix (it is something people often complain about).
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/]
More information about the pulseaudio-discuss
mailing list