[pulseaudio-discuss] [PATCH 1/5] core: Add infrastructure for synchronizing HW and SW volume changes
Jyri Sarha
oku at iki.fi
Fri Oct 15 03:00:25 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.
>
Sure.
>
>> 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]
>
It looks like you are right. I am only wondering how the code
still seems to work. If I understand correctly the same problem
is there also without sync-volume if sink updates soft volume
within set_volume(). The updated soft volume would not be
propagated to IO-thread.
The problem would be there also if we were using soft-volume only,
but then we would not be using flat volume and there would be no
pa_sink_set_volume() calls with send_msg=FALSE.
This all seems clear in theory. However, it does not work like
that when I try to produce the problem by updating a sink-input
volume and check that HW volume remains unchanged. Everything
just seems to work as it should. I'll offer virtual beer to one
who can explain why the code still works :).
Anyway I did change the code as you proposed and tested it and
the code still work.
>> @@ -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]
>
Seems reasonable. Changed.
>> @@ -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.
>
Changed. At least the unwrapped style has the benefit of giving more
useful result when greping the code.
>
>
>> @@ -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.
Sorry, I do not get it. What should be changed?
>
>> @@ -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.
>
Sure. Changed.
>
>> @@ -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.
>
You are right, the missing dot is not intentional. Fixed.
> 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...
>
Yep, fixed. (I think I already fixed that, but maybe I just thought I did.)
>
>> 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 :)
>
Changed (as I said earlier).
On Thu 14 Oct 2010 20:24:09 Colin Guthrie <gmane at colin.guthr.ie> wrote:
> Hi,
>
> Review below:
>
> 'Twas brillig, and oku at iki.fi at 13/10/10 17:40 did gyre and gimble:
> > diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> > index 1108a79..ada5da9 100644
> > --- a/src/modules/alsa/alsa-sink.c
> > +++ b/src/modules/alsa/alsa-sink.c
>
> > @@ -1651,8 +1713,15 @@ static int setup_mixer(struct userdata *u,
> pa_bool_t ignore_dB) {
> >
> > u->sink->get_volume = sink_get_volume_cb;
> > u->sink->set_volume = sink_set_volume_cb;
> > + u->sink->write_volume = sink_write_volume_cb;
> > +
> > + if (sync_volume) {
> > + u->sink->flags |= PA_SINK_HW_VOLUME_CTRL |
> > + (u->mixer_path->has_dB ? (PA_SINK_DECIBEL_VOLUME |
> PA_SINK_SYNC_VOLUME) : 0);
> > + pa_log_info("Successfully enabled synchronous volume.");
> > + } else
> > + u->sink->flags |= PA_SINK_HW_VOLUME_CTRL |
> (u->mixer_path->has_dB ? PA_SINK_DECIBEL_VOLUME : 0);
> >
> > - u->sink->flags |= PA_SINK_HW_VOLUME_CTRL |
(u->mixer_path->has_dB
> ? PA_SINK_DECIBEL_VOLUME : 0);
> > pa_log_info("Using hardware volume control. Hardware dB scale
> %s.", u->mixer_path->has_dB ? "supported" : "not supported");
> > }
>
>
> I think this could be written more neatly.. e.g. more like:
>
>
> u->sink->flags |= PA_SINK_HW_VOLUME_CTRL;
> if (u->mixer_path->has_dB) {
> u->sink->flags |= PA_SINK_DECIBEL_VOLUME;
> if (sync_volume) {
> u->sink->flags |= PA_SINK_SYNC_VOLUME;
> pa_log_info(....);
> }
> }
>
> While it's more assignment operations, I think it's easier to read what
> is being done.
>
Yes, your version is definitely better. Changed.
Thanks! I'll mail updated patches shortly. I am on a work trip
for whole next week, but if there is any need I'll get back to
this as soon as I can.
Cheers,
Jyri
ps. About the man page patch I leave you (assumed) native
speakers to discuss about wording.
More information about the pulseaudio-discuss
mailing list