When an application sets a device for a newly created stream, we treat
that as a temporary setting, and don't save it as the preferred device
for future streams. The handling for this was broken, however: if the
stream already had a preferred device saved in the stream-restore
database, that was unset.
This was a regression introduced in
bc0e728320 and
70bbbcdc84. These commits tried to detect
in subscribe_callback() when the preferred device is cleared, but as a
side effect the preferred device started to get cleared from the
database also when a stream was created with a device set by the
application.
There's no way for subscribe_callback() to distinguish the different
cases of the preferred device being NULL. This problem is solved by
using the PREFERRED_SINK/SOURCE_CHANGED hooks. The hooks are only called
when the preferred device really changes.
Fixes: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/issues/1063
Part-of: <https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/535>
Newer GCC warns us that the channel_map and volume in legacy entries are
accessed via pointers, and these might be unaligned as the legacy entry
is a packed structure. For this reason, we read out those values into
local variables before accessing them as pointers.
The warnings are:
[146/433] Compiling C object src/modules/module-device-restore.so.p/module-device-restore.c.o
../src/modules/module-device-restore.c: In function ‘legacy_entry_read’:
../src/modules/module-device-restore.c:554:51: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
554 | if (le->volume_valid && !pa_channel_map_valid(&le->channel_map)) {
| ^~~~~~~~~~~~~~~~
../src/modules/module-device-restore.c:559:48: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
559 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
| ^~~~~~~~~~~
../src/modules/module-device-restore.c:559:104: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
559 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
| ^~~~~~~~~~~
../src/modules/module-device-restore.c:559:117: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
559 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
| ^~~~~~~~~~~~~~~~
[211/433] Compiling C object src/modules/module-stream-restore.so.p/module-stream-restore.c.o
../src/modules/module-stream-restore.c: In function ‘legacy_entry_read’:
../src/modules/module-stream-restore.c:1076:51: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
1076 | if (le->volume_valid && !pa_channel_map_valid(&le->channel_map)) {
| ^~~~~~~~~~~~~~~~
../src/modules/module-stream-restore.c:1081:48: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
1081 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
| ^~~~~~~~~~~
../src/modules/module-stream-restore.c:1081:104: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
1081 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
| ^~~~~~~~~~~
../src/modules/module-stream-restore.c:1081:117: warning: taking address of packed member of ‘struct legacy_entry’ may result in an unaligned pointer value [-Waddress-of-packed-member]
1081 | if (le->volume_valid && (!pa_cvolume_valid(&le->volume) || !pa_cvolume_compatible_with_channel_map(&le->volume, &le->channel_map))) {
|
If write_entry fails to store new entry in database, next time we can try creating new entry again.
With DBUS enabled this will create another dbus entry for same name leading to crash inserting duplicate into dbus_entries map.
Fix this by checking if dbus entry exists in dbus_entries map before creating it.
Fixes: #974
Prior to commits f899d5f466 and
f62a49b8cf, GNOME's sound settings
overwrote the routing for all entries in the stream-restore database
when selecting a device. Now we prevent that from happening (see the
aforementioned commits), but the old overwritten settings can still be in
the database after updating to PulseAudio 14.0, and they can cause
problems, as documented here:
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/issues/832
We can't distinguish between devices set by GNOME's sound settings
and devices set by the user, so this patch discards all old device
settings, even though that is going to cause PulseAudio to forget routing
settings for many users. This is less bad than keeping the incorrect
routing settings in the database, because it's difficult for users to
figure out how to fix the situation when e.g. speaker test tones go to
the internal speakers no matter what device is selected as the default,
whereas old manual configuration can be restored restored by doing the
manual configuration again. Also, it's probably more common to have at
some point changed the default device in GNOME's sound settings than it
is to have any manual per-stream routing settings.
This is disabled by default, because this causes data loss, but
distributions that use GNOME are recommended to enable this with
the --enable-stream-restore-clear-old-devices (Autotools) or
-Dstream-restore-clear-old-devices=true (Meson) build option.
Fixes: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/issues/832
pa_namereg_is_valid_name() will hit an assertion if the name string is
NULL. Maybe it would make sense to change pa_namereg_is_valid_name() so
that it would return false on NULL, but I didn't want to change the
function semantics at this time.
e->device and e->card can be NULL even when device_valid and card_valid
are set to true if the database contains bad data.
I ran into this crash while developing new code, I haven't seen the
crash in the wild.
Storing the version in the entry struct is pointless. We should always
write entries using the current version. When we encounter older
versions when reading, those need to be converted to the current version
anyway, because all code that uses the entry struct assumes that the
data is stored according to the current version semantics.
We're currently at the first version of the database entries, so
currently there's no version conversion happening. I have a patch that
will increment the entry version, so this is preparation for that.
When users select an input device from gnome-control-center UI, the
source of this input device will be set to the
configured_default_source and the default_source, these actions are
expected, but after these actions, the gnome-control-center will call
extension_cb() to modify the entries in the database, let all stream
entries to bind the source users select, this is not correct since the
source is default_source now.
This is a temp fix for this issue, after gnome-control-center fixes
this problem, this patch should be reverted.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When a source is unlinked, all streams of this source are moved to
default_source, this action is implemented in the core rather than
modules now.
And after this change, the module-rescue-streams is not needed, but
for backward compatibility, we keep it as a dummy module.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When a new source appears, all streams that have their
preferred_source set to the new source should be moved to the new
source.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When the user moves a stream to the current default source, the
preferred_source should be set to NULL and module-stream-restore
should clear the routing for that stream in the stream database. From
that point on the stream will be always routed to the default source.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
If the source here is NULL, that means users want to clear the
preferred_source and move the source-output to the default_source,
otherwise set the preferred_source to the source->name and move the
source-output to the source. After that fire the source_output_change
event.
After adding this API, we can use this API to simplify the entry_apply
in the module-stream-restore.c.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
The finial objective is to store the preferred source name in the
source-output struct, and use module-stream-restore to save and
restore it.
This patch just replaces the save_source with preferred_source, and
tries to keep the original logic.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When users select an output device from gnome-control-center UI, the
sink of this output device will be set to the configured_default_sink
and the default_sink, these actions are expected, but after these
actions, the gnome-control-center will call extension_cb() to modify
the entries in the database, let all stream entries to bind the sink
users select, this is not correct since the sink is default_sink now.
This is a temp fix for this issue, after gnome-control-center fixes
this problem, this patch should be reverted.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When a sink is unlinked, all streams of this sink are moved to
default_sink, this action is implemented in the core rather than
modules now.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When a new sink appears, all streams that have their preferred_sink
set to the new sink should be moved to the new sink.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
When the user moves a stream to the current default sink, the
preferred_sink should be set to NULL and module-stream-restore
should clear the routing for that stream in the stream database. From
that point on the stream will be always routed to the default sink.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
If the sink here is NULL, that means users want to clear the
preferred_sink and move the sink-input to the default_sink, otherwise
set the preferred_sink to the sink->name and move the sink-input to
the sink. After that fire the sink_input_change event.
After adding this API, we can use this API to simplify the entry_apply
in the module-stream-restore.c.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
The finial objective is to store the preferred sink name in the
sink-input struct, and use module-stream-restore to save and restore
it.
This patch just replaces the save_sink with preferred_sink, and tries
to keep the original logic.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
We met two problems recently, one happened on a Lenovo machine with
dual analogue codecs, the other happened on a Dell machine with
a digital mic directly connected to PCH. The two problems are
basically same, there is an internal mic and an external mic, the
internal mic always shows up in the gnome-control-center, the external
mic only shows up when it is plugged. After the external mic is
plugged and users select it from gnome-control-center, the
gnome-control-center will read all saved streams through extension_cb,
and bind the source of external mic to all streams, after that the
apps only record sound via the source of external mic, after the
external mic is unplugged, the internal mic will automatically be
selected since it is the only left input device in the
gnome-control-center, since users don't select it, all streams are
still bond the source of external mic. When users record sound via
apps, they can't record any sound even the default_source is the
source of internal mic and the internal mic is selected in the UI.
It is very common that a machine has internal mic and external mic,
but this problem didn't expose before, that is because both internal
mic and external mic belong to one source, but for those two
machines, the internal mic belongs to one source, while the external
mic belongs to another source (they are in differnt codecs or one is
in the codec and the other is from PCH),
To fix it with a mininal change, we just check if the active_port is
PA_AVAILABLE_NO or not when building a new stream, if it is, don't
restore the device to the new built stream, let pa_source_output_new()
decide the source device for this stream.
And we also do the same change to sink_input.
This change only affects the new built streams, it will not change
the database, so the users' preference is still saved in the database,
after the active_port is not PA_AVAILABLE_NO, the new streams will
still restore to the preferred device.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
pa_sink_get_state() and pa_source_get_state() just return the state
variable. We can as well access the state variable directly.
There are no behaviour changes, except that module-virtual-source
accessed the main thread's sink state variable from its push() callback.
I fixed the module so that it uses the thread_info.state variable
instead. Also, the compiler started to complain about comparing a sink
state variable to a source state enum value in protocol-esound.c. The
underlying bug was that a source pointer was assigned to a variable
whose type was a sink pointer (somehow using the pa_source_get_state()
macro confused the compiler enough so that it didn't complain before).
I fixed the variable type.
pa_sink_input_get_state() and pa_source_output_get_state() just return
the state variable. We can as well access the state variable directly.
There are no behaviour changes, except that some filter sources accessed
the main thread's state variable from their push() callbacks. I fixed
them so that they use the thread_info.state variable instead.
This removes the symdef header generation m4 magic in favour of a
simpler macro method, allowing us to skip one unnecessary build step
while moving to meson, and removing an 11 year old todo!
When a stream is created, and the stream creator specifies which device
should be used, that can affect automatic routing policies.
Specifically, module-device-manager shouldn't apply its priority list
routing when a stream has been routed by the application that created
the stream.
A stream that was initially routed by the application may be moved for
some valid reason (e.g. user requesting a move, or the original device
disappearing). When the stream is moved away from its initial device,
the "device requested by application" flag isn't relevant any more, so
it's set to false and never reset to true again.
The change in module-device-manager's routing logic will be done in the
following patch.
module-stream-restore primarily uses the role of a stream for restoring. The sink-inputs
and source-outputs of filters all have role "filter", therefore currently all filters are
treated equally and are restored to the same device and volume.
This patch lets module-stream-restore ignore the streams that connect the filter to the
master.
Bug link: https://bugs.freedesktop.org/show_bug.cgi?id=100065
If first part of test is false and e->device is NULL pa_streq will
segfault. Fix by using pa_safe_streq, which checks strings for NULL
before doing strcmp.
modules/module-stream-restore.c: In function 'clean_up_db':
modules/module-stream-restore.c:2344:74: warning: comparison of constant '0' with boolean expression is always true [-Wbool-compare]
pa_assert_se(entry_write(u, item->entry_name, item->entry, true) >= 0);
reported by Ubuntu gcc-6
Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
pa_tagstruct_new() is called either with no data, i.e. (NULL, 0)
to create a dynamic tagstruct or with a pointer to fixed data
introduce a new function pa_tagstruct_new_fixed() for the latter case
Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
FSF addresses used in PA sources are no longer valid and rpmlint
generates numerous warnings during packaging because of this.
This patch changes all FSF addresses to FSF web page according to
the GPL how-to: https://www.gnu.org/licenses/gpl-howto.en.html
Done automatically by sed-ing through sources.
It seems at some point the code migrated to use the entry_write calls,
but fill_db is still using the old syntax, causing the entry to be
invalid.
The crash happens when clean_up_db gets called, which then calls
entry_read, causing the crash.
Signed-off-by: Ricardo Salveti de Araujo <ricardo.salveti@canonical.com>
Since the hashmap stores a pointer to the key provided at pa_hashmap_put()
time, it make sense to allow the hashmap to be given ownership of the key and
have it free it at pa_hashmap_remove/free time.
To do this cleanly, we now provide the key and value free functions at hashmap
creation time with a pa_hashmap_new_full. With this, we do away with the free
function that was provided at remove/free time for freeing the value.
All pa_cvolume_snprint(), pa_volume_snprint(),
pa_sw_cvolume_snprint_dB() and pa_sw_volume_snprint_dB() calls have
been replaced with pa_cvolume_snprint_verbose() and
pa_volume_snprint_verbose() calls, making the log output more
informative and the code sometimes simpler.
This patch removes all occurrences of double and triple
newlines.
Command used for this:
find . -type d \( -name ffmpeg \) -prune -o \
-regex '\(.*\.[hc]\|.*\.cc\)' \
-a -not -name 'adrian-aec.*' -a -not \
-name reserve.c -a -not -name 'rtkit.*' \
-exec sed -i -e '/^$/{N;s/^\n$//}' {} \;
Two passes were needed to remove triple newlines.
The excluded files are mirrored files from external sources.
The previous patch removed module-gconf's dependency on the userdata
pointer of the free callback, and that was the only place where the
userdata pointer of pa_free2_cb_t was used, so now there's no need for
pa_free2_cb_t in pa_hashmap_free(). Using pa_free_cb_t instead allows
removing a significant amount of repetitive code.
Stream-restore DBus API method argument list is missing last boolean
argument apply_immediately, causing assert to fail in AddEntry handling.
Signed-off-by: Juho Hämäläinen <jusa@hilvi.org>
