[PATCH] Restore volume in pulseaudio

Post by Johannes WeiÃl
from
http://lists-archives.org/mplayer-dev-eng/28826-make-volume-restore-with-pulseaudio-work.html
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &pa_vol,
+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â NULL,

Hi Johannes,

Thanks a lot for the patch!

The only thing that bothers me a little is that at the very start
(before playback of first track is started) cmus shows 100% volume,
regardless of the "real" volume level.

This is caused by the fact that PulseAudio stream is created only once
sample format of a track is known, that is, only when the track starts
playing; and I don't know if it is possible to retrieve the volume
level before the stream exists. I've attached a patch that causes cmus
to show zero volume before the start of playback.

Do you think it makes any sense?

Â Â Â Â Â Â Â Â Gregory

Gregory Petrosyan

2010-12-10 17:39:44 UTC

On Fri, Dec 10, 2010 at 6:57 PM, Gregory Petrosyan

Post by Gregory Petrosyan
I've attached a patch that causes cmus
to show zero volume before the start of playback.

That's only for PulseAudio, of course.

Gregory

Johannes Weißl

2010-12-10 23:36:39 UTC

Hi Gregory,

Post by Gregory Petrosyan
The only thing that bothers me a little is that at the very start
(before playback of first track is started) cmus shows 100% volume,
regardless of the "real" volume level.
This is caused by the fact that PulseAudio stream is created only once
sample format of a track is known, that is, only when the track starts
playing; and I don't know if it is possible to retrieve the volume
level before the stream exists. I've attached a patch that causes cmus
to show zero volume before the start of playback.

Yeah, I realized that while writing the patch. VLC shows 100%,
gmplayer 0% before playing. In my opinion neither of these values makes
more sense than the other. Maybe it would be best to not show any value?
What do you think?

Johannes

Paul van der Walt

2010-12-11 10:33:23 UTC

Hi all,

Post by Johannes WeiÃl
What do you think?

I know this has nothing to do with me, but the last few days I
have been using PulseAudio for the first time, and I was highly
unimpressed that the defaults caused me to blast music really
loud out of my speakers (computer connected to powerful hifi),
before I figured out that you should set flat_volume to true (or
something like that; should be the default IMHO!). I therefore
really think this patch is a good idea, thanks!

Gregory Petrosyan

2010-12-13 15:55:09 UTC

Post by Paul van der Walt

Post by Johannes WeiÃl
What do you think?

Hi Paul,

Are you talking about the PulseAudio defaults or about cmus defaults?
Because if the latter, we should definitely fix all the wrong ones.

Gregory

Paul van der Walt

2010-12-13 18:17:11 UTC

Post by Paul van der Walt
I know this has nothing to do with me, but the last few days I
have been using PulseAudio for the first time, and I was highly
unimpressed that the defaults caused me to blast music really
loud out of my speakers (computer connected to powerful hifi),
before I figured out that you should set flat_volume to true (or
something like that; should be the default IMHO!). I therefore
really think this patch is a good idea, thanks!

Are you talking about the PulseAudio defaults or about cmus defaults?
Because if the latter, we should definitely fix all the wrong ones.

No, the defaults I'm referring to were PA. It made that since
cmus always started at 100%, the PA global volume was pulled up
to 100% too. This meant a lot of loud surprises.

As far as what would be desirable behaviour for cmus, I'm really
not sure. I guess I'm still not really used to the PA Way.

Paul

--
O< ascii ribbon campaign - stop html mail - www.asciiribbon.org

Gregory Petrosyan

2010-12-13 18:26:59 UTC

Post by Paul van der Walt

Are you talking about the PulseAudio defaults or about cmus defaults?
Because if the latter, we should definitely fix all the wrong ones.

No, the defaults I'm referring to were PA. It made that since
cmus always started at 100%, the PA global volume was pulled up
to 100% too. This meant a lot of loud surprises.

That is really strange — cmus should have no effect at all at the
global volume level (if using PA). It only controls volume of its own
private "sink". Do you use a PA-enabled distro or have you
installed/configured it yourself?

Gregory

Johannes Weißl

2010-12-13 19:38:55 UTC

That is really strange â cmus should have no effect at all at the
global volume level (if using PA). It only controls volume of its own
private "sink". Do you use a PA-enabled distro or have you
installed/configured it yourself?

Actually, it does (since 0.9.15) if flat-volumes = yes (which it is by
default!). I have my system level at 30%, after starting cmus (without
pulse-volume patches), both the system level and cmus' level are
increased to 100%. If flat-volumes is set to "no" in
/etc/pulse/daemon.conf, everything works as expected (no application
changes global level, all application levels are relative to the global
level). Thanks to this discussion I now know that I definitely want a
non-flat PA.

Johannes

Gregory Petrosyan

2010-12-13 20:04:15 UTC

Post by Gregory Petrosyan
That is really strange — cmus should have no effect at all at the
global volume level (if using PA). It only controls volume of its own
private "sink". Do you use a PA-enabled distro or have you
installed/configured it yourself?

Yeah, flat-volume feature
(http://0pointer.de/blog/projects/oh-nine-fifteen.html) is new to me
too. Hopefully, soon cmus will be usable no matter how flat or not the
volume is :-)

Gregory

Paul van der Walt

2010-12-13 19:47:25 UTC

Post by Paul van der Walt
No, the defaults I'm referring to were PA. It made that since
cmus always started at 100%, the PA global volume was pulled up
to 100% too. This meant a lot of loud surprises.

My media box runs Debian (testing). I just installed PA; the
option I had to set was

flat-volumes = no
in /etc/pulse/daemon.conf.

The default was yes, and I'm not sure, but the effect was that
the main volume was set to (at least) the program's volume (i.e.
decreasing cmus had no effect on the global volume, but on
increasing cmus' volume the main volume was kept >= cmus'
volume).

Paul

--
O< ascii ribbon campaign - stop html mail - www.asciiribbon.org

Gregory Petrosyan

2010-12-11 21:52:44 UTC

Post by Johannes WeiÃl
Hi Gregory,

The best way, of course, is to make cmus remember the volume itself,
for all output plugins. That way, the user will not experience any
surprises: from the very start cmus will display the correct volume
level.

