GNOME Bugzilla – Bug 568936
Add fade slider
Last modified: 2009-05-10 00:55:39 UTC
There should be a fade slider, like the balance slider, that controls front/back. It should only be enabled if the output actually has front/back channels.
To test: - In pacmd, call: load-module module-null-sink channels=6 - Apply small patch: Index: gvc-mixer-control.c =================================================================== --- gvc-mixer-control.c (revision 4177) +++ gvc-mixer-control.c (working copy) @@ -507,9 +507,9 @@ #endif /* for now completely ignore virtual streams */ - if (!(info->flags & PA_SINK_HARDWARE)) { - return; - } +// if (!(info->flags & PA_SINK_HARDWARE)) { +// return; +// }
Would require 0.9.15 (for can_fade).
Created attachment 128465 [details] [review] gvc-fade-balance.patch Half-done. on_adjustment_value_changed() needs fixing. Would also fix bug #569702 Waiting on the new PA release.
For can_balance = FALSE: load-module module-null-sink channels=1 But I can't find any ways to enable the fade, eg. pa_channel_map_can_fade() always returns false.
Working around a bug in the null module in PA: To load the a null sink with can-fade: load-module module-null-sink channels=6 channel_map=surround-51 or load-module module-null-sink channels=6 channel_map=front-left,front-right,rear-left,rear-right,lfe,front-center Problem I see now is that we should be using pa_cvolume_set_balance(), and pa_cvolume_set_fade(), not the current code in gvc-balance-bar.c (on_adjustment_value_changed ()).
Created attachment 128970 [details] [review] gvc-fade-balance-3.patch Testing this on a sink: ** (gnome-volume-control:20215): DEBUG: Volume changed ** (gnome-volume-control:20215): DEBUG: Changing volume for sink: n=6 vol=52427 bal=0.241584 fade=0.000000 ** Message: pa map (null) ** (gnome-volume-control:20215): DEBUG: volume for sink: bal=0.000000 fade=0.000000 I think our "saving" of the channel_map in gvc-channel-map.c doesn't actually work... Any ideas?
Created attachment 128975 [details] [review] gvc-fade-balance-4.patch This seems to work. Note that changing the balance/fade isn't a reversible operation, and will reduce the overall volume. I'm not sure that's correct...
A few comments: This is not correct: pa_cvolume_set (cv, map->priv->num_channels, 1.0); The third argument is a pa_volume_t, i.e. an integer, not a float. pa_cvolume_scale() modifies the volume in a way that afterwards pa_cvolume_max() equals what you passed in. These three lines: + pa_cvolume_init (cv); + pa_cvolume_set (cv, map->priv->num_channels, 1.0); + pa_cvolume_scale (cv, (pa_volume_t) volume); should be replaced by: pa_cvolume_set (cv, map->priv->num_channels, (pa_volume_t) volume); (given that volume is in range the usual 0..65536 range) _set_fade() and _set_balance() are designed that pa_cvolume_max() returns the curretn 'value' of the volume, not pa_cvolume_avg()! I.e. what _max() returns is left unmodified by _set_fade() and _set_balance().
Created attachment 133775 [details] [review] newer patch Here is an iteration that applies against 2.26.0. It also has the change pointed out by Lennart, and changes one pa_cvolume_avg back to pa_cvolume_max. That last change seems to have fixed the 'shrinking volume' problem. I don't have the hardware to test the fade slider, but at least the balance slider works as it used to with this patch.
Created attachment 134024 [details] [review] gvc-fade-balance-5.patch Fix build-time warning, and update for latest master.
Unless I'm doing something stupid, this patch has the same problem as my original patch in that the volume level will just sky-rocket when changing the balance. I got to 10000% volume level by just changing the balance and back (checked against pavucontrol). This only happens with > 2 speakers. You can create a fake 6 speakers sink using: load-module module-null-sink channels=6 channel_map=surround-51 in pacmd. Lennart, any ideas?
Hmpf. I am not happy with the fact that you store the value/balance/fade properties as is. I think it would be more correct to store them indirectly, in a pa_cvolume. Conversion from v/b/f to cvolume is not necessary reversible (which is btw explicitly documented). Conversion back to v/b/f from cvolume isn't either. Since cvolume is the format internally used by PA I'd suggest storing the values in this format everywhere and do the conversions only for presentation. For conversion from cvolume to v/b/f use: pa_cvolume_max() pa_cvolume_get_balance() pa_cvolume_get_fade() For conversion from v/b/f use: pa_cvolume_scale() pa_cvolume_set_balance() pa_cvolume_set_fade() Also, when doing this make sure to not synthesize a cvolume from nothing, but always base your work on the previous setting of cvolume. i.e. this is bad: /* Reininitalize cv completely */ pa_cvolume_set(&cv, nchannels, value); pa_cvolume_set_balance(&cv, balance); pa_cvolume_set_fade(&cv, fade); This is better: /* Reuse the previously initialized cv */ pa_cvolume_scale(&cv, value); pa_cvolume_set_balance(&cv, balance); pa_cvolume_set_fade(&cv, fade);
Created attachment 134039 [details] [review] gvc-fade-balance-6.patch This is still pretty rough, but it works. We're missing the gvc-balance-bar not listening for the volume-changed event (eg. changing the balance will change the fade).
Created attachment 134043 [details] [review] gvc-fade-balance-7.patch This seems to work better. The balance and fade are updated correctly when changed from the outside, and changing the fade or balance also has minore changes on the other one. Please test thoroughly, and I'll push that into master.
Created attachment 134273 [details] [review] a few fixes from Lennart This just fixes the feedback loop issue and reworks how the channel map/cvolume data is stored inside of GvcChannelMap.
Doesn't apply cleanly against current master, could you please rebase?
Created attachment 134329 [details] [review] rebased patch
Works for me. I fixed a few tabs creeping in from my out-of-band patch, and a few style changes. commit e4c8d688709e54a8fabfb818664874d9d77ca42d Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 10 01:46:33 2009 +0100 Bug 568936 – Add fade slider With help from Lennart Poettering and Matthias Clasen. - Add a new type of balance bar, the fade balance bar, to balance volume between front and rear - Don't update the UI when volume change operations are still pending on the sink we're modifying - Rework the internals to store pa_cvolume, like PA does internally, instead of balance/fade/volume separately, as the value isn't reversible to what we'd pass PA There's still a bug in GTK+ with the marks on the balance bar not looking centered. Will probably need to update the GTK+ reqs to avoid bug reports here...