[pulseaudio-discuss] [PATCH v3 18/24] echo-cancel: Improve webrtc canceller error handling a bit

[Arun Raghavan]

On 9 February 2016 at 23:41, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
>
>> From: Arun Raghavan <git at arunraghavan.net>
>>
>> ---
>>  src/modules/echo-cancel/webrtc.cc | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-cancel/webrtc.cc
>> index 2732b38..5741f45 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -284,7 +284,10 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>>          webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* reverse input stream */
>>          webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* reverse output stream */
>>      };
>> -    apm->Initialize(pconfig);
>> +    if (apm->Initialize(pconfig) != webrtc::AudioProcessing::kNoError) {
>> +        pa_log("Error initialising audio processing module");
>> +        goto fail;
>> +    }
>>
>>      if (hpf)
>>          apm->high_pass_filter()->Enable(true);
>> @@ -313,7 +316,8 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>>              ec->params.webrtc.agc = false;
>>          } else {
>>              apm->gain_control()->set_mode(webrtc::GainControl::kAdaptiveAnalog);
>> -            if (apm->gain_control()->set_analog_level_limits(0, WEBRTC_AGC_MAX_VOLUME) != apm->kNoError) {
>> +            if (apm->gain_control()->set_analog_level_limits(0, WEBRTC_AGC_MAX_VOLUME) !=
>> +                    webrtc::AudioProcessing::kNoError) {
>>                  pa_log("Failed to initialise AGC");
>>                  goto fail;
>>              }
>> @@ -361,7 +365,8 @@ void pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t *play) {
>>      pa_assert(play_frame.samples_per_channel_ <= webrtc::AudioFrame::kMaxDataSizeSamples);
>>      memcpy(play_frame.data_, play, ec->params.webrtc.blocksize * pa_frame_size(ss));
>>
>> -    apm->ProcessReverseStream(&play_frame);
>> +    if (apm->ProcessReverseStream(&play_frame) != webrtc::AudioProcessing::kNoError)
>> +        pa_log("Failed to process playback stream");
>>  }
>>
>>  void pa_webrtc_ec_record(pa_echo_canceller *ec, const uint8_t *rec, uint8_t *out) {
>> @@ -387,7 +392,8 @@ void pa_webrtc_ec_record(pa_echo_canceller *ec, const uint8_t *rec, uint8_t *out
>>      }
>>
>>      apm->set_stream_delay_ms(0);
>> -    apm->ProcessStream(&out_frame);
>> +    if (apm->ProcessStream(&out_frame) != webrtc::AudioProcessing::kNoError)
>> +        pa_log("Failed to process capture stream");
>
> I don't like error messages that can spam the syslog. Would a hard
> failure be acceptable here (that is, unload the module, or change to a
> failure state where the module stays around but does no processing)?

This particular case is extremely low probability, so I'd like to not
change it if possible. I don't think the extra code to add a noop mode
is justified since we're very unlikely to hit this.

Maybe we could/should change the run/play/record vmethods to return an
error code, but I'd prefer to do that separately if it's done.

-- Arun