I am just not entirely sure if this (changing the volume level when
mixer is open by cmus) is the desired behaviour for some legacy output
plugins (when cmus is e.g. 'attached' to some system-wide mixer). But
I think this is the best option overall — do you agree or not?

Gregory

Johannes Weißl

2010-12-13 09:28:25 UTC

Hi Gregory,

Post by Gregory Petrosyan
The best way, of course, is to make cmus remember the volume itself,
for all output plugins. That way, the user will not experience any
surprises: from the very start cmus will display the correct volume
level.
I am just not entirely sure if this (changing the volume level when
mixer is open by cmus) is the desired behaviour for some legacy output
plugins (when cmus is e.g. 'attached' to some system-wide mixer). But
I think this is the best option overall â do you agree or not?

I think cmus should never store or restore global mixer settings. But,
correct me if I'm wrong, most output plugins don't support private
mixer settings (alsa, ao, oss, sun), so saving is not desirable
here. For pulse, storing and restoring is done automatically, so no need
to duplicate it in cmus either.

Soft volume is private to cmus, but it is saved and restored already.
What about roar and arts? I've never used them...

I've attached a patch to not display any volume if it is unknown. It
needs the two other patches (yours and mine) to work.

Johannes

Gregory Petrosyan

2010-12-13 15:52:52 UTC

Post by Johannes WeiÃl
Hi Gregory,

I think cmus should never store or restore global mixer settings. But,
correct me if I'm wrong, most output plugins don't support private
mixer settings (alsa, ao, oss, sun), so saving is not desirable
here. For pulse, storing and restoring is done automatically, so no need
to duplicate it in cmus either.
Soft volume is private to cmus, but it is saved and restored already.
What about roar and arts? I've never used them...
I've attached a patch to not display any volume if it is unknown. It
needs the two other patches (yours and mine) to work.

OK, I've pushed all 3 patches to the jw/pa-volume branch for now, so
anyone interested can check them out.

To be honest, I don't like the result from the end user's standpoint.
Before our patches, cmus on top of PA would always start with 100%
volume, but the user sees this and can change the volume to the
desired level before the playback starts. Now, user has no idea if
cmus will shout or whisper after the playback will start, which is IMO
bad.

I'll try to find out if it is possible to take the best from both
approaches by using some magic PA APIs (I doubt that). If that is the
case, everything is fine.

Otherwise, if the current behaviour of jw/pa-volume branch is really
desired, I think the best will be to add a PA output plugin option for
that.

Gregory

Johannes Weißl

2010-12-13 16:12:25 UTC

Post by Gregory Petrosyan
To be honest, I don't like the result from the end user's standpoint.
Before our patches, cmus on top of PA would always start with 100%
volume, but the user sees this and can change the volume to the
desired level before the playback starts. Now, user has no idea if
cmus will shout or whisper after the playback will start, which is IMO
bad.

Yes, but chances are that the user wants to use the same volume as last
time, and even if not, he perhaps remembers what volume he used last
time. Plus, all other applications (sooner or later) will behave that
way. Right now cmus is shouting all the time when I start it, since I
use the microphone input of an old stereo, for which the input volume
has to be really low.

Post by Gregory Petrosyan
I'll try to find out if it is possible to take the best from both
approaches by using some magic PA APIs (I doubt that). If that is the
case, everything is fine.

We could initialize a "default" stream right at the start and get the
volume, but that would be very hacky...

Post by Gregory Petrosyan
Otherwise, if the current behaviour of jw/pa-volume branch is really
desired, I think the best will be to add a PA output plugin option for
that.

Yes, adding an option could make everyone happy. I vote for making it
default, but let's hear what others have to say.

Johannes

Mirko Augsburger

2010-12-13 22:29:49 UTC

Yes, but chances are that the user wants to use the same volume as last
time, and even if not, he perhaps remembers what volume he used last
time.

I, as a user, want cmus to always start with the same volume, because I
don't remember what I set it to the day before.
Probably you could add an option "vol_save" that the user can set to
what he like cmus to behave

Greeting
mist

a.l.e

2010-12-14 07:13:41 UTC

On Mon, 13 Dec 2010 23:29:49 +0100

Post by Mirko Augsburger

Post by Gregory Petrosyan
To be honest, I don't like the result from the end user's
standpoint. Before our patches, cmus on top of PA would always
start with 100% volume, but the user sees this and can change the
volume to the desired level before the playback starts. Now, user
has no idea if cmus will shout or whisper after the playback will
start, which is IMO bad.

Yes, but chances are that the user wants to use the same volume as
last time, and even if not, he perhaps remembers what volume he
used last time.

I, as a user, want cmus to always start with the same volume, because
I don't remember what I set it to the day before.
Probably you could add an option "vol_save" that the user can set to
what he like cmus to behave

i, as a user, too, want cmus to start at the current system volume... as far as i can tell this is also what it is doing in cmus 2.3.3 on debian testing.

when i change the volume i want that the system volume also gets modified.

ciao
a.l.e

Gregory Petrosyan

2010-12-16 16:21:38 UTC

OK,

I've spent a couple of hours with my beloved PulseAudio, and, as the
result, everybody should be able to enjoy the volume save/restore
functionality now.

Please check out the branch pa-stream-restore (or pu, which contains
the pa-stream-restore) and report if it works for you or not!

Gregory

Johannes Weißl

2010-12-16 19:53:03 UTC

Post by Gregory Petrosyan
I've spent a couple of hours with my beloved PulseAudio, and, as the
result, everybody should be able to enjoy the volume save/restore
functionality now.
Please check out the branch pa-stream-restore (or pu, which contains
the pa-stream-restore) and report if it works for you or not!

The idea is great! I wrote a patch for an option (mixer.pulse.restore),
but this is better. I had a few problems though: First cmus was crashing
when changing the volume after a track has stopped (which I could not
reproduce later, maybe I have to delete the ~/.pulse directory), then
starting and stopping a track multiple times (fast) let to two cmus
streams in pavucontrol and cmus freezing. I will investigate that
further!

Greetings,
Johannes

Gregory Petrosyan

2010-12-20 08:17:23 UTC

Any updates on this?

I wasn't able to reproduce this; the only thing that I've managed to
achieve is "failed to open output plugin" message which can appear
when spamming play/stop really hard, but I doubt it is in any way
related to these patches.

