Bugzilla – Bug 1069
fade_volume broken
Last modified: 2008-08-18 10:53:01 UTC
Slim::Player::Player::fade_volume is supposed to provide a way to fade volume up or down over time. But it seems to do nothing. I believe the reason is it relies on Slim::Player::Client::volume to set the volume temporarily, that is without permanently altering the client's preferences. And that method has a problem. Here's the code: sub volume { my ($client, $volume, $temp) = @_; if (defined($volume)) { if ($volume > $client->maxVolume()) { $volume = $client->maxVolume(); } if ($volume < $client->minVolume()) { $volume = $client->minVolume(); } Slim::Utils::Prefs::clientSet($client, "volume", $volume) if (!$temp); } return Slim::Utils::Prefs::clientGet($client, "volume"); } As you can see, if $temp is true. This method does nothing. It seems that setting the preference is the only way to actually affect the players volume. I'd like to fade alarm sounds in without changing the player's prefs.
Not true, actually. Both Slim::Player::Squeezebox and Slim::Player::Squeezebox2 have a volume() method that should change the volume. For which type of hardware are you having problems?
I'm using SqueezeboxG at the moment (when I detected the problem). I suspect the problem is with Client::volume still, in that it returns the prefs setting, rather than the recently set temporary value. Squeezebox::play calls: $client->volume($client->volume()); (which in looking at it, seems like a pretty useless call). And $client->volume returns the pref, not the temp setting. So I'm guessing the fade stuff works only after the music is already playing. In my case I want the music to start softly, so I start the fade before playing.
Created attachment 336 [details] tell the server we're muting how does this look? if we've decided we're muting, seemed to make sense to set the pref it was checking. also, compares $fvolume{$client} since that's the temp volume.
Kevin, I think that's a different problem, if the mute setting is ignored. In my case, if mute is set I think I want to specifically unmute it, but I can do that seperately. My case, by the way, is the alarm clock. I want volume to start low an slowly rise. I've had too many rude awakenings. The fix for my case it to make client->volume() return the current volume, whether its temporary or not.
yeah, this fix was a bit of a shortcut around the problem. I'm still workng on soemthing. Looks like all the mute problems are tied into this. At times, I fix my muted playback problem, and to try to fix the unmute problem I end up breaking this. almost there tho. hopefully what I'm doing wont affect what you've managed.
Created attachment 338 [details] met mute handle mute, fade_volume handle fade ok, I think this should help overall. It clears up bug1027 as well. fade_volume should just handle the fade, becuase we use mute as a callback. this way you can use fade_volume with a different callback if ever needed. speed control should let this do a long fade.
ah yes, now I get it. the fix I have in, makes fade actually go through its process, but without the changes you have, Dave, it never changes the volume as it goes. I had to hardcode a far slower fade to really see it. got a patch for client::volume? I'm thinking something like: return $temp ? $volume : Slim::Utils::Prefs::clientGet($client, "volume");
Created attachment 344 [details] fix for fade_volume Kevin, I haven't explored the mute issue, so I don't know if this affects it. But here's the patch I'm thinking of checking in to allow alarms to fade in. Its somewhat more involved than your initial suggestion, which was the first thing I considered, but client->volume gets called from lots of places, and the temporary value gets quickly overwritten if we're not careful. Please take a look if you have time.
yes, I guess that explains some not so smooth transitions. I did experiment with the $client->fade_volume() function. While non-intuitive, it can be drawn out to longer times. This patch looks like we're duplicating function a bit (unless I'm getting my line numbers wrong). Does this use fade_volume, or is that really useless for anything but quick fades into pause, etc.
Kevin, I don't think we're trying to address the same issue here. I'm trying to fix fade_volume, which seemed to work when fading down (i.e. sleeping), but not fading up, and I wanted to let alarms fade up. And yes, I'm using fade_volume to fade over a long period of time, which is not necessarily optimal. It updates the volume 20 times a second, which to me seems a lot. So my plan is to check in most of my changes, but by default, I will not make alarms fade in. Users can turn that on by hand editing the prefs file (for now). However, the fix should make fade_volume work in all cases, fading up or down. -Dave
sounds good to me. I've looked at the fade timer too. I was thinking that might be nice to be an added argument to fade_volume, so a function could deviate from 20/sec if desired. if you get a clean fade in and fade out with this patch, then you got my goals covered too :)
Resolved with revision 2534. Kevin, I was considering another change to fade_volume as well, namely taking the desired volume as a parameter. That way something like the alarm could fade to the alarmvolume, which may be different from the default volume. As it is, the alarm code simply sets the volume to be the alarm volume. Which is then the permanent setting, not good, but that's a whole other issue.