pa_ncpu() is supposed to report the number of processors available on
the system. For that, it currently calls sysconf(_SC_NPROCESSORS_CONF).
However, since the operating system can disable individual processors,
we should call sysconf(_SC_NPROCESSORS_ONLN) to determine the number
of processors currently available [1]. Consequently, the once-test will
fail since pthread_setaffinity_np() is called with CPUs that are
currently not available.
It might also be advisable to change the code in the future to use CPU
sets on Linux as even the suggested change is not 100% safe but at least
it improves over the existing code. If PulseAudio was to be run in a CPU
set [2], the number of processors available to PulseAudio could be even
less than the number of CPUs currently online (_SC_NPROCESSORS_CONF).
[1] https://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html
[2] http://man7.org/linux/man-pages/man7/cpuset.7.html
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96809
Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Doesn't really affect logic, but Coverity reports this as dead-code, and
I figure it makes sense to be consistent about our use of HAVE_MEMFD.
CID: 1352045
It was a very confusing state variable that required a lot of
fiddling. It was also redundant in that it can be computed from
the other variables, removing any risk of it getting out of sync.
In the same spirit, make sure "requested" also always contains a
sane value, even though it may not be used by every caller.
Having it handled in the callers proved to be a poor fit as it
became difficult to handle a shrinking minreq sanely. It could end
up in a state where the request was never sent downstream to the
client.
The behaviour is to leave the value unchanged. The idea is to init the value
with a default before the call and not treat a missing value as error. That
way, only parsing errors or validating errors actually return error codes.
I want module-alsa-card to set the availability of unavailable
profiles before the initial card profile gets selected, so that the
selection logic can use correct availability information.
module-alsa-card initializes the jack state after calling
pa_card_new(), however, and the profile selection happens in
pa_card_new(). This patch solves that by moving parts of pa_card_new()
to pa_card_choose_initial_profile() and pa_card_put().
pa_card_choose_initial_profile() applies the profile selection policy,
so module-alsa-card can first call pa_card_new(), then initialize the
jack state, and then call pa_card_choose_initial_profile(). After that
module-alsa-card can still override the profile selection policy, in
case module-alsa-card was loaded with the "profile" argument. Finally,
pa_card_put() finalizes the card creation.
An alternative solution would have been to move the jack
initialization to happen before pa_card_new() and use pa_card_new_data
instead of pa_card in the jack initialization code, but I disliked
that idea (I want to get rid of the "new data" pattern eventually).
The order in which the initial profile policy is applied is reversed
in this patch. Previously the first one to set it won, now the last
one to set it wins. I think this is better, because if you have N
parties that want to set the profile, we avoid checking N times
whether someone else has already set the profile.
There is currently no use for allowing modules to cancel card creation,
and I don't see need for that in the future either. Let's simplify
things by removing the failure handling code.
This allows us to parse an extra set of modargs to tack on to an
existing set. Duplicates in the second set are ignored, since this fits
our use best. In the future, this could be extended to support different
merge modes (ignore dupes vs. replace with dupes), but I've left this
out since there isn't a clear need and it would be dead code for now.
Renamed all variables pertaining to latency offsets of sinks and sources,
calling them "port_latency_offset" or similar instead. All of these variables
refer to latency offsets inherited from ports, rather than being unique to
the sinks or sources themselves.
This change is to pave the way for additional functionality for setting
latency offsets on sources and sinks independenly from the value they inherit
from their port. In order to implement them we first need this rename so that
the two latency offsets can be stored individually and summed when reporting
the total latency of the source or sink.
The renames made are:
pa_sink_set_latency_offset() -> pa_sink_set_port_latency_offset()
pa_source_set_latency_offset() -> pa_source_set_port_latency_offset()
sink->latency_offset -> sink->port_latency_offset
sink->thread_info.latency_offset -> sink->thread_info.port_latency_offset
source->latency_offset -> source->port_latency_offset
source->thread_info.latency_offset -> source->thread_info.port_latency_offset
PA_SINK_MESSAGE_SET_LATENCY_OFFSET -> PA_SINK_MESSAGE_SET_PORT_LATENCY_OFFSET
PA_SOURCE_MESSAGE_SET_LATENCY_OFFSET -> PA_SOURCE_MESSAGE_SET_PORT_LATENCY_OFFSET
Unlike pa_sink_set_port(), which calls pa_sink_set_latency_offset() to update
the latency offset of the sink to match that of its newly set port,
pa_source_set_port() did not do so. This patch adds the appropriate call to
pa_source_set_latency_offset() in pa_source_set_port() to fix this.
Sink(-input) and source(-output) called unlink function when reference
count dropped to zero. This would result in unlink hooks being called
with an object having a reference count of zero, and this is not a
situation we want modules to have to deal with. It is better to just
remove the redundant unlinking code from sink(-input) and
source(-output) and assert on reference count in unlink functions as well.
It is expected that in well behaving code the owner of an object will
always unlink the object before unreferencing.
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
While investigating bug 95352, I noticed that
pa_pstream_set_revoke_callback() and pa_pstream_set_release_callback()
were identical - both set the release callback.
pa_pstream_set_revoke_callback() was obviously broken - it was setting
the wrong callback.
The only place where set_revoke_callback() is called is in
protocol-native.c. The code there looks like this:
pa_pstream_set_revoke_callback(c->pstream, pstream_revoke_callback, c);
pa_pstream_set_release_callback(c->pstream, pstream_release_callback, c);
Since set_release_callback() is called last, the release callback gets
set correctly. The only problem is that the revoke callback stays
unset. What are the consequences of that? The code that calls the
revoke callback looks like this:
if (p->revoke_callback)
p->revoke_callback(p, block_id, p->revoke_callback_userdata);
else
pa_pstream_send_revoke(p, block_id);
So the intended callback is replaced with a pa_pstream_send_revoke()
call. What does the intended callback, that doesn't get called, do?
if (!(q = pa_thread_mq_get()))
pa_pstream_send_revoke(p, block_id);
else
pa_asyncmsgq_post(q->outq, PA_MSGOBJECT(userdata), CONNECTION_MESSAGE_REVOKE, PA_UINT_TO_PTR(block_id), 0, NULL, NULL);
So the native protocol's revoke callback is anyway going to call
pa_pstream_send_revoke() when called from the main thread. If the
revoking is done from an IO thread, an asynchronous message is sent to
the main thread instead, and the message handler will then call
pa_pstream_send_revoke().
In conclusion, the only effect of this bug was that
pa_pstream_send_revoke() was sometimes being called from an IO thread
when it should have been called from the main thread. I don't know if
this caused the crash in bug 95352. Probably not.
As reported by valrgrind
==30002== Conditional jump or move depends on uninitialised value(s)
==30002== at 0x5CB883C: pa_cmsg_ancil_data_close_fds (pstream.c:193)
==30002== by 0x5CBB161: do_write (pstream.c:759)
==30002== by 0x5CB8B51: do_pstream_read_write (pstream.c:233)
==30002== by 0x5CB8EE8: io_callback (pstream.c:279)
...
The pa_cmsg_ancil_data structure has two main guards:
'creds_valid', which implies that it holds credentials
information, and 'nfd', which implies it holds file descriptors.
When code paths create a credentials ancillary data structure,
they just set the 'nfd' guard to zero. Typically, the rest of
pa_cmsg_ancil_data fields related to fds are _all_ left
_uninitialized_.
pa_cmsg_ancil_data_close_fds() has broken the above contract:
it accesses the new 'close_fds_on_cleanup' flag, which is related
to file descriptors, without checking the 'nfd == 0' guard first.
Fix this inconsistency.
Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
As shown by valgrind
==10615== Conditional jump or move depends on uninitialised value(s)
==10615== at 0x5CC0483: shm_marker_size (shm.c:97)
==10615== by 0x5CC1685: shm_attach (shm.c:381)
==10615== by 0x5CC1990: pa_shm_cleanup (shm.c:453)
==10615== by 0x5CC068E: sharedmem_create (shm.c:150)
...
Solution is to fix the shm_marker_size() signature itself: At
certain code paths like shm_attach(), we don't want to initialize
_any_ field in the passed SHM segment descriptor except after
making sure all error exit conditions have been passed.
Reported-by: Alexander E. Patrakov <patrakov@gmail.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
Fix memory leak in pa_resampler_new() in resampler.c, Deallocating
memory of r->lfe_filter in case of fail.
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
The current LFE crossover filter removes low frequencies from the main
channels and puts them into the LFE channel with the wrong amplitude.
It is not known for sure what is the correct relative amplitude (acoustic
measurements are required with real hardware), and changing that might
introduce a new bug, "it clips the LFE channel".
So just disable the feature by default until a better understanding
emerges how it should work. This, essentially, returns the defaults
to their state as of PulseAudio 6.0.
Some more observations:
- Most of available active analog speakers on the market do the
necessary crossover filtering already, and HDMI receivers can be
configured to do that, too, so a crossover filter in PulseAudio is
harmful in these use cases.
- The "laptop with a builtin subwoofer" use case requires manual
configuration anyway because the default crossover frequency (120 Hz) is
wrong for laptop speakers.
- Finally, Windows 10 with a built-in USB audio driver does not synthesize
the LFE channel given a 5.1 card and a stereo audio stream by default.
Hides: https://bugs.freedesktop.org/show_bug.cgi?id=95021
Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
ffmpeg_data was not freed properly before return due to error.
It is now freed properly.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=95347
Signed-off-by: Sachin Kumar Chauhan <sachin.kc@samsung.com>
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
This is needed so we don't keep stale jack availability information
while the card is suspended.
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=93259
Signed-off-by: Arun Raghavan <arun@arunraghavan.net>
I will modify module-switch-on-port-available so that it will keep
track of which input and output port the user prefers on the card,
based on the user's profile and port switches. The preference needs
to be saved on disk, for which I will use module-card-restore.
To facilitate communication between the two modules, this patch adds
preferred_input_port and preferred_output_port fields to pa_card, and
a hook for monitoring the variable changes. It would be nice if the
two modules would communicate directly with each other, but
implementing that would be somewhat complicated, so I chose this time
for adding the functionality to the core. In theory some other routing
module might want to manage the new variables instead of
module-switch-on-port-available, but admittedly that's not very likely
to happen...
Now that all layers in the stack support memfd blocks, add memfd
support for the daemon's global core mempool. Also introduce
"enable-memfd=" daemon argument and configuration option.
For now, memfd support is an opt-in feature to be activated only
when daemon's enable-memfd= is set to yes.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Now that all layers in the stack support memfd blocks, add memfd
pools support for client context and audio playback data.
Use such memfd pools by default only if the server signals memfd
support in its connection negotiations.
Also add ability for clients to force-disable memfd transport
through the `enable-memfd=' client configuration option.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
pa_sink_input_update_proplist() is inconvenient in many cases, because
it requires allocating a new proplist, even if the goal is to just set
one property. pa_sink_input_update_properties also can't properly log
property changes, because it has to assume that all values are
arbitrary binary data.
This patch adds pa_sink_input_set_property() for setting a string
value for a single property, and pa_sink_input_set_property_arbitrary()
for setting a binary value for a single property.
pa_sink_input_update_properties() is reimplemented as a wrapper around
pa_sink_input_set_property_arbitrary() to centralize logging and
sending change notifications.
(The above mentions only sink input functions for brevity, but the
same changes are implemented for source outputs too.)
Before a device is unlinked, the unlink hook is fired, and it's
possible that a routing module tries to move streams to the unlinked
device in that hook, because it doesn't know that the device is being
unlinked. Of course, the unlinking is obvious when the code is in an
unlink hook callback, but it's possible that some other module does
something in the unlink hook that in turn triggers some other hook,
and it's this second hook where the routing module may get confused.
This patch adds an "unlink_requested" flag that is set before the
unlink hook is fired, and moving streams to a device with that flag
set is prevented.
This patch is motivated by seeing module-device-manager moving a
stream to a sink that was being unlinked. It was a complex case where
an alsa card was changing its profile, while an echo-cancel sink was
connected to the old alsa sink. module-always-sink loaded a null sink
in the middle of the profile change, and after a stream had been
rescued to the null sink, module-device-manager decided to move it
back to the old alsa sink that was being unlinked. That move made no
sense, so I came up with this patch.
srbchannel needs fd passing. Otherwise we get the following error
for systems without SCM_CREDENTIALS support:
Code should not be reached at pulsecore/pstream-util.c:95,
function pa_pstream_send_tagstruct_with_fds(). Aborting.
[[ The root cause is that we define HAVE_CREDS only if
SCM_CREDENTIALS is defined, but SCM_CREDENTIALS is a Linux-specific
symbol. Thus HAVE_CREDS is always disabled on Solaris.
And since pulse couples the non-portable creds passing support
with the portable fd passing one, through _35_ places where
HAVE_CREDS is used, a real fix needs a PA redesign -- assuming that
latency on Solaris is something people care about. ]]
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94339
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Now that we have the necessary infrastructure to memexport and
mempimport a memfd memblock, extend that support higher up in the
chain with pstreams.
A PA endpoint can now _transparently_ send a memfd memblock to the
other end by simply calling pa_pstream_send_memblock() – provided
the block's memfd pool was earlier registered with the pstream.
If the pipe does not support memfd transfers, we fall back to
sending the block's full data instead of just its reference.
** Further details:
A single pstream connection usually transfers blocks from multiple
pools including the server's srbchannel mempool, the client's
audio data mempool, and the server's global core mempool.
If these mempools are memfd-backed, we now require registering
them with the pstream before sending any blocks they cover. This
is done to minimize fd passing overhead and avoid fd leaks.
Moreover, to support all these pools without hard-coding their
number or nature in the Pulse communication protocol itself, a new
REGISTER_MEMFD_SHMID command is introduced. That command can be
sent _anytime_ during the pstream's lifetime and is used for
creating on demand SHM ID to memfd mappings.
Suggested-by: David Henningsson <david.henningsson@canonical.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Color global mempools with a special mark. This special marking
is needed for handling memfd-backed pools.
To avoid fd leaks, memfd pools are registered with the connection
pstream to create an ID<->memfd mapping on both PA endpoints.
Such memory regions are then always referenced by their IDs and
never by their fds, and so their fds can be safely closed later.
Unfortunately this scheme cannot work with global pools since the
registration ID<->memfd mechanism needs to happen for each newly
connected client, and thus the need for a more special handling.
That is, for the pool's fd to be always open :-(
Almost all mempools are now created on a per-client basis. The
only exception is the pa_core's mempool which is still shared
between all clients of the system.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
To transfer memfd-backed blocks without passing their fd every time,
thus minimizing overhead and avoiding fd leaks, a command is sent
with the memfd fd as ancil data very early on.
This command has an ID that uniquely identifies the memfd region.
Further memfd block references are then exclusively done using this
ID.
This commit implements the details of such 'permanent' mappings on
the receiving end, using memimport segments.
Suggested-by: David Henningsson <david.henningsson@canonical.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Memfd is a simple memory sharing mechanism, added by the systemd/kdbus
developers, to share pages between processes in an anonymous, no global
registry needed, no mount-point required, relatively secure, manner.
This patch introduces the necessary building blocks for using memfd
shared memory transfers in PulseAudio.
Memfd support shall also help us in laying out the necessary (but not
yet sufficient) groundwork for application sandboxing, protecting PA
from its clients, and protecting clients data from each other.
We plan to exclusively use memfds, instead of POSIX SHM, on the way
forward.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
pa_shm_create_rw() is responsible for creating two types of memory:
POSIX shared memory and regular malloc()-ed ones.
A third memory type, memfds, will be added later. Thus to add this
extra shared memory type in a sane manner, refactor private memory
allocations into their own static methods.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
Soon we're going to have three types of memory pools: POSIX shm_open()
pools, memfd memfd_create() ones, and privately malloc()-ed pools.
Thus introduce annotations for the memory types supported and change
pa_mempool_new() into a factory method based on required memory.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
The PA daemon currently uses a single SHM file for all clients
sending and receiving commands over the low-latency srbchannel
mechanism.
To avoid leaks between clients in that case, and to provide the
necessary ground work later for sandboxing and memfds, create the
srbchannel SHM files on a per-client basis.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
In future commits, server-wide SHMs will be replaced with per-client
ones that will be dynamically created and freed according to clients
connections open and close.
Meanwhile, current PA design does not guarantee that the per-client
mempool blocks are referenced only by client-specific objects.
Thus reference count the pools and let each memblock inside the pool
itself, or just attached to it, increment the pool's refcount upon
allocation. This way, per-client mempools will only be freed when no
further component in the system holds any references to its blocks.
DiscussionLink: https://goo.gl/qesVMV
Suggested-by: Tanu Kaskinen <tanuk@iki.fi>
Suggested-by: David Henningsson <david.henningsson@canonical.com>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
The daemon `shm-size-bytes' configuration value was read, and then
directly used, for creating the initial server-wide SHM files.
This is fine for now, but soon, such server-wide SHMs will be replaced
with per-client SHM files that will be dynamically created and deleted
according to clients open and close. Thus, appropriately cache this
configuration value.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
sd_journal_send() implicitly add fields for the source file,
function name and code line from where it's invoked. As code
location fields CODE_FILE, CODE_LINE and CODE_FUNC are handled
by PA's log module, we do not want the automatic values
supplied by the sd_journal API.
Without suppressing these, both the actual log event source
and the call to sd_journal_send() will be logged:
$ journalctl -b -f -o json-pretty
[...]
CODE_FILE : [ pulsecore/log.c, pulsecore/module.c ],
CODE_LINE : [ 505, 181 ],
MESSAGE : Failed to load module module-gconf
CODE_FUNC : [ pa_log_levelv_meta, pa_module_load ],
[...]
(Commit log adapted from abrt libreport commit d1eaae97f0287f)
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
In file included from pulse/timeval.c:32:0:
pulse/timeval.c: In function 'pa_timeval_add':
./pulsecore/macro.h:303:28: warning: left shift of negative value [-Wshift-negative-value]
? ~(~(type) 0 << (8*sizeof(type)-1))
reported by Ubuntu gcc-6
gcc-6 adds -Wshift-negative-value (enabled by -Wextra) which warns
about left shifting a negative value. Such shifts are undefined
because they depend on the representation of negative values.
also works with -Wshift-overflow=2
Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
This gets rid of an error message from the debug log. If
volume_factor_source would actually be used somewhere, this bug would
have caused more severe problems.
volume_factor_source should have the source's channel map. When moving
the stream, the volume needs to be remapped from the old source's
channel map to the new source's map. However, when the stream is being
moved, there is a period where the old source has already been
forgotten and the new source isn't yet known, so the remapping can't
be done directly between the two channel maps. Instead, the volume is
remapped from the old source's map to the stream's own map when the
move starts, and again remapped from the stream's map to the new
source's map when the move finishes.
The first remapping was missing, causing the second remapping fail and
print an error to the log.
(I checked the sink input code as well. It didn't have this bug.)