Gregory

Johannes Weißl

2010-12-20 08:49:10 UTC

I had a few problems though: First cmus was crashing when changing
the volume after a track has stopped (which I could not reproduce
later, maybe I have to delete the ~/.pulse directory), then starting
and stopping a track multiple times (fast) let to two cmus streams
in pavucontrol and cmus freezing. I will investigate that further!

Any updates on this?
I wasn't able to reproduce this; the only thing that I've managed to
achieve is "failed to open output plugin" message which can appear
when spamming play/stop really hard, but I doubt it is in any way
related to these patches.

At least managed to reproduce certain errors:

- "opening audio device: internal error"
1. pkill -f pulse ; rm -rf ~/.pulse* ; sleep 5
2. open cmus
3. start playing
4. stop playing
5. try to start playing again -> "opening audio device: internal error"
6. try to change volume -> crash in pulseaudio or cmus hanging
- can't change volume at all if no previous settings exist
1. pkill -f pulse ; rm -rf ~/.pulse* ; sleep 5
2. open cmus
3. unable to change volume in cmus at all (stays always at 0)

Are you able to reproduce this?

Johannes

Johannes Weißl

2010-12-20 12:16:35 UTC

Also, the restore patch returns wrong results for me, e.g.
- start cmus, volume is at 96%
- play a song, volume is at 75%
- stop playing (volume is back at 96%), reduce volume to 76%
- start playing again, volume is at 59% (?)

Maybe we shouldn't display / allow changing of volume in PA if there is
no playback at all?

Gregory Petrosyan

2010-12-20 20:05:39 UTC

Post by Johannes WeiÃl
- "opening audio device: internal error"
1. pkill -f pulse ; rm -rf ~/.pulse* ; sleep 5
2. open cmus
3. start playing
4. stop playing
5. try to start playing again -> "opening audio device: internal error"
6. try to change volume -> crash in pulseaudio or cmus hanging
- can't change volume at all if no previous settings exist
1. pkill -f pulse ; rm -rf ~/.pulse* ; sleep 5
2. open cmus
3. unable to change volume in cmus at all (stays always at 0)
Are you able to reproduce this?

No, I can't reproduce any of these. Latest Ubuntu,
PulseAudio 1:0.9.22~0.9.21+stable-queue-32-g8478-0ubuntu21.1. Both flat and
non-flat volume settings work.

However, after switching to the flat volume, cmus seems to be unable to save
stream volume anymore (even after switching back to non-flat and restarting
PA). Strange.

Gregory

Gregory Petrosyan

2010-12-20 20:20:01 UTC

No, I can't reproduce any of these. Latest Ubuntu,
PulseAudio 1:0.9.22~0.9.21+stable-queue-32-g8478-0ubuntu21.1. Both flat and
non-flat volume settings work.
However, after switching to the flat volume, cmus seems to be unable to save
stream volume anymore (even after switching back to non-flat and restarting
PA). Strange.

If I don't try to delete ~/.pulse* and pkill pulse, but instead use

pulseaudio -k && pulseaudio --start

to reload PA, then the *only* bug I can reproduce is saving/loading different
volumes when flat-volume feature is enabled.

Not sure if this is cmus' bug â it works fine with flat volume disabled. Will
investigate now.

Gregory

Johannes Weißl

2011-01-27 03:39:36 UTC

Hi!

I did a lot of testing, and I think I figured out two bugs in
pa-stream-restore:

1) "opening audio device: internal error":
"can't change volume at all if no previous settings exist"

In __pa_stream_restore_read_cb() we overwrite pa_cmap and pa_val even if
the information is invalid (which e.g. happens after a fresh start of
pulseaudio). If we have no valid settings, it should just return:

| @@ -500,6 +500,12 @@ static void __pa_stream_restore_read_cb(pa_context *c,
| if (!info || eol || strcmp(info->name, pa_stream_restore_id))
| return;
|
| + if (!pa_channel_map_valid(&info->channel_map))
| + return;
| +
| + if (!pa_cvolume_valid(&info->volume))
| + return;
| +
| pa_cmap = info->channel_map;
| pa_vol = info->volume;

2) "two different saved volumes (one when stopped, one when playing)"

This comes from the fact that we use two different pa_proplist, one when
stopped (restore id: "sink-input-by-application-name:C* Music Player")
and one when playing (restore id: "sink-input-by-media-role:music").
Is there a reason why we have two? I looked at rhythmbox and pacat and
they both seem to have only one. It would resolve the bug!

Another problem (or feature?) is PA_PROP_MEDIA_ROLE. If it is set, it
overrides all other proplist settings for the restore id. This means
other music player can change the start volume of cmus.

With flat-volumes=no almost everything works as expected now (besides a
rare crash in pulseaudio when often changing the volume, could be a bug
in PA). With flat-volumes=yes volumes still jumps pretty randomly (but
in my opinion that sadly seems to be the feature of flat-volumes).

I pushed my patches to
http://gitorious.org/~jmuc/cmus/jw-cmus/commits/pa-stream-restore-fixes

Johannes

Gregory Petrosyan

2011-01-30 14:56:48 UTC

Post by Johannes WeiÃl
I did a lot of testing, and I think I figured out two bugs in
"can't change volume at all if no previous settings exist"
In __pa_stream_restore_read_cb() we overwrite pa_cmap and pa_val even if
the information is invalid (which e.g. happens after a fresh start of
| if (!info || eol || strcmp(info->name, pa_stream_restore_id))
| return;
|
| + if (!pa_channel_map_valid(&info->channel_map))
| + return;
| +
| + if (!pa_cvolume_valid(&info->volume))
| + return;
| +
| pa_cmap = info->channel_map;
| pa_vol = info->volume;

Looks great, thanks!

Post by Johannes WeiÃl
2) "two different saved volumes (one when stopped, one when playing)"
This comes from the fact that we use two different pa_proplist, one when
stopped (restore id: "sink-input-by-application-name:C* Music Player")
and one when playing (restore id: "sink-input-by-media-role:music").
Is there a reason why we have two? I looked at rhythmbox and pacat and
they both seem to have only one. It would resolve the bug!
Another problem (or feature?) is PA_PROP_MEDIA_ROLE. If it is set, it
overrides all other proplist settings for the restore id. This means
other music player can change the start volume of cmus.

