After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 568936 - Add fade slider
Add fade slider
Status: RESOLVED FIXED
Product: gnome-media
Classification: Deprecated
Component: gnome-volume-control
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome media maintainers
gnome media maintainers
Depends on:
Blocks: 581922
 
 
Reported: 2009-01-24 00:59 UTC by Matthias Clasen
Modified: 2009-05-10 00:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gvc-fade-balance.patch (11.37 KB, patch)
2009-02-11 14:04 UTC, Bastien Nocera
needs-work Details | Review
gvc-fade-balance-3.patch (25.39 KB, patch)
2009-02-18 11:26 UTC, Bastien Nocera
needs-work Details | Review
gvc-fade-balance-4.patch (27.44 KB, patch)
2009-02-18 13:04 UTC, Bastien Nocera
none Details | Review
newer patch (28.02 KB, patch)
2009-05-02 01:46 UTC, Matthias Clasen
none Details | Review
gvc-fade-balance-5.patch (29.08 KB, patch)
2009-05-05 13:24 UTC, Bastien Nocera
needs-work Details | Review
gvc-fade-balance-6.patch (30.51 KB, patch)
2009-05-05 16:13 UTC, Bastien Nocera
none Details | Review
gvc-fade-balance-7.patch (32.87 KB, patch)
2009-05-05 17:07 UTC, Bastien Nocera
none Details | Review
a few fixes from Lennart (40.08 KB, patch)
2009-05-08 17:38 UTC, Lennart Poettering
none Details | Review
rebased patch (39.39 KB, patch)
2009-05-09 22:40 UTC, Lennart Poettering
none Details | Review

Description Matthias Clasen 2009-01-24 00:59:53 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.
Comment 1 Bastien Nocera 2009-02-11 09:03:34 UTC
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;
+//        }
Comment 2 Bastien Nocera 2009-02-11 10:26:29 UTC
Would require 0.9.15 (for can_fade).
Comment 3 Bastien Nocera 2009-02-11 14:04:15 UTC
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.
Comment 4 Bastien Nocera 2009-02-13 12:38:34 UTC
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.
Comment 5 Bastien Nocera 2009-02-13 15:16:01 UTC
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 ()).
Comment 6 Bastien Nocera 2009-02-18 11:26:29 UTC
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?
Comment 7 Bastien Nocera 2009-02-18 13:04:50 UTC
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...
Comment 8 Lennart Poettering 2009-02-18 16:51:12 UTC
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().

Comment 9 Matthias Clasen 2009-05-02 01:46:25 UTC
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.
Comment 10 Bastien Nocera 2009-05-05 13:24:47 UTC
Created attachment 134024 [details] [review]
gvc-fade-balance-5.patch

Fix build-time warning, and update for latest master.
Comment 11 Bastien Nocera 2009-05-05 13:31:43 UTC
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?
Comment 12 Lennart Poettering 2009-05-05 15:04:47 UTC
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);
  
Comment 13 Bastien Nocera 2009-05-05 16:13:58 UTC
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).
Comment 14 Bastien Nocera 2009-05-05 17:07:00 UTC
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.
Comment 15 Lennart Poettering 2009-05-08 17:38:11 UTC
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.
Comment 16 Bastien Nocera 2009-05-09 01:37:35 UTC
Doesn't apply cleanly against current master, could you please rebase?
Comment 17 Lennart Poettering 2009-05-09 22:40:03 UTC
Created attachment 134329 [details] [review]
rebased patch
Comment 18 Bastien Nocera 2009-05-10 00:55:39 UTC
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...