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