I think this is a "feature" of PA's media roles. Dunno if it is actually good,
though.

Post by Johannes WeiÃl
With flat-volumes=no almost everything works as expected now (besides a
rare crash in pulseaudio when often changing the volume, could be a bug
in PA). With flat-volumes=yes volumes still jumps pretty randomly (but
in my opinion that sadly seems to be the feature of flat-volumes).

Here's my experience with flat-volume: let us assume flat-volume is enabled, and
system-level volume is 100%; cmus is set to 60%. When you start decreasing the
system-level volume, cmus' volume changes, too, to more or less maintain
60/100 ratio. So cmus is, say, 31% now, and system volume is near 50%. After
cmus restart, it will show 60% (when nothing is playing), because that is the
number that PA saved. After the start of the playback, however, cmus' volume
will jump back to 31% again. Increasing the system-level volume to 100% will
bring cmus back to 60% again.

So, to ears, volume seems to restore properly.
Eyes, however, see something a little bit strange. Especially if in this
31%/~50% situation you will change cmus' volume: you will see e.g. 52% in
cmus, but after the restart (and only before the playback starts) it will be
showing something like 80% (because, again, seems like PA saves "volume app
will have if the system volume will be changed to 100%"), the number you've
never seen before.

Sorry for this wall of text :-)

I still don't quite understand how flat-volume is supposed to work, I guess I
need to consult PA mailing list on all these issues.

Gregory

Johannes Weißl

2011-01-31 08:26:14 UTC

I think this is a "feature" of PA's media roles. Dunno if it is actually good,
though.

Yeah, I guess that's alright. What about that "only one proplist" thing
though? If both patches are good, you could push them to the
pa-stream-restore branch.

Here's my experience with flat-volume: let us assume flat-volume is enabled, and
system-level volume is 100%; cmus is set to 60%. When you start decreasing the
system-level volume, cmus' volume changes, too, to more or less maintain
60/100 ratio. So cmus is, say, 31% now, and system volume is near 50%. After
cmus restart, it will show 60% (when nothing is playing), because that is the
number that PA saved. After the start of the playback, however, cmus' volume
will jump back to 31% again. Increasing the system-level volume to 100% will
bring cmus back to 60% again.
So, to ears, volume seems to restore properly.
Eyes, however, see something a little bit strange. Especially if in this
31%/~50% situation you will change cmus' volume: you will see e.g. 52% in
cmus, but after the restart (and only before the playback starts) it will be
showing something like 80% (because, again, seems like PA saves "volume app
will have if the system volume will be changed to 100%"), the number you've
never seen before.
Sorry for this wall of text :-)
I still don't quite understand how flat-volume is supposed to work, I guess I
need to consult PA mailing list on all these issues.

Many people don't, the web is full of rant against it. Here is a long
discussion on the mailing list:
https://tango.0pointer.de/pipermail/pulseaudio-discuss/2009-May/003925.html

I think the stream-restore extension is very rarely used by media
players (if at all!), and flat-volumes were introduced pretty recently,
so they are incompatible. If we could read system level volume and
flat-volumes setting, we could correct it ourselves... but maybe that's
too much already. All other players just pass NULL to context_new() and
allow changing volume only when playing. I made a short list of some
players I have installed (I think I've sent it to you earlier
already...):

totem: doesn't allow stopping, doesn't show volume before playback
rhythmbox: doesn't allow stopping, volume slider is useless (has no
effect / shows wrong values) before playback
vlc: uses soft-mixer
gmplayer: shows 0% volume before playback, crashes if volume is changed
while stopped
amarok: doesn't allow stopping, volume slider is useless (has no
effect / shows wrong values) before playback

I would suggest the following for cmus:
- by default, pass NULL to context_new() and don't allow / display
volume when stopped (with appropriate error message when +/- is
pressed). flat-volumes is enabled by default also, and setting volume
to 100% is just wrong (and possible dangerous to ears) in this case
- introduce a pulse.* option to enable the old behaviour

If we manage to get pa-stream-restore to work 100% correct in all cases,
we can always replace the default behaviour later with it. What do you
think?

Johannes

Jason Woofenden

2011-01-31 15:28:35 UTC

Post by Johannes WeiÃl
- by default, pass NULL to context_new() and don't allow / display
volume when stopped (with appropriate error message when +/- is
pressed). flat-volumes is enabled by default also, and setting volume
to 100% is just wrong (and possible dangerous to ears) in this case

This gets my vote.

I don't fiddle with the cmus volume. I used pulseaudio's
gnome-volume-control to to make cmus a bit quieter than my speech
synthesizer, then day-to-day, I just use the keys on my keyboard
that are mapped to change the system-wide master volume.

This seems to me like the normal thing to do, though I guess we all
develop our habits independently.

If people think it is important for cmus to show it's personal
volume when it's not playing (and I hope they don't), then I think
cmus should do this by opening up the PA output stream right away
(and keeping it open while stopped.)

Thanks, - Jason

Gregory Petrosyan

2011-01-31 15:49:24 UTC

Post by Jason Woofenden
If people think it is important for cmus to show it's personal
volume when it's not playing (and I hope they don't), then I think
cmus should do this by opening up the PA output stream right away
(and keeping it open while stopped.)

It was discussed before — we can't do that (easily), because to open
PA stream, we need to know the sample format, and we do not know it
beforehand.

It really sucks that such a simple thing (proper volume control) is
nearly impossible to implement with the most advanced and widespread
linux audio system :-(

Gregory

Gregory Petrosyan

2011-01-31 15:44:45 UTC

I tend to agree with you on this now: "proper" solution is requiring
way too much effort (if is possible at all).

Post by Johannes WeiÃl
- introduce a pulse.* option to enable the old behaviour

Is this really required? If current behaviour is looking buggy to most
people, I don't think we should try to maintain compatibility.

Post by Johannes WeiÃl
If we manage to get pa-stream-restore to work 100% correct in all cases,
we can always replace the default behaviour later with it. What do you
think?

I'll create a new branch for all of this and push it to -pu as soon as
I'll have a little bit of free time (in the next couple of days I
hope, work is killing me right now :-)

Gregory

Johannes Weißl

2011-01-31 17:15:17 UTC

