[pulseaudio-discuss] [PATCH] echo-cancel: Limit the maximum sink/source latency

[Georg Chini]

On 09.03.2017 16:35, Arun Raghavan wrote:
>
> On Thu, 9 Mar 2017, at 04:01 PM, Georg Chini wrote:
>> On 09.03.2017 06:53, Arun Raghavan wrote:
>>> 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.
>>> ---
>>>    src/modules/echo-cancel/module-echo-cancel.c | 32 +++++++++++++++++++++-------
>>>    1 file changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
>>> index ed75e0c..0a9f290 100644
>>> --- a/src/modules/echo-cancel/module-echo-cancel.c
>>> +++ b/src/modules/echo-cancel/module-echo-cancel.c
>>> @@ -145,6 +145,8 @@ static const pa_echo_canceller ec_table[] = {
>>>    
>>>    #define MEMBLOCKQ_MAXLENGTH (16*1024*1024)
>>>    
>>> +#define MAX_LATENCY_BLOCKS 10
>>> +
>>>    /* Can only be used in main context */
>>>    #define IS_ACTIVE(u) ((pa_source_get_state((u)->source) == PA_SOURCE_RUNNING) && \
>>>                          (pa_sink_get_state((u)->sink) == PA_SINK_RUNNING))
>>> @@ -515,6 +517,7 @@ static int sink_set_state_cb(pa_sink *s, pa_sink_state_t state) {
>>>    /* Called from source I/O thread context */
>>>    static void source_update_requested_latency_cb(pa_source *s) {
>>>        struct userdata *u;
>>> +    pa_usec_t latency;
>>>    
>>>        pa_source_assert_ref(s);
>>>        pa_assert_se(u = s->userdata);
>>> @@ -525,15 +528,17 @@ static void source_update_requested_latency_cb(pa_source *s) {
>>>    
>>>        pa_log_debug("Source update requested latency");
>>>    
>>> -    /* Just hand this one over to the master source */
>>> -    pa_source_output_set_requested_latency_within_thread(
>>> -            u->source_output,
>>> -            pa_source_get_requested_latency_within_thread(s));
>>> +    /* Cap the maximum latency so we don't have to process too large chunks */
>>> +    latency = PA_MIN(pa_source_get_requested_latency_within_thread(s),
>>> +                     pa_bytes_to_usec(u->source_blocksize, &s->sample_spec) * MAX_LATENCY_BLOCKS);
>>> +
>>> +    pa_source_output_set_requested_latency_within_thread(u->source_output, latency);
>>>    }
>>>    
>>>    /* Called from sink I/O thread context */
>>>    static void sink_update_requested_latency_cb(pa_sink *s) {
>>>        struct userdata *u;
>>> +    pa_usec_t latency;
>>>    
>>>        pa_sink_assert_ref(s);
>>>        pa_assert_se(u = s->userdata);
>>> @@ -544,10 +549,11 @@ static void sink_update_requested_latency_cb(pa_sink *s) {
>>>    
>>>        pa_log_debug("Sink update requested latency");
>>>    
>>> -    /* Just hand this one over to the master sink */
>>> -    pa_sink_input_set_requested_latency_within_thread(
>>> -            u->sink_input,
>>> -            pa_sink_get_requested_latency_within_thread(s));
>>> +    /* Cap the maximum latency so we don't have to process too large chunks */
>>> +    latency = PA_MIN(pa_sink_get_requested_latency_within_thread(s),
>>> +                     pa_bytes_to_usec(u->sink_blocksize, &s->sample_spec) * MAX_LATENCY_BLOCKS);
>>> +
>>> +    pa_sink_input_set_requested_latency_within_thread(u->sink_input, latency);
>>>    }
>>>    
>>>    /* Called from sink I/O thread context */
>>> @@ -1658,6 +1664,7 @@ int pa__init(pa_module*m) {
>>>        uint32_t temp;
>>>        uint32_t nframes = 0;
>>>        bool use_master_format;
>>> +    pa_usec_t blocksize_usec;
>>>    
>>>        pa_assert(m);
>>>    
>>> @@ -2020,6 +2027,15 @@ int pa__init(pa_module*m) {
>>>    
>>>        u->thread_info.current_volume = u->source->reference_volume;
>>>    
>>> +    /* We don't want to deal with too many chunks at a time */
>>> +    blocksize_usec = pa_bytes_to_usec(u->source_blocksize, &u->source->sample_spec);
>>> +    pa_source_set_latency_range(u->source, blocksize_usec, blocksize_usec * MAX_LATENCY_BLOCKS);
>>> +    pa_source_output_set_requested_latency(u->source_output, blocksize_usec * MAX_LATENCY_BLOCKS);
>>> +
>>> +    blocksize_usec = pa_bytes_to_usec(u->sink_blocksize, &u->sink->sample_spec);
>>> +    pa_sink_set_latency_range(u->sink, blocksize_usec, blocksize_usec * MAX_LATENCY_BLOCKS);
>>> +    pa_sink_input_set_requested_latency(u->sink_input, blocksize_usec * MAX_LATENCY_BLOCKS);
>>> +
>>>        pa_sink_put(u->sink);
>>>        pa_source_put(u->source);
>>>    
>> You should save the initial latency ranges and restore them when the
>> module is unloaded.
> The latency range is set on the echo-cancel sink and source (not the
> master), so when the module is unloaded, that sink and source go away as
> well.
>
> -- Arun

Sorry, looks good then.