Bug 93855 - Module restore restores volume and mute settings from wrong sink input
Summary: Module restore restores volume and mute settings from wrong sink input
Status: RESOLVED MOVED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: modules (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-25 15:25 UTC by Leonard den Ottolander
Modified: 2018-07-30 10:12 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Separate pa_proplist_get_stream_id() function for use by module-stream-restore.c. (6.35 KB, patch)
2016-01-25 16:06 UTC, Leonard den Ottolander
Details | Splinter Review
Updated separate pa_proplist_get_stream_id() function for use by module-stream-restore.c. (6.82 KB, patch)
2016-01-25 18:48 UTC, Leonard den Ottolander
Details | Splinter Review

Description Leonard den Ottolander 2016-01-25 15:25:10 UTC
When using the stream restore module sink inputs have a property module-stream-restore.id that is set to something like '"sink-input-by-application-name:ALSA plug-in [mpg123]"'. Because an application name is not a unique identifier I suspect this is why on stream restoration volume and mute values get taken from the last modified input sink in a scenario where we have multiple sink inputs using the same application (name).

This causes behaviour like this (volinlistall is a wrapper around pacmd list-sink-inputs and volinset is a wrapper around pactl set-sink-input-volume):

$ volinlistall
11664 20% yes
11848 65% no
12021 33% yes
$ volinlistall
11664 20% yes
11848 65% no
12022 33% yes
$ volinset 11664 9%
$ volinlistall
11664 9% yes
11848 65% no
12022 33% yes
$ volinlistall
11664 9% yes
11848 65% no
12023 9% yes

The third sink input is being remapped repeatedly. After setting the volume for the first sink input the third takes over its volume. Same thing happens for mute settings (not shown in this example).

So module-stream-restore.c needs a unique sink input / source output id to restore them from the right source. The called pa_proplist_get_stream_group() in proplist-util.c currently provides the non unique string.

Jan 21 18:43:32 firewall pulseaudio[3081]: [pulseaudio] sink-input.c:     module-stream-restore.id = "sink-input-by-application-name:ALSA plug-in [mpg123]"

Basing the returned string from pa_proplist_get_stream_group() on the pid ensures a unique id is being returned. Only module-stream-restore.c and module-filter-apply.c rely on this function and I suppose the latter requires a unique id also.

--- proplist-util.c.000	2015-02-12 15:10:35.000000000 +0100
+++ proplist-util.c	2016-01-23 05:23:03.982692363 +0100
@@ -256,7 +256,9 @@ char *pa_proplist_get_stream_group(pa_pr
     if (!prefix)
         prefix = "stream";
 
-    if ((r = pa_proplist_gets(p, PA_PROP_MEDIA_ROLE)))
+    if ((r = pa_proplist_gets(p, PA_PROP_APPLICATION_PROCESS_ID)))
+        t = pa_sprintf_malloc("%s-by-application-pid:%s", prefix, r);
+    else if ((r = pa_proplist_gets(p, PA_PROP_MEDIA_ROLE)))
         t = pa_sprintf_malloc("%s-by-media-role:%s", prefix, r);
     else if ((r = pa_proplist_gets(p, PA_PROP_APPLICATION_ID)))
         t = pa_sprintf_malloc("%s-by-application-id:%s", prefix, r);


This crudely fixes the wandering of volume and mute settings over the sink inputs.

A more complete fix would be something like this, but another approach could be taken, for example using the sink-input id (which should be unique) itself.

--- pulseaudio-6.0/src/pulsecore/proplist-util.c.000	2015-02-12 15:10:35.000000000 +0100
+++ pulseaudio-6.0/src/pulsecore/proplist-util.c	2016-01-23 16:07:16.872497382 +0100
@@ -256,16 +256,25 @@ char *pa_proplist_get_stream_group(pa_pr
     if (!prefix)
         prefix = "stream";
 
-    if ((r = pa_proplist_gets(p, PA_PROP_MEDIA_ROLE)))
-        t = pa_sprintf_malloc("%s-by-media-role:%s", prefix, r);
-    else if ((r = pa_proplist_gets(p, PA_PROP_APPLICATION_ID)))
-        t = pa_sprintf_malloc("%s-by-application-id:%s", prefix, r);
-    else if ((r = pa_proplist_gets(p, PA_PROP_APPLICATION_NAME)))
-        t = pa_sprintf_malloc("%s-by-application-name:%s", prefix, r);
-    else if ((r = pa_proplist_gets(p, PA_PROP_MEDIA_NAME)))
-        t = pa_sprintf_malloc("%s-by-media-name:%s", prefix, r);
-    else
-        t = pa_sprintf_malloc("%s-fallback:%s", prefix, r);
+    if (r = pa_proplist_gets(p, PA_PROP_APPLICATION_PROCESS_ID))
+        t = pa_sprintf_malloc("%s-by-application-pid:%s", prefix, r);
+    else {
+        char randomstring[17];
+        int i;
+        long int l;
+
+        srandom(time(NULL));
+        for (i = 0; i < 16; i++) {
+            l = (random() & 31) + 97;
+            if (l > (25 + 97)) {
+                i--;
+                continue;
+            }
+            randomstring[i] = (char)l;
+        }
+        randomstring[16] = '\0';
+        t = pa_sprintf_malloc("%s-fallback:%s", prefix, randomstring);
+    }
 
     if (cache)
         pa_proplist_sets(p, cache, t);

I would like to suggest to rename pa_proplist_get_stream_group() to something like pa_proplist_get_stream_id() and implement it to generate a unique id in all circumstances. Note that the fallback uses a null pointer r.

Patches vs. CentOS 7's pulseaudio-6.0-7.el7.x86_64.
Comment 1 Tanu Kaskinen 2016-01-25 16:02:48 UTC
The purpose of module-stream-restore is to restore the previously set volume when an application reappears. Using the pid as the identifier would defeat this purpose, because when an application is restarted, it would look like a different application if it was identified by its pid.

Sometimes it would be desirable to opt out from restoring the volume for certain instances of applications, which is maybe what you'd like to do. We don't have a nice way to do that. It's possible, however, to work around that by setting an environment variable when starting an application: PULSE_PROP="module-stream-restore.id=foo".

If you want to disable volume restoring altogether, add the parameter "restore_volume=false" for module-stream-restore in /etc/pulse/default.pa, or comment out the module from the configuration (that will have the additional effect that stream routing won't be restored either).
Comment 2 Leonard den Ottolander 2016-01-25 16:06:02 UTC
Created attachment 121271 [details] [review]
Separate pa_proplist_get_stream_id() function for use by module-stream-restore.c.

Had a short look at module-filter-apply.c. It does in fact require the gathering of a stream group, i.e. multiple entries.

This however seems /not/ to be the case for module-stream-restore.c. It just requires a unique id to restore the sink inputs/source outputs.

I suppose a separate function pa_proplist_get_stream_id() needs to be implemented. See attached patch.
Comment 3 Leonard den Ottolander 2016-01-25 16:15:53 UTC
I think you misunderstand what I say.

To restore the stream a /unique/ identifier needs to be used. The currently used function function pa_proplist_get_stream_group() does not do this as it returns a non unique application name.

Using the pid of the original stream is perfectly fine. The module is just looking for a unique string and the pid of the (old) stream is such a unique name. The string is not used to look for the process with that particular pid, it is just a way to ensure that the string that identifies the stream is unique.

The patches in my first example actually fix the module restore stream, but as I explain in comment #2 reusing the function pa_proplist_get_stream_group() is a  bad idea as it interferes with module-filter-apply.c. This is why I split out the function pa_proplist_get_stream_id() in the attached patch.
Comment 4 Tanu Kaskinen 2016-01-25 16:35:29 UTC
I'm not convinced that I have misunderstood anything. The intended behaviour is that when you set volume for mpg123, the volume of all future instances of mpg123 will get that same volume, and the currently used id is sufficient to identify all instances of mpg123. I get the impression that you don't like that behaviour, and the solution to that is to disable volume restoring either on per-instance basis with the PULSE_PROP environment variable, or permanently by modifying default.pa.

I am confused, however, about this paragraph:

> Using the pid of the original stream is perfectly fine. The module is just
> looking for a unique string and the pid of the (old) stream is such a unique
> name. The string is not used to look for the process with that particular pid,
> it is just a way to ensure that the string that identifies the stream is
> unique.

Why do you say the string isn't used to look for the process with that particular pid? The string is used to group streams that belong to the same group. The string is the group id. If you use the pid in the string, then the group is defined as "all streams belonging to the process with this pid". Two different instances of mpg123 won't belong in the same group then, which I guess is what you want, but that's not what is usually wanted.
Comment 5 Leonard den Ottolander 2016-01-25 16:51:08 UTC
> The intended behaviour is that when you set volume for mpg123,
> the volume of all future instances of mpg123 will get that same volume

I am setting the volume for individual streams which all happen to be mpg123 instances. This causes all three sink inputs to be identified with the same identifier string, the application name in this case.

As you can see from my example, the three streams all have different volume settings. At a certain point I set the volume for si 11664 to 9%. A little while later si 12022 gets restored as 12023. But instead of setting the volume to the 33% which was the setting on si 12022, si 12023 now takes its volume from si 11664, the one that was last modified.

This behaviour is not limited to the volume setting, but also to the mute setting. This means that on restoration streams get muted and unmuted spontaneously. If you argue this is intended behaviour I'm stumped.

Perhaps a mechanism that can both identify a stream as part of a group /and/ individually is desired. But on sink input restoration I expect the restored stream to use the same volume and mute setting and not suddenly take on the settings of a random other stream that happens to use the same application name...
Comment 6 Tanu Kaskinen 2016-01-25 17:21:08 UTC
(In reply to Leonard den Ottolander from comment #5)
> > The intended behaviour is that when you set volume for mpg123,
> > the volume of all future instances of mpg123 will get that same volume
> 
> I am setting the volume for individual streams which all happen to be mpg123
> instances. This causes all three sink inputs to be identified with the same
> identifier string, the application name in this case.
> 
> As you can see from my example, the three streams all have different volume
> settings. At a certain point I set the volume for si 11664 to 9%. A little
> while later si 12022 gets restored as 12023. But instead of setting the
> volume to the 33% which was the setting on si 12022, si 12023 now takes its
> volume from si 11664, the one that was last modified.

Your proposal of using the pid as the identifier would not restore 12022's volume for 12023 either, because there would be nothing linking 12023 back to 12022.

It's working as designed. However, I disagree with the design in one thing: when you set the volume for 11664, I think the volume should be updated for all streams in the group. That is, 11484 and 12022 should change to 9% volume too. It should be pretty easy to make a patch to change this, but somehow I haven't got around to doing that...

> This behaviour is not limited to the volume setting, but also to the mute
> setting. This means that on restoration streams get muted and unmuted
> spontaneously. If you argue this is intended behaviour I'm stumped.
> 
> Perhaps a mechanism that can both identify a stream as part of a group /and/
> individually is desired.

There are many possible ways to group streams, and I agree that we lack a good mechanism to configure the grouping. It's very much hard-coded currently. Using the PULSE_PROP environment variable to set module-stream-restore.id is a flexible way to configure the grouping, but not user friendly at all.

> But on sink input restoration I expect the restored
> stream to use the same volume and mute setting and not suddenly take on the
> settings of a random other stream that happens to use the same application
> name...

If all current streams in a group would update their volumes when you set the volume of one stream, this source of confusion would disappear.
Comment 7 Leonard den Ottolander 2016-01-25 17:48:40 UTC
> Your proposal of using the pid as the identifier would not restore 12022's
> volume for 12023 either, because there would be nothing linking 12023 back
> to 12022.

Yes it does because yes there is! The entry for 12022 still exists at the point of restoration even if the pid no longer does. This patch DOES work. I am running with this patch atm and it fixes the issue I describe.

It is very much like the PULSE_PROP="module-stream-restore.id=foo" workaround you suggest. However, it seems rather ugly to have the user to provide a unique id to identify the stream instead of having pulse keeping track of this by itself.

Of course instead of using the pid using the sink input id itself would be even nicer, but it seems not to be available in the property list, which is why I fall back to using the pid or a unique string if no pid is available.

> It's working as designed. However, I disagree with the design in one thing:
> when you set the volume for 11664, I think the volume should be updated for
> all streams in the group. That is, 11484 and 12022 should change to 9% volume
> too. It should be pretty easy to make a patch to change this, but somehow I
> haven't got around to doing that...

Not agreeing with you. If I set the volume on a single sink input I want only that volume to change. pactl set-sink-input-volume should work on a single sink input volume. The behaviour you describe could be implemented as volinset pactl set-sink-input-group-volume.

The point is that on restoration NOTHING should change. No group setting should be applied. Only the current values should be restored.

I think the main confusion here is that the apply filter and stream restore modules use the same function pa_proplist_get_stream_group() to identify the streams. Using this function in module-filter-apply.c is fine but for stream restoration the entry should be uniquely identified. The attached patch defines the pa_proplist_get_stream_id() for this purpose and leaves pa_proplist_get_stream_group() intact for use by the apply filter module.

 > If all current streams in a group would update their volumes when you set
 > the volume of one stream, this source of confusion would disappear.

The source of confusion would disappear if a restored stream just takes it's orginal values. This can be accomplished by identifying the entry with a unique name, unrelated to the group name that you can still use for the application of filters.

By the way, are you sure module-filter-apply.c even relies on the module-stream-restore.id? I think that particular identification string is only used by module-stream-restore.c so if I am not mistaken there is no reason not to make this identifier unique.

To summarize: filter apply work on groups which I believe not to be impacted by identifying a stream with a unique id as proposed.
Comment 8 Leonard den Ottolander 2016-01-25 18:14:28 UTC
Please do not look at the inline patch proposals in the original comment. Look at the attached patch that leaves pa_proplist_get_stream_group() intact for use by module-filter-apply.c. In the attached patch a /new/ function pa_proplist_get_stream_id() is defined so module-stream-restore.c can identify the sink input entries uniquely.

If you think using a unique id to identify streams (sink input/source output) interferes with module-filter-apply.c then please explain how. Afaict the string "module-stream-restore.id" is only referenced in module-stream-restore.c and not by module-filter-apply.c. module-filter-apply.c just gets the group by calling pa_proplist_get_stream_group() directly.

To restore the original volume the streams need to be identified with a unique id. Currently there is no mechanism to uniquely identify entries. This is what I attempt to fix.

Please do not get hung up on the use of the pid. It is about labeling the different streams with unique ids to be able to distinguish them. I use the pid as that is readily available, but as you can see I fall back to creating a random string in case the pid is not available in the property list. Read "pid" as "a unique string identifying the stream" as I only reference the /entry/ with this id, I'm not trying to do anything with the process identified by this pid itself.
Comment 9 Leonard den Ottolander 2016-01-25 18:21:10 UTC
One last thing: When I say this works I mean I have tested the inline patch that still modifies pa_proplist_get_stream_group() (so that might interfere with filter application!) but not yet the attached patch that adds the function pa_proplist_get_stream_id().
Comment 10 Leonard den Ottolander 2016-01-25 18:48:05 UTC
Created attachment 121275 [details] [review]
Updated separate pa_proplist_get_stream_id() function for use by module-stream-restore.c.

Forgot to add declartion of pa_proplist_get_stream_id() to proplist-util.h. Fixed here.
Comment 11 Tanu Kaskinen 2016-01-25 19:57:39 UTC
Ok, I'm baffled, if you say that with your patch something actually gets restored. If you use pid or some random string as the entry id, I don't understand how two mpg123 invocations get linked together.

Let's have a simple example: you start mpg123, you set its volume to 42%, then the program exits, and you start mpg123 again. Do you expect the second stream volume to get restored to 42%? If yes, how exactly does pulseaudio know that it's the "same" stream as before? If not, how does your proposal differ from just having module-stream-restore not loaded (let's ignore mute and routing restoring for now)?

We can leave module-filter-apply out of the discussion for now. I'm not concerned about it as long as the situation with module-stream-restore is unclear.
Comment 12 Tanu Kaskinen 2016-01-25 20:07:27 UTC
Hmm, what you're saying makes sense if you're talking about a case where one mpg123 process plays several files, and creates separate streams for each file, and you'd like the volume persist only for the duration of the single process lifetime. In that case, sorry, users generally want their applications to retain their volume across reboots. Needing separate volumes for multiple instances of the same application running simultaneously is an edge case that has lower priority than the restore-across-reboots use case.
Comment 13 Leonard den Ottolander 2016-01-25 20:44:37 UTC
I'll try to rephrase.

I make 2 assumptions:

1) module-stream-restore.c is supposed to restore a stream, that is a sink input or a source output, with its original volume and mute settings.

2) To be able to restore the correct volume setting the entry of the original stream needs to be uniquely identified. Currently there is no mechanism to uniquely identify such an entry. The entries are labelled with non unique labels.


In the case where we have multiple instances of the same application (name) pa_proplist_get_stream_group() will return multiple values because there are multiple sink input entries with the same name (the application name in this case).

if ((e = entry_read(u, name))) {

will loop through these values and apply the last value to the stream to be restored. This causes the volume and mute settings to wander across the restored streams. (See my original example.)

To remedy this situation it is necessary to be able to identify the one and only original entry from which the volume and mute settings will be restored. This can be achieved by setting a unique string for every entry. Because the sink input id itself is unavailable in the property list I fall back on using the pid related to the sink input, as this is a unique identifier, and the only unique identifier in the property list.

It is not necessary to use the pid per se as we are not referencing the application that runs under that pid in any way. All we want to do is to identify the correct entry from which to restore the settings. So we need a unique string, pid or random string, it does not matter. So as a fallback, in case the pid is not available in the property list I use a randomly generated string in pa_proplist_get_stream_id().

So the pid is only used to give the entry a unique id. We do not necessarily have to use the pid, but the string identifying an entry must be unique. Also mpg123 does not die, only the sink input gets remapped. The pid of mpg 123 actually persists, but this is not important for the issue we're dealing with.

What my patch does is to provide a new function pa_proplist_get_stream_id() that uniquely identifies the entry. This way the volume and mute values of the original stream (sink input) can be restored on the new stream.

If you think that this patch interferes with group filter application please explain how that would work. (I understand it would if I had modified pa_proplist_get_stream_group() as I did initially, but the last version of the patch uses a separate function.)

Now in practice the patch works only for the mute value. For the volume it does not, as there is volume remapping and scaling going on that should not happen in the case of stream restoration. But this is another matter to be addressed later. Lets start by making sure that the correct entry is referenced from which the values are restored.

So although not perfect, at least the attached patch fixes the issue I was having where remapped sink inputs suddenly got unmuted. It's a place to start from, not a complete solution.
Comment 14 Leonard den Ottolander 2016-01-25 20:51:09 UTC
> if ((e = entry_read(u, name))) {
> 
> will loop through these values and apply the last value to the stream to be
> restored. This causes the volume and mute settings to wander across the
> restored streams. (See my original example.)

This is incorrect, but the point is that

if ((e = entry_read(u, name))) {

does return one of the multiple entries with this particular name (it seems the last modified entry) which is not necessarily the entry from which the settings should be restored.
Comment 15 Tanu Kaskinen 2016-01-26 10:25:44 UTC
(In reply to Leonard den Ottolander from comment #13)
> I'll try to rephrase.
> 
> I make 2 assumptions:
> 
> 1) module-stream-restore.c is supposed to restore a stream, that is a sink
> input or a source output, with its original volume and mute settings.

A single sink input or source output is an ephemeral object. A sink input gets created once and when it dies, it will never appear again, so there is nothing to restore later. Since there is nothing to restore, I still don't understand what you mean when you are restoring the volume "of a single unique stream".

module-stream-restore defines the grouping by which sink inputs are associated with each other, and then restores the last user-configured volume for all sink inputs in the group. If you use the pid as the group id, all streams created by the same process are grouped together, and if you use a random string, no grouping happens at all, and as a result module-stream-restore doesn't have any effect on anything, and therefore using a random id is equivalent with not having module-stream-restore loaded at all (except that the hard disk fills with ever-growing set of stream entries that are never going to be used again).

> 2) To be able to restore the correct volume setting the entry of the
> original stream needs to be uniquely identified. Currently there is no
> mechanism to uniquely identify such an entry. The entries are labelled with
> non unique labels.
> 
> 
> In the case where we have multiple instances of the same application (name)
> pa_proplist_get_stream_group() will return multiple values because there are
> multiple sink input entries with the same name (the application name in this
> case).

No, there is only one entry per stream group.

> if ((e = entry_read(u, name))) {
> 
> will loop through these values and apply the last value to the stream to be
> restored. This causes the volume and mute settings to wander across the
> restored streams. (See my original example.)

It doesn't "wander" (if by that you mean that the process is somehow undeterministic). The last user-configured volume is consistently restored for new streams.
Comment 16 Leonard den Ottolander 2016-01-26 17:01:03 UTC
> It doesn't "wander" (if by that you mean that the process is somehow
> undeterministic). The last user-configured volume is consistently restored
> for new streams.

It is deterministic in that the volume and mute settings get restored from the last changed entry onto the restored entry (assuming all streams use the same application and hence the same restore id).

This means that existing streams that get restored while running suddenly take on other volumes and mute value of the stream settings of the last changed stream.

So when running three mpg123 streams as in my example, I mute all 3, then unmute 1, once one of the sink inputs gets restored it unmutes and I'm suddenly listening to 2 streams. Are you telling me that is by design?
Comment 17 Tanu Kaskinen 2016-01-26 19:31:02 UTC
(In reply to Leonard den Ottolander from comment #16)
> So when running three mpg123 streams as in my example, I mute all 3, then
> unmute 1, once one of the sink inputs gets restored it unmutes and I'm
> suddenly listening to 2 streams. Are you telling me that is by design?

That's the result of grouping sink inputs by the application. That works in most cases, but doesn't work when you want to use multiple instances of the same application simultaneously, and also want them to have independent volumes. If you have good suggestions for better grouping that servers all use cases, I'd be very glad to hear them. So far you have only proposed grouping by the process, or not grouping at all, which unfortunately are not viable alternatives.

I doubt there is any better default grouping possible. Probably the only possibility is to offer some better tools for configuring the grouping. For example, we could make it possible to say that sink inputs belonging to application X should be grouped by the process, not by the application. So you could enable per-process grouping for mpg123 and still enjoy properly restored volumes for Rhythmbox etc.
Comment 18 Leonard den Ottolander 2016-01-26 21:47:44 UTC
I understand that the group mapping is a useful thing for when an (audio) application is started and its stream is linked to a sink input for the first time. In that case you might want to restore a (group) setting that you used before.

The problem is that streams that automatically get remapped to another sink input - this happens "spontaneously" from the user perspective, probably as a result of a "stuttering" steam - also have these group settings applied. So the remapped stream now suddenly has different mute and volume settings, with a startling result.

So what the software should do is distinguish between these two situations: When a stream is first linked to a sink input (when the stream is created/started) it is fine to apply group settings, but when a stream is merely remapped to the next sink input no changes to volume and mute settings should be made.

I don't claim to have a complete solution or even a partial one, only the suggestion: Make the software distinguish between initial mapping and remapping of input sinks so the restoration module can make a choice whether to apply the group settings or not.

I've only been studying the code since a few days and will certainly not claim to understand all the ins and outs or know how to implement the suggestion I make. It's just that I am looking for a solution to a situation where I suddenly hear (a possibly quite loud) stream coming from my speakers without having touched the volume settings.

I guess for now I have to live with providing a unique module-stream-restore.id via PULSE_PROPS to every stream I start to remedy this situation.
Comment 19 Leonard den Ottolander 2016-01-26 22:08:13 UTC
> If you have good suggestions for better grouping that servers all use cases,
> I'd be very glad to hear them.

The problem is that the settings are available /only/ via the group. You need to be able to distinguish between a "restore group" and a "restore id". So instead of adding only a group label you also need to add a label unique for every sink input. That way you can reapply the settings of the original sink input to the remapped one and still apply group settings to newly created streams when they are initially linked to a sink input.
Comment 20 Tanu Kaskinen 2016-01-27 13:22:34 UTC
(In reply to Leonard den Ottolander from comment #18)
> I understand that the group mapping is a useful thing for when an (audio)
> application is started and its stream is linked to a sink input for the
> first time. In that case you might want to restore a (group) setting that
> you used before.
> 
> The problem is that streams that automatically get remapped to another sink
> input - this happens "spontaneously" from the user perspective, probably as
> a result of a "stuttering" steam - also have these group settings applied.
> So the remapped stream now suddenly has different mute and volume settings,
> with a startling result.
> 
> So what the software should do is distinguish between these two situations:
> When a stream is first linked to a sink input (when the stream is
> created/started) it is fine to apply group settings, but when a stream is
> merely remapped to the next sink input no changes to volume and mute
> settings should be made.

"Stream" as something spanning multiple sink inputs is a concept you have invented, and there's no reliable way to synthesize "streams" when all we have is a series of sink inputs. The best heuristic I can think of is to consider all sink inputs from the same process to be part of the same stream, but that's not perfect. I agree, however, that using that heuristic would be a net benefit compared to the current behaviour, at least as long as the application doesn't create multiple simultaneous sink inputs.

I still think, however, that volume changes to the group volume should update the volume of not only future streams, but all existing streams in that group. That would make the change described above ineffectual. My preferred solution to your problem therefore remains that if you want separate volumes for three mpg123 instances, the streams shouldn't be associated with the "mpg123 group" to begin with. They should be assigned to separate groups by manually setting module-stream-restore.id to a custom value.

> I don't claim to have a complete solution or even a partial one, only the
> suggestion: Make the software distinguish between initial mapping and
> remapping of input sinks so the restoration module can make a choice whether
> to apply the group settings or not.
> 
> I've only been studying the code since a few days and will certainly not
> claim to understand all the ins and outs or know how to implement the
> suggestion I make. It's just that I am looking for a solution to a situation
> where I suddenly hear (a possibly quite loud) stream coming from my speakers
> without having touched the volume settings.
> 
> I guess for now I have to live with providing a unique
> module-stream-restore.id via PULSE_PROPS to every stream I start to remedy
> this situation.

Why do you have three mpg123 instances running at the same time, by the way? If it's some permanent or repeating setup where each instance has its own defined role, you should define a group id for each role instead of generating a brand new id every time you start mpg123. That way the stream-restore database doesn't grow unnecessarily, and each mpg123 instance inherits the role-specific state.
Comment 21 Leonard den Ottolander 2016-01-28 14:10:26 UTC
> "Stream" as something spanning multiple sink inputs is a concept you have
> invented,

Just trying to describe what I see. The mpg123 streams I have running occassionally get remapped to a new sink input (see the sink input ids (first field in the volinlistall output) change in my original example). So it's still the same mpg123 instance (with the same pid - you cannot see this in the output, but the three associated pids remain the same while the sink input ids keep increasing -), now linked to the next sink input. And on the remapping to the next sink input the group volume and mute settings gets applied, thus changing these settings for an already running stream.

So in that sense, yes, the stream is "spanning" multiple sinks, in that it gets remapped to the next and the next etc without user interaction while the mpg123 pids remain the same. How and where exactly in the code this is happening I haven't figured out yet, but it happens.


> and there's no reliable way to synthesize "streams" when all we
> have is a series of sink inputs.

Well that is sort of the point, we can only work on series of sink inputs. A mechanism to distinguish individual sink inputs is required to make it possible to distinguish between actions on sink input groups and individual sinks.

Making available the sink input id and/or source output id in the property list seems like an easy enough approach. I only used the pid in my patch because it was the only property available that distinguishes the sink input uniquely. Using the sink input id directly would of course be much cleaner, but for what I tried to accomplish this would have required a much more intrusive and thus larger patch.


As I tried to explain in my last comments there are two different situations that should be handled differently, but the stream restore module does the same thing in both cases:

1) A stream gets linked to a sink input initially. Applying group settings at this point is fine.

2) A stream gets remapped to the next sink input (this is not a user action but something pulse does on what for lack of a better word I call a "stuttering" stream). In this case we only want to reapply the same volume and mute settings the stream that gets remapped was initially using.

So we need a way to distinguish individual sink inputs and still be able to access sink inputs as a group and the restore stream module needs to be able to distinguish between the initial linking of a stream to a sink input and a "spontaneous" remapping (as in caused by the software, not by user interaction).

Settings for "live" individual sinks of course need not be cached on disk, just to be available in the running application.


> The best heuristic I can think of is to consider all sink inputs from the
> same process to be part of the same stream, but that's not perfect.

Not sure if I understand what you say here. In this particular example there are three /different/ mpg123 processes that all remain running under the same pid. So they are all different processes, unless you mean something else with "process" here...


> I still think, however, that volume changes to the group volume should update
> the volume of not only future streams, but all existing streams in that group.

It shouldn't be impossible to have both individual controls and group controls and a way to let the user add members to a group (being it all applications with the same name or individual (live) sink inputs). The problem is that pulse currently only understands groups and not individual sink inputs and this seems a major drawback to me.


> Why do you have three mpg123 instances running at the same time, by the way?
> If it's some permanent or repeating setup where each instance has its own
> defined role, you should define a group id for each role instead of
> generating a brand new id every time you start mpg123. That way the
> stream-restore database doesn't grow unnecessarily, and each mpg123 instance
> inherits the role-specific state.

Yes, I got this from your earlier explanation. The settings for every group get cached on disk so using unique ids every time just fills up this cache with stale entries.
Comment 22 Tanu Kaskinen 2016-01-28 15:37:46 UTC
(In reply to Leonard den Ottolander from comment #21)
> > "Stream" as something spanning multiple sink inputs is a concept you have
> > invented,
> 
> Just trying to describe what I see. The mpg123 streams I have running
> occassionally get remapped to a new sink input (see the sink input ids
> (first field in the volinlistall output) change in my original example). So
> it's still the same mpg123 instance (with the same pid - you cannot see this
> in the output, but the three associated pids remain the same while the sink
> input ids keep increasing -), now linked to the next sink input. And on the
> remapping to the next sink input the group volume and mute settings gets
> applied, thus changing these settings for an already running stream.
> 
> So in that sense, yes, the stream is "spanning" multiple sinks, in that it
> gets remapped to the next and the next etc without user interaction while
> the mpg123 pids remain the same. How and where exactly in the code this is
> happening I haven't figured out yet, but it happens.

When the old sink input dies and the new one is created, it means that mpg123 closed and reopened the alsa device (the pulse plugin in this case). PulseAudio doesn't destroy/create sink inputs behind mpg123's back.

> > and there's no reliable way to synthesize "streams" when all we
> > have is a series of sink inputs.
> 
> Well that is sort of the point, we can only work on series of sink inputs. A
> mechanism to distinguish individual sink inputs is required to make it
> possible to distinguish between actions on sink input groups and individual
> sinks.
> 
> Making available the sink input id and/or source output id in the property
> list seems like an easy enough approach. I only used the pid in my patch
> because it was the only property available that distinguishes the sink input
> uniquely. Using the sink input id directly would of course be much cleaner,
> but for what I tried to accomplish this would have required a much more
> intrusive and thus larger patch.

Putting the sink input index in the proplist would achieve nothing. When the previous sink input dies and mpg123 creates a new one, the two sink inputs have different indexes, and therefore the index provides no link between the two sink inputs.

> As I tried to explain in my last comments there are two different situations
> that should be handled differently, but the stream restore module does the
> same thing in both cases:
> 
> 1) A stream gets linked to a sink input initially. Applying group settings
> at this point is fine.

As far as pulseaudio is concerned, a stream and a sink input is the same thing. There's no "linking" outside your own mental model that treats the two things as separate concepts.

> 2) A stream gets remapped to the next sink input (this is not a user action
> but something pulse does on what for lack of a better word I call a
> "stuttering" stream). In this case we only want to reapply the same volume
> and mute settings the stream that gets remapped was initially using.
> 
> So we need a way to distinguish individual sink inputs and still be able to
> access sink inputs as a group and the restore stream module needs to be able
> to distinguish between the initial linking of a stream to a sink input and a
> "spontaneous" remapping (as in caused by the software, not by user
> interaction).

There's no way to reliably distinguish "spontaneous remapping" from other kinds of sink input creation. Only heuristics can be used, such as treating all sink inputs from the same process as being part of the same "stream". A reliable mechanism would be to require the application to attach a "stream id" to every sink input it creates, then pulseaudio could compare the stream ids of two sink inputs and treat them as the same stream if the ids match. But no such identifier is provided by any existing application.

> > The best heuristic I can think of is to consider all sink inputs from the
> > same process to be part of the same stream, but that's not perfect.
> 
> Not sure if I understand what you say here. In this particular example there
> are three /different/ mpg123 processes that all remain running under the
> same pid. So they are all different processes, unless you mean something
> else with "process" here...

I use "process" and "pid" in the meaning that the kernel uses those terms. "PID" is short for "process identifier". In that meaning it's not possible to have three simultaneous processes with the same pid, so I wonder what meaning you assign to those terms.
Comment 23 Leonard den Ottolander 2016-01-28 16:05:18 UTC
> When the old sink input dies and the new one is created, it means that mpg123
> closed and reopened the alsa device (the pulse plugin in this case).
> PulseAudio doesn't destroy/create sink inputs behind mpg123's back.

Right... Thanks for clearing that up. This was the essence of my misunderstanding. I thought pulse was responsible for the remapping and hence should be able to track the sink input change.

> There's no way to reliably distinguish "spontaneous remapping" from other
> kinds of sink input creation. Only heuristics can be used, such as treating
> all sink inputs from the same process as being part of the same "stream".

Yes, right.

> I use "process" and "pid" in the meaning that the kernel uses those terms.
> "PID" is short for "process identifier". In that meaning it's not possible
> to have three simultaneous processes with the same pid, so I wonder what
> meaning you assign to those terms.

Never mind, just misreading what you wrote. I read "all sink inputs from the same process" as "all simultaneous sink inputs", as if you were thinking that one mpg123 instance was providing multiple streams, which of course you were not ;) .

So yes, tracking the pids associated with the sink inputs to establish that a sink input has been remapped seems like a usable approach to avoid sudden volume and mute setting changes. That requires a second IDENTIFICATION_PROPERTY that should only be stored internally.
Comment 24 GitLab Migration User 2018-07-30 10:12:38 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/268.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.