Post by Johannes WeiÃl
- by default, pass NULL to context_new() and don't allow / display
Â volume when stopped (with appropriate error message when +/- is
Â pressed). flat-volumes is enabled by default also, and setting volume
Â to 100% is just wrong (and possible dangerous to ears) in this case

I tend to agree with you on this now: "proper" solution is requiring
way too much effort (if is possible at all).

Yes... even with flat-volumes=no and the two patches applied to
pa-stream-restore, cmus crashes and hangs in pulse-libs when changing
volume (often) before playback for me :-(.

Post by Johannes WeiÃl
- introduce a pulse.* option to enable the old behaviour

Is this really required? If current behaviour is looking buggy to most
people, I don't think we should try to maintain compatibility.

I think the cmus audience might be split in half:
- New users with flat-volumes=yes (because they never bothered to change
the distro-default) consider it as outright bug if this new audio
player they installed messes with the volume so badly (and entirely
different as all the other players), i.e. sets system level always to
100% even if that is very very load (think of laptop headphones with
no volume slider).
- Experienced users with flat-volumes=no probably want the old behavior,
because it offers everything: correct volume is always displayed,
volume change is always possible, setting volume to 100% at start
doesn't hurt since without flat-volumes the system level is never
touched. For them cmus always worked out well. It would be bad to
remove this possibility, especially when it's so easy to keep both
(few code changes).

I attached two patches to illustrate how small the change (to maint)
would be.

Post by Johannes WeiÃl
If we manage to get pa-stream-restore to work 100% correct in all cases,
we can always replace the default behaviour later with it. What do you
think?

I'll create a new branch for all of this and push it to -pu as soon as
I'll have a little bit of free time (in the next couple of days I
hope, work is killing me right now :-)

No problem, I won't have as much time either, registered my masters's
thesis today :-).

Johannes

Gregory Petrosyan

2011-02-06 15:22:19 UTC

Post by Johannes WeiÃl
- introduce a pulse.* option to enable the old behaviour

Is this really required? If current behaviour is looking buggy to most
people, I don't think we should try to maintain compatibility.

- New users with flat-volumes=yes (because they never bothered to change
the distro-default) consider it as outright bug if this new audio
player they installed messes with the volume so badly (and entirely
different as all the other players), i.e. sets system level always to
100% even if that is very very load (think of laptop headphones with
no volume slider).
- Experienced users with flat-volumes=no probably want the old behavior,
because it offers everything: correct volume is always displayed,
volume change is always possible, setting volume to 100% at start
doesn't hurt since without flat-volumes the system level is never
touched. For them cmus always worked out well. It would be bad to
remove this possibility, especially when it's so easy to keep both
(few code changes).
I attached two patches to illustrate how small the change (to maint)
would be.

Thanks, I've merged the version you posted on Gitorious (with a few minor
modifications) to maint.

I am tempted now to simply merge these (or similar) patches to master, and
drop the pulse-stream-restore branch, because, despite all the effort, it is
not working properly with default flat-volumes PA configuration. Folks on PA
mailing list suggest to just "grey out the volume slider", too.

What do you think?

Gregory

Johannes Weißl

2011-02-07 07:34:35 UTC

Post by Gregory Petrosyan
Thanks, I've merged the version you posted on Gitorious (with a few minor
modifications) to maint.

Thanks!

Post by Gregory Petrosyan
I am tempted now to simply merge these (or similar) patches to master, and
drop the pulse-stream-restore branch, because, despite all the effort, it is
not working properly with default flat-volumes PA configuration. Folks on PA
mailing list suggest to just "grey out the volume slider", too.
What do you think?

Yes, I'm in favor for that! I have a question about the main branch
though: Nearly every patch for maint should go to master, too. Why is
the current strategy to wait until lots of patches have queued up in
maint until mering them back?

Johannes

Gregory Petrosyan

2011-02-07 21:25:57 UTC

Post by Johannes WeiÃl
Yes, I'm in favor for that! I have a question about the main branch
though: Nearly every patch for maint should go to master, too. Why is
the current strategy to wait until lots of patches have queued up in
maint until mering them back?

Well, I thought it would be a bit more logical to keep maint and
master mostly separate except for merges, and have them both in pu for
testing. It may very well be not the best development model though, I
think I'll start keeping all maint stuff in master all the time, and
backport the necessary commits to -maint.

Gregory

Johannes Weißl

2011-02-07 22:32:09 UTC

I think that would be perfect! It has only advantages (I think):
- cleaner git history (better for git bisect)
- easier creation of pu (no merge conflicts between maint and master)
- when writing new patches for master, nobody has to deal with buggy
(possibly segfaulting) code

Johannes

Johannes Weißl

2011-02-07 07:53:42 UTC

Post by Gregory Petrosyan
Thanks, I've merged the version you posted on Gitorious (with a few minor
modifications) to maint.

I saw that you replaced mixer.pulse.restore_volume 1/0 with true/false.
I intentionally didn't use true/false because the mixer-options are not
toggable (have no real boolean options). With true/false and without
toggle, it is a little counterintuitive to change the value. Do you
think toggle options should be possible for mixer/dsp?

As for the pa-stream-restore branch, do you think it would be good to
rename it old/pa-stream-restore and keep it around? Or should I push it
to my branch. I think it would be good to keep it visible if somebody
else in the future wants to improve it (maybe if pulseaudio changed
the extensions).

Johannes

Gregory Petrosyan

2011-02-10 15:48:36 UTC

Post by Gregory Petrosyan
Thanks, I've merged the version you posted on Gitorious (with a few minor
modifications) to maint.

In general, I think It definitely makes sense to toggle dsp/mixer
options, they are not special in this regard. It does not look like
much-requested feature now, however.

I've changed it back to 1/0, thanks for the review!

Gregory

Johannes Weißl

2011-02-10 15:56:47 UTC

Post by Gregory Petrosyan
Thanks, I've merged the version you posted on Gitorious (with a few minor
modifications) to maint.

In general, I think It definitely makes sense to toggle dsp/mixer
options, they are not special in this regard. It does not look like
much-requested feature now, however.
I've changed it back to 1/0, thanks for the review!

Thanks! Yes, I thought the same, it is not that important to spent a lot
of time (would need to use structures instead of simple strings for
mixer-options). I have a feeling it could be time for v2.3.4 and a
merge back to master now :-).

