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
