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

[Tanu Kaskinen]

On Sat, 2010-10-16 at 12:34 +0100, Colin Guthrie wrote: 
> 'Twas brillig, and Tanu Kaskinen at 15/10/10 11:30 did gyre and gimble:
> > 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.
> 
> I've committed this patch now, but thinking about this in a bit more
> depth, is simply setting send_msg = true the right thing to do or should
> there be more structure in place to deal with this flag?
> 
> AFAICT it's fine the way it is, but as you've got a better idea of
> what's going on here than me, I figured I'd ask :D

I have meant to have a look at this for more than four months now, and
now I finally did. It seems to me that there never was any bug, even
though I thought also myself that there was. That is, the else branch
seems to be unnecessary. Was I wrong then or am I wrong now?

...Well, I did now some science, and I have empirical evidence that the
else branch is not needed after all. Patch will follow.

-- 
Tanu