Johannes

Gregory Petrosyan

2011-02-10 16:24:48 UTC

I have a feeling it could be time for v2.3.4 and a merge back to master now :-)

Yeah, definitely — already working on it!

Gregory

Gregory Petrosyan

2011-02-10 15:18:52 UTC

Post by Johannes WeiÃl
I pushed my patches to
http://gitorious.org/~jmuc/cmus/jw-cmus/commits/pa-stream-restore-fixes

I've merged them, and renamed the branch to archive/pa-stream-restore. It will
not be merged into pu or anything, it will just hold all the work done in this
direction (per your suggestion).

Gregory

Johannes Weißl

2011-02-10 15:28:50 UTC

Post by Johannes WeiÃl
I pushed my patches to
http://gitorious.org/~jmuc/cmus/jw-cmus/commits/pa-stream-restore-fixes

I've merged them, and renamed the branch to archive/pa-stream-restore. It will
not be merged into pu or anything, it will just hold all the work done in this
direction (per your suggestion).

Perfect! Maybe it will be revived in the future, who knows...

Johannes

Jason Woofenden

2011-01-21 23:34:00 UTC

Hi all,

Sorry I've been out of touch for a year. Hopefully that won't
happen again. I just caught up on reading the mailing list. So glad
to see that you all are continuing to improve this lovely little
player.

I just git pulled about a year of updates!

I've got master, then I did a git merge on pa-stream-restore. That
seemed to do what it was supposed to (stop maxing my system volume
every time I launch cmus) but now cmus segfaults when I quit.
Here's what I see in the terminal:

*** glibc detected *** cmus: double free or corruption (fasttop): 0x0865dc20 ***
======= Backtrace: =========
/lib/i686/cmov/libc.so.6(+0x6b281)[0xb762b281]
/lib/i686/cmov/libc.so.6(+0x6cad8)[0xb762cad8]
/lib/i686/cmov/libc.so.6(cfree+0x6d)[0xb762fbbd]
/home/jasonwoof/software/virt/lib/cmus/op/pulse.so(+0x19e6)[0xb77949e6]
cmus(op_exit_plugins+0x43)[0x8064ce3]
cmus(main+0x54e)[0x8071a5e]
/lib/i686/cmov/libc.so.6(__libc_start_main+0xe6)[0xb75d6c76]
cmus[0x8050401]
======= Memory map: ========
08048000-0807c000 r-xp 00000000 fe:02 47189 /home/jasonwoof/software/virt/bin/cmus
0807c000-0807e000 rw-p 00033000 fe:02 47189 /home/jasonwoof/software/virt/bin/cmus
0807e000-08083000 rw-p 00000000 00:00 0
084e7000-08692000 rw-p 00000000 00:00 0 [heap]
b4a00000-b4a21000 rw-p 00000000 00:00 0
b4a21000-b4b00000 ---p 00000000 00:00 0
b4bdc000-b4bdd000 ---p 00000000 00:00 0
b4bdd000-b53dd000 rw-p 00000000 00:00 0
b558e000-b558f000 ---p 00000000 00:00 0
b558f000-b5d8f000 rw-p 00000000 00:00 0
b5d8f000-b5d90000 ---p 00000000 00:00 0
b5d90000-b6590000 rw-p 00000000 00:00 0
b6590000-b6591000 ---p 00000000 00:00 0
b6591000-b6d91000 rw-p 00000000 00:00 0
b6ef8000-b6f15000 r-xp 00000000 08:06 408909 /lib/libgcc_s.so.1
b6f15000-b6f16000 rw-p 0001c000 08:06 408909 /lib/libgcc_s.so.1
b6f32000-b6f42000 r-xp 00000000 08:06 418067 /lib/i686/cmov/libresolv-2.11.2.so
b6f42000-b6f43000 r--p 00010000 08:06 418067 /lib/i686/cmov/libresolv-2.11.2.so
b6f43000-b6f44000 rw-p 00011000 08:06 418067 /lib/i686/cmov/libresolv-2.11.2.so
b6f44000-b6f46000 rw-p 00000000 00:00 0
b6f46000-b70ab000 r-xp 00000000 08:06 585740 /usr/lib/libvorbisenc.so.2.0.7
b70ab000-b70bc000 rw-p 00165000 08:06 585740 /usr/lib/libvorbisenc.so.2.0.7
b70bc000-b7107000 r-xp 00000000 08:06 585660 /usr/lib/libFLAC.so.8.2.0
b7107000-b7108000 rw-p 0004b000 08:06 585660 /usr/lib/libFLAC.so.8.2.0
b7108000-b711b000 r-xp 00000000 08:06 418081 /lib/i686/cmov/libnsl-2.11.2.so
b711b000-b711c000 r--p 00012000 08:06 418081 /lib/i686/cmov/libnsl-2.11.2.so
b711c000-b711d000 rw-p 00013000 08:06 418081 /lib/i686/cmov/libnsl-2.11.2.so
b711d000-b711f000 rw-p 00000000 00:00 0
b711f000-b7123000 r-xp 00000000 08:06 585448 /usr/lib/libXdmcp.so.6.0.0
b7123000-b7124000 rw-p 00003000 08:06 585448 /usr/lib/libXdmcp.so.6.0.0
b7124000-b7128000 r-xp 00000000 08:06 409169 /lib/libattr.so.1.1.0
b7128000-b7129000 rw-p 00003000 08:06 409169 /lib/libattr.so.1.1.0
b7129000-b7161000 r-xp 00000000 08:06 408974 /lib/libdbus-1.so.3.4.0
b7161000-b7162000 r--p 00037000 08:06 408974 /lib/libdbus-1.so.3.4.0
b7162000-b7163000 rw-p 00038000 08:06 408974 /lib/libdbus-1.so.3.4.0
b7163000-b7167000 r-xp 00000000 08:06 582162 /usr/lib/libasyncns.so.0.1.0
b7167000-b7168000 rw-p 00003000 08:06 582162 /usr/lib/libasyncns.so.0.1.0
b7168000-b71c9000 r-xp 00000000 08:06 581033 /usr/lib/libsndfile.so.1.0.23
b71c9000-b71cb000 rw-p 00061000 08:06 581033 /usr/lib/libsndfile.so.1.0.23
b71cb000-b71cf000 rw-p 00000000 00:00 0
b71cf000-b71d6000 r-xp 00000000 08:06 408890 /lib/libwrap.so.0.7.6
b71d6000-b71d7000 rw-p 00007000 08:06 408890 /lib/libwrap.so.0.7.6
b71d7000-b71e3000 r-xp 00000000 08:06 581443 /usr/lib/libXi.so.6.1.0
b71e3000-b71e4000 rw-p 0000c000 08:06 581443 /usr/lib/libXi.so.6.1.0
b71e4000-b71f2000 r-xp 00000000 08:06 584535 /usr/lib/libXext.so.6.4.0
b71f2000-b71f3000 rw-p 0000d000 08:06 584535 /usr/lib/libXext.so.6.4.0
b71f3000-b71f6000 r-xp 00000000 08:06 409019 /lib/libuuid.so.1.3.0
b71f6000-b71f7000 rw-p 00002000 08:06 409019 /lib/libuuid.so.1.3.0
b71f7000-b720f000 r-xp 00000000 08:06 581824 /usr/lib/libxcb.so.1.1.0
b720f000-b7210000 rw-p 00017000 08:06 581824 /usr/lib/libxcb.so.1.1.0
b7210000-b7217000 r-xp 00000000 08:06 418072 /lib/i686/cmov/librt-2.11.2.so
b7217000-b7218000 r--p 00006000 08:06 418072 /lib/i686/cmov/librt-2.11.2.so
b7218000-b7219000 rw-p 00007000 08:06 418072 /lib/i686/cmov/librt-2.11.2.so
b7219000-b721c000 r-xp 00000000 08:06 408951 /lib/libcap.so.2.19
b721c000-b721d000 rw-p 00002000 08:06 408951 /lib/libcap.so.2.19
b721d000-b7222000 r-xp 00000000 08:06 584356 /usr/lib/libgdbm.so.3.0.0
b7222000-b7223000 rw-p 00004000 08:06 584356 /usr/lib/libgdbm.so.3.0.0
b7223000-b726d000 r-xp 00000000 08:06 696290 /usr/lib/libpulsecommon-0.9.21.so
b726d000-b726e000 rw-p 00049000 08:06 696290 /usr/lib/libpulsecommon-0.9.21.so
b726e000-b7272000 r-xp 00000000 08:06 584796 /usr/lib/libXtst.so.6.1.0
b7272000-b7273000 rw-p 00004000 08:06 584796 /usr/lib/libXtst.so.6.1.0
b7273000-b727a000 r-xp 00000000 08:06 581991 /usr/lib/libSM.so.6.0.1
b727a000-b727b000 rw-p 00006000 08:06 581991 /usr/lib/libSM.so.6.0.1
b727b000-b728f000 r-xp 00000000 08:06 696604 /usr/lib/libICE.so.6.3.0
b728f000-b7291000 rw-p 00013000 08:06 696604 /usr/lib/libICE.so.6.3.0
b7291000-b7292000 rw-p 00000000 00:00 0
b7292000-b73ab000 r-xp 00000000 08:06 695042 /usr/lib/libX11.so.6.3.0
b73ab000-b73af000 rw-p 00118000 08:06 695042 /usr/lib/libX11.so.6.3.0
b73af000-b73ef000 r-xp 00000000 08:06 697321 /usr/lib/libpulse.so.0.12.2
b73ef000-b73f0000 rw-p 00040000 08:06 697321 /usr/lib/libpulse.so.0.12.2
b73f0000-b7417000 r-xp 00000000 08:06 582113 /usr/lib/libvorbis.so.0.4.4
b7417000-b7418000 rw-p 00026000 08:06 582113 /usr/lib/libvorbis.so.0.4.4
b7418000-b742d000 r-xp 00000000 08:06 586200 /usr/lib/libmad.so.0.2.1
b742d000-b742e000 rw-p 00014000 08:06 586200 /usr/lib/libmad.so.0.2.1
b7443000-b744a000 r--s 00000000 08:06 499650 /usr/lib/gconv/gconv-modules.cache
b744a000-b75bf000 r--p 00000000 08:06 547937 /usr/lib/locale/locale-archive
b75bf000-b75c0000 rw-p 00000000 00:00 0
b75c0000-b7700000 r-xp 00000000 08:06 418082 /lib/i686/cmov/libc-2.11.2.so
b7700000-b7702000 r--p 0013f000 08:06 418082 /lib/i686/cmov/libc-2.11.2.so
b7702000-b7703000 rw-p 00141000 08:06 418082 /lib/i686/cmov/libc-2.11.2.so
b7703000-b7706000 rw-p 00000000 00:00 0
b7706000-b772a000 r-xp 00000000 08:06 418066 /lib/i686/cmov/libm-2.11.2.so
b772a000-b772b000 r--p 00023000 08:06 418066 /lib/i686/cmov/libm-2.11.2.so
b772b000-b772c000 rw-p 00024000 08:06 418066 /lib/i686/cmov/libm-2.11.2.so
b772c000-b772e000 r-xp 00000000 08:06 418074 /lib/i686/cmov/libdl-2.11.2.so
b772e000-b772f000 r--p 00001000 08:06 418074 /lib/i686/cmov/libdl-2.11.2.so
b772f000-b7730000 rw-p 00002000 08:06 418074 /lib/i686/cmov/libdl-2.11.2.so
b7730000-b7731000 rw-p 00000000 00:00 0
b7731000-b7774000 r-xp 00000000 08:06 408825 /lib/libncursesw.so.5.7
b7774000-b7777000 rw-p 00042000 08:06 408825 /lib/libncursesw.so.5.7
b7777000-b778c000 r-xp 00000000 08:06 418071 /lib/i686/cmov/libpthread-2.11.2.so
b778c000-b778d000 r--p 00014000 08:06 418071 /lib/i686/cmov/libpthread-2.11.2.so
b778d000-b778e000 rw-p 00015000 08:06 418071 /lib/i686/cmov/libpthread-2.11.2.sozsh: abort cmus

this happens every time. I tried with softvol=true and softvol=false

I thought I could help with testing this before I set PA's stupid flat-volumes thing to no.

I rebuilt with DEBUG=2 and export CFLAGS="-ggdb" and used gdb to get this backtrace:

