On 2014-08-27 12:09, Tanu Kaskinen wrote: > On Tue, 2014-08-26 at 13:54 +0200, David Henningsson wrote: >> Module-device-restore sets real_volume, but soft_volume remains at >> max, so if a device only has soft_volume (i e no hw volume controls), >> its volume was not restored correctly. > > module-device-restore deals with the sink reference volume only, so I > think the wording of the commit message should be improved. Claiming > that module-device-restore sets the real volume confused me a bit > (making me think initially the patch was totally misguided). It's true > that the real volume is always initialized directly from reference > volume, however. Right. It makes more sense to use reference_volume instead of real_volume here, so posting a v2 now. >> Reported-by: Richardo Salveti de Araujo <ricardo.salveti at canonical.com> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com> >> --- >> >> Note: I haven't looked in detail what happens if the hw steps are coarse, >> so we need to set both soft_volume and reference_volume from real_volume. >> So it's possible this is just a bandaid. > > I don't know why the steps being coarse would have any effect on the > logic - if there's hw volume, it will anyway be too coarse to be used > without software compensation (assuming that there's dB information - > otherwise soft_volume should always be at max anyway). > > I had a look, and if alsa provides dB information, alsa-sink.c will take > care of initializing real_volume and soft_volume correctly (btw, I think > it would be better if alsa-sink.c didn't directly manipulate real_volume > and soft_volume, but that's another discussion). sink.c will then update > reference_volume correctly in pa_sink_put(), so this case works fine. If > alsa doesn't provide dB information, alsa-sink.c will initialize > real_volume correctly and leave soft_volume alone (should be at max all > the time when there's no dB information), so this works too. > > The patch looks good to me, except for the commit message. Ok, thanks for the look and the review! -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic