[pulseaudio-discuss] [PATCH 1/5] core: Add infrastructure for synchronizing HW and SW volume changes

[Tanu Kaskinen]

On Fri, 2010-10-15 at 13:00 +0300, Jyri Sarha wrote:
> >> @@ -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.

No, soft volume syncing is not the problem, the problem is that in sync
volume mode set_volume is not called at all when send_msg is FALSE. In
all cases where send_msg is FALSE, the soft volume syncing is handled
differently - for example, when moving a stream,
pa_sink_input_finish_move() first calls pa_sink_set_volume() with
send_msg as FALSE, and after that it sends PA_SINK_MESSAGE_FINISH_MOVE.
The soft volume syncing is done by the PA_SINK_MESSAGE_FINISH_MOVE
handler.

> 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 :).

In your test case (changing sink input volume) send_msg is TRUE. Thanks
for the virtual beer :)

> >> +    }
> >> +    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.)

You did fix this, but you apparently didn't apply the fix to the
upstream version of the patches - hopefully you didn't forget any other
fixes (I checked, and I believe there isn't anything else missing).

-- 
Tanu Kaskinen

Digia Plc