#0 0xb7fe2424 in __kernel_vsyscall ()
#1 0xb7e1e751 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2 0xb7e21b82 in abort () at abort.c:92
#3 0xb7e5518d in __libc_message (do_abort=2, fmt=0xb7f19738 "*** glibc detected *** %s: %s: 0x%s ***\n")
at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
#4 0xb7e5f281 in malloc_printerr (action=<value optimized out>, str=0x6 <Address 0x6 out of bounds>, ptr=0xb1329200)
at malloc.c:6267
#5 0xb7e60ad8 in _int_free (av=<value optimized out>, p=<value optimized out>) at malloc.c:4795
#6 0xb7e63bbd in __libc_free (mem=0xb1329200) at malloc.c:3739
#7 0xb7fc6dda in __op_pulse_exit () at pulse.c:329
#8 0xb7fc6df0 in op_pulse_exit () at pulse.c:336
#9 0x08068d97 in op_exit_plugins () at output.c:171
#10 0x08076e2b in exit_all () at ui_curses.c:2173
#11 0x08076ff5 in main (argc=1, argv=0xbffff658) at ui_curses.c:2272

valgrind says this:

==11652== Memcheck, a memory error detector
==11652== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==11652== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==11652== Command: cmus
==11652==
==11652== Invalid free() / delete / delete[]
==11652== at 0x4023B6A: free (vg_replace_malloc.c:366)
==11652== by 0x478FDD9: __op_pulse_exit (pulse.c:329)
==11652== by 0x478FDEF: op_pulse_exit (pulse.c:336)
==11652== by 0x8068D96: op_exit_plugins (output.c:171)
==11652== by 0x8076E2A: exit_all (ui_curses.c:2173)
==11652== by 0x8076FF4: main (ui_curses.c:2272)
==11652== Address 0x78abc70 is 0 bytes inside a block of size 43 free'd
==11652== at 0x4023B6A: free (vg_replace_malloc.c:366)
==11652== by 0x478FDD9: __op_pulse_exit (pulse.c:329)
==11652== by 0x4790502: op_pulse_mixer_exit (pulse.c:560)
==11652== by 0x8068D7D: op_exit_plugins (output.c:169)
==11652== by 0x8076E2A: exit_all (ui_curses.c:2173)
==11652== by 0x8076FF4: main (ui_curses.c:2272)
==11652==
==11652==
==11652== HEAP SUMMARY:
==11652== in use at exit: 1,227,638 bytes in 32,534 blocks
==11652== total heap usage: 249,503 allocs, 216,970 frees, 9,623,074 bytes allocated
==11652==
==11652== LEAK SUMMARY:
==11652== definitely lost: 0 bytes in 0 blocks
==11652== indirectly lost: 0 bytes in 0 blocks
==11652== possibly lost: 370,348 bytes in 123 blocks
==11652== still reachable: 857,290 bytes in 32,411 blocks
==11652== suppressed: 0 bytes in 0 blocks
==11652== Rerun with --leak-check=full to see details of leaked memory
==11652==
==11652== For counts of detected and suppressed errors, rerun with: -v
==11652== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 176 from 13)

and just to make sure this isn't only an issue when merging master and
pa-stream-restore, I tried a clean checkout of pa-stream-restore, and I see
what appears to be the same crash on exit.

Oh, and I just double-checked that I do _not_ get the crash when running from
master (without merging in pa-stream-restore.)

Please let me know if there's more testing/diagnostics I can do to get further
info on this, or if you cannot reproduce this, please give me some pointers as
to where to look for cleaning this up.

I'm running pulseaudio-0.9.21-3 on debian unstable.

Thank you, - Jason

e***@cs.umn.edu

2011-01-22 01:56:14 UTC

I can't seem to recreate the errors. I updated master, then did a
merge with pa-stream-restore, which only changed pulse.c. After
compiling, I ran cmus, then closed it, but I didn't get a segfault.

Did merging change more files for you than for me by any chance?

-Evan

Jason Woofenden

2011-01-22 05:19:56 UTC

Post by e***@cs.umn.edu
I can't seem to recreate the errors. I updated master, then did a
merge with pa-stream-restore, which only changed pulse.c. After
compiling, I ran cmus, then closed it, but I didn't get a segfault.
Did merging change more files for you than for me by any chance?
-Evan

We should end up with exactly the same working tree. Here's what it
says when I merge pa-stream-restore:

Auto-merging pulse.c
Merge made by recursive.
pulse.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 128 insertions(+), 16 deletions(-)

Does your valgrind show something similar to mine? If this is a
double-free issue, then it's quite common for it to crash on one
system and not another.

Here's my script to rebuild cmus, if that's relevant:

#!/bin/bash

make uninstall
make clean

export CFLAGS="-ggdb"

./configure prefix=/home/jasonwoof/software/virt DEBUG=2 \
CONFIG_FLAC=n CONFIG_MAD=y CONFIG_MODPLUG=n CONFIG_MIKMOD=n \
CONFIG_MPC=n CONFIG_VORBIS=y CONFIG_TREMOR=n CONFIG_WAV=n \
CONFIG_WAVPACK=n CONFIG_MP4=n CONFIG_AAC=n CONFIG_FFMPEG=n \
CONFIG_PULSE=y CONFIG_ALSA=n CONFIG_AO=n CONFIG_ARTS=n \
CONFIG_OSS=n CONFIG_SUN=n CONFIG_WAVEOUT=n \
&& make -j2 && make install

Take care, - Jason

Gregory Petrosyan

2011-01-22 15:37:07 UTC

Welcome back :-)

Post by Paul van der Walt
I've got master, then I did a git merge on pa-stream-restore. That
seemed to do what it was supposed to (stop maxing my system volume
every time I launch cmus) but now cmus segfaults when I quit.
*** glibc detected *** cmus: double free or corruption (fasttop): 0x0865dc20 ***

Hmm, strange. I'll try to reproduce it this evening.

pa-stream-restore is not yet ready for master or -pu — with my
testing, it was behaving strangely (or, to be precise, PA was behaving
strangely) in either flat-volume or non-flat-volume configurations,
and I have not yet found a way to make it work in both of them.

Gregory

Johannes Weißl

2011-01-22 19:18:47 UTC