The suspend cause isn't yet used by any of the callbacks. The alsa sink
and source will use it to sync the mixer when the SESSION suspend cause
is removed. Currently the syncing is done in pa_sink/source_suspend(),
and I want to change that, because pa_sink/source_suspend() shouldn't
have any alsa specific code.
There are no behaviour changes, the code from almost all the SET_STATE
handlers is moved with minimal changes to the newly introduced
set_state_in_io_thread() callback. The only exception is module-tunnel,
which has to call pa_sink_render() after pa_sink.thread_info.state has
been updated. The set_state_in_io_thread() callback is called before
updating that variable, so moving the SET_STATE handler code to the
callback isn't possible.
The purpose of this change is to make it easier to get state change
handling right in modules. Hooking to the SET_STATE messages in modules
required care in calling pa_sink/source_process_msg() at the right time
(or not calling it at all, as was the case on resume failures), and
there were a few bugs (fixed before this patch). Now the core takes care
of ordering things correctly.
Another motivation for this change is that there was some talk about
adding a suspend_cause variable to pa_sink/source.thread_info. The
variable would be updated in the core SET_STATE handler, but that would
not work with the old design, because in case of resume failures modules
didn't call the core message handler.
The suspend cause isn't yet used by any of the callbacks. The alsa sink
and source will use it to sync the mixer when the SESSION suspend cause
is removed. Currently the syncing is done in pa_sink/source_suspend(),
and I want to change that, because pa_sink/source_suspend() shouldn't
have any alsa specific code.
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.
When a filter is loaded and module-switch-on-connect is present, switch-on-connect
will make the filter the default sink or source and move streams from the old
default to the filter. This is done from the sink/source put hook, therefore streams
are moved to the filter before the module init function of the filter calls
sink_input_put() or source_output_put(). The move succeeds because the asyncmsq
already points to the queue of the master sink or source. When the master sink or
source is attached to the sink input or source output, the attach callback will call
pa_{sink,source}_attach_within_thread(). These functions assume that all streams
are detached. Because streams were already moved to the filter by switch-on-connect,
this assumption leads to an assertion in pa_{sink_input,source_output}_attach().
This patch fixes the problem by reverting the order of the pa_{sink,source}_put()
calls and the pa_{sink_input,source_output}_put calls and creating the sink input
or source output corked. The initial rewind that is done for the master sink is
moved to the sink message handler. The order of the unlink calls is swapped as well
to prevent that the filter appears to be moving during module unload.
The patch also seems to improve user experience, the move of a stream to the filter
sink is now done without any audible interruption on my system.
The patch is only tested for module-echo-cancel.
Bug-Link: https://bugs.freedesktop.org/show_bug.cgi?id=100065
When module-echo-cancel is loaded and there is only one sound card, then during a
profile switch, all sinks and sources can become temporarily unavailable. If
module-always sink is loaded, it will load a null-sink in that situation. If
also module-switch-on-connect is loaded, it will try to move the sink-inputs to
the new null-sink. If a sink-input was connected to the echo-cancel sink,
pa_sink_input_start_move() will send a PA_SINK_GET_LATENCY message to the
echo-cancel sink. The message handler will then in turn call
pa_sink_get_latency_within_thread() for the invalid master sink of module-echo-cancel.
This lead to a segfault.
This patch checks in the message handler if the master sink (or source) is valid and
returns 0 if not.
The reported latency of source or sink is based on measured initial conditions.
If the conditions contain an error, the estimated latency values may become negative.
This does not indicate that the latency is indeed negative but can be considered
merely an offset error. The current get_latency_in_thread() calls and the
implementations of the PA_{SINK,SOURCE}_MESSAGE_GET_LATENCY messages truncate negative
latencies because they do not make sense from a physical point of view. In fact,
the values are truncated twice, once in the message handler and a second time in
the pa_{source,sink}_get_latency_within_thread() call itself.
This leads to two problems for the latency controller within module-loopback:
- Truncating leads to discontinuities in the latency reports which then trigger
unwanted end to end latency corrections.
- If a large negative port latency offsets is set, the reported latency is always 0,
making it impossible to control the end to end latency at all.
This patch is a pre-condition for solving these problems.
It adds a new flag to pa_{sink,source}_get_latency_within_thread() to allow
negative return values. Truncating is also removed in all implementations of the
PA_{SINK,SOURCE}_MESSAGE_GET_LATENCY message handlers. The allow_negative flag
is set to false for all calls of pa_{sink,source}_get_latency_within_thread()
except when used within PA_{SINK,SOURCE}_MESSAGE_GET_LATENCY. This means that the
original behavior is not altered in most cases. Only if a positive latency offset
is set and the message returns a negative value, the reported latency is smaller
because the values are not truncated twice.
Additionally let PA_SOURCE_MESSAGE_GET_LATENCY return -pa_sink_get_latency_within_thread()
for monitor sources because the source gets the data before it is played.
We don't always know whether the in-flight memory chunks will be
rendered or skipped (if the source is not in RUNNING). This can cause us
to have an erroneous estimate of drift, particularly when the canceller
starts.
To avoid this, we explicitly flush out the send and receive sides of the
message queue of audio chunks going from the sink to the source before
trying to perform a resync.
If pa_sink_input_cork() or pa_source_output_cork() were called without a sink
or source attached, the calls would crash pulseaudio.
This patch fixes the problem, so that a source output or sink input can still
be corked or uncorked while source or sink are invalid. This is needed to
correct the corking logic in module-loopback.
The webrtc canceller seems to have changed to require that the
set_stream_drift_samples() method be called before every call of
ProcessStream().
So we now call ec->set_stream_drift_samples() before calling
ec->record() by:
1. Always calling do_push_drift_comp() instead of only when the sink is
running
2. Calling set_stream_drift_samples() in the loop with record() instead
of outside
We do kind of leak this quirk of the webrtc canceller into the generic
bits of module-echo-cancel, but this should not be harmful in the
general case either.
On systems with constrained CPUs, we might run into a situation where
the master source/sink is configured to have too high a latency.
On the source side, this would cause us to wake up with a large chunk of
data to process, which might cause us to exhust our RT limit and thus be
killed.
So it makes sense to limit the overall latency that we request from the
source (and correspondingly, the sink, so we don't starve for playback
data on the source side).
The 10 blocks maximum is somewhat arbitrary (I'm assuming the system has
enough headroom to process 10 chunks through the canceller without
getting close to the RT limit). This might make sense to make tunable in
the future.
Bug 96741 shows a case where an assertion is hit, because
pa_asyncq_new() failed due to running out of file descriptors.
pa_asyncq_new() is used in only one place (not counting the call in
asyncq-test): pa_asyncmsgq_new(). Now pa_asyncmsgq_new() can fail too,
which requires error handling in many places. One of those places is
pa_thread_mq_init(), which can now fail too, and that needs additional
error handling in many more places. Luckily there weren't any places
where adding better error handling wouldn't have been easy, so there are
many changes in this patch, but they are not complicated.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96741
When autoloaded, module-echo-cancel doesn't support moving the sink
input and source output that it creates, but the move prevention was
implemented by manually requesting module unloading in the middle of
the stream move procedure, rather than by just setting the DONT_MOVE
flags. This patch removes the module unloading code from the moving()
callbacks and adds the DONT_MOVE flags. In addition to saving some
code, this also prevents problems related to trying to move streams
connected to the echo cancel sink or source while the echo cancel sink
or source is in the middle of a move too (a crash will happen in such
situation, as demonstrated in
https://bugs.freedesktop.org/show_bug.cgi?id=93443).
It is expected that the underlying AGC mechanism will likely provide a
single volume for the source rather than a per-channel volume. Dealing
with per-channel volumes just adds complexity with regards to the
actual volume setting (depending on whether volume sharing is enabled or
not, we would set the volume on the source output of the virtual source,
and their sample specs may be different).
Using a single volume allows us to sidestep this problem entirely.
This is required to have unequal channel counts on capture in and out
streams, which is needed for beamforming to work. The deinterleaved API
only works with floating point samples.
The calculations around how many samples were sent to the canceller
engine was not updated when we started supporting different channel
counts for playback and capture.
The AGC code no longer seems to honour the analog volume limits we set,
and internally uses 0-255 as the volume range. So we switch to use that
(keeping the old API usage as is in case this gets fixed upstream).
This allows us to inherit the sample spec parameters from the sink and
source master (rather than forcing 32 kHz / mono). It is still possible
to override some of the parameters for the source side with modargs.
My original testing showed that these parameters provided a decent
perf/quality trade-off on lower end hardware (which I no longer have
access to). I figure it makes sense to continue with that for now, and
in the future this can be relaxed (use_master_format=yes could be the
default, and resource-constrained systems can disable it).
In the refactoring, I'm expressing the constraints in what I see to be a
more natural way -- rec_ss expresses what we're feeding the canceller,
so it makes sense to apply the constraints on what the canceller accepts
there. This then propagates to the output spec.
This also exposes the range of sample rates that the library actually
supports (8, 16, 32 and 48 kHz).
The original intention was to configure low enough parameters to keep
CPU consumption down. Prior to this change, we assumed that the EC
backend would override the sink parameters based on the source
parameters to achieve this goal, and with this change we remove that
assumption by forcing the default parameters for the sink to be low
enough.
It's not possible to enable the intelligibility enhancer at the
moment, because the feature would require modifying the audio that we
play to speakers, which we don't do currently. All audio processing is
done at the source side, and it's not easy to change that.
This patch is based on Arun Raghavan's code, I just reordered things
a bit and reworded the FIXME comment.
This creates a longer filter that is more complex and less sensitive to
incorrect delay reporting from the hardware. There is also a
delay-agnostic mode that can eventually be enabled if required.
In some very quick testing, not enabling this seems to provide better
results during double-talk.
This forces the canceller engine to be invoked even if playback is not
currently active. We need to do this for cases where the engine provides
additional processing that is independent of playback, such as noise
suppression and AGC.
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83557
Adding AGC broke this test, so we hard-disable the volume code in test
mode. This is probably okay for now, since at least with analog AGC, the
source volume changes and the data we get is going to be with AGC
applied, but digital gain won't be encapsulated here.
Long term, we might need to figure out how to deal with this properly.
When we the underlying sink/source goes away, there is an intermediate
state where the asyncmsgqs that we were using for the sink-input and
source-output go away. This is usually okay if the sink-input and
source-output are moved to another device, but can be problematic if we
don't support moving (which is the case when the filter is autoloaded).
This becomes a problem because of the following chain of events:
* The underlying sink goes away
* Moving the filter sink-input fails (because it is autloaded)
* At this point the sink-input has no underlying sink, and thus
no underlying asyncmsgq
* This also applies to all sink-inputs connected to the echo-cancel
module
* The sink-input is killed, triggering a module unload
* On unlink, module-rescue-streams tries to move sink-inputs to
another sink, starting with a START_MOVE message
* There is no asyncmsgq for the message, so we crash
* We can't just perform a NULL check for the asyncmsgq, since there
are state changes we need to effect during the move
To fix this, we pretend to allow the move to the new sink, and then
unlink ourselves *after* the move is complete. This ensures that we
never find ourselves in a position where we need the underlying
sink/asyncmsgq to be present when it is not.
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=90416
