On Thu, 9 Mar 2017, at 09:53 PM, Georg Chini wrote:
> On 09.03.2017 17:14, Georg Chini wrote:
> > On 09.03.2017 16:33, Arun Raghavan wrote:
> >>
> >> On Thu, 9 Mar 2017, at 03:27 PM, Georg Chini wrote:
> >>> On 09.03.2017 05:37, Arun Raghavan wrote:
> >>>> 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.
> >>>> ---
> >>>> src/modules/echo-cancel/module-echo-cancel.c | 7 ++++++-
> >>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/src/modules/echo-cancel/module-echo-cancel.c
> >>>> b/src/modules/echo-cancel/module-echo-cancel.c
> >>>> index dfd05b6..ed75e0c 100644
> >>>> --- a/src/modules/echo-cancel/module-echo-cancel.c
> >>>> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> >>>> @@ -683,8 +683,13 @@ static void do_resync(struct userdata *u) {
> >>>> pa_log("Doing resync");
> >>>> /* update our snapshot */
> >>>> - source_output_snapshot_within_thread(u, &latency_snapshot);
> >>>> + /* 1. Get sink input latency snapshot, might cause buffers to
> >>>> be sent to source thread */
> >>>> pa_asyncmsgq_send(u->sink_input->sink->asyncmsgq,
> >>>> PA_MSGOBJECT(u->sink_input), SINK_INPUT_MESSAGE_LATENCY_SNAPSHOT,
> >>>> &latency_snapshot, 0, NULL);
> >>>> + /* 2. Pick up any in-flight buffers (and discard if needed) */
> >>>> + while (pa_asyncmsgq_process_one(u->asyncmsgq))
> >>>> + ;
> >>>> + /* 3. Now get the source output latency snapshot */
> >>>> + source_output_snapshot_within_thread(u, &latency_snapshot);
> >>>> /* calculate drift between capture and playback */
> >>>> diff_time = calc_diff(u, &latency_snapshot);
> >>> While taking a look at the patch I noticed something else in the
> >>> do_resync().
> >>> You are doing:
> >>>
> >>> source_output_snapshot_within_thread(u, &latency_snapshot);
> >>> pa_asyncmsgq_send(u->sink_input->sink->asyncmsgq,
> >>> PA_MSGOBJECT(u->sink_input), SINK_INPUT_MESSAGE_LATENCY_SNAPSHOT,
> >>> &latency_snapshot, 0, NULL);
> >>>
> >>> from the source thread. I tried something similar in module loopback
> >>> and
> >>> found that you should not send a message to the sink thread from there.
> >>>
> >>> At least for bluetooth it looks like input and output is done in the
> >>> same thread,
> >>> so the pa_asyncmsg_send() will hang. I tested it with my headset and it
> >>> hangs
> >>> indeed.
> >> Interesting. Do you mean for HSP, or HFP, or ...?
> >>
> >> -- Arun
> >
> > HSP, native backend. As I said, I had the same problem in
> > module-loopback.
> This was the command I issued:
>
> pacmd load-module module-echo-cancel source_name=echo_cancel_source
> sink_name=echo_cancel_sink aec_method=webrtc use_volume_sharing=false
> adjust_time=2 sink_master=bluez_sink.0C_E0_E4_31_23_2D.headset_head_unit
> source_master=bluez_source.0C_E0_E4_31_23_2D.headset_head_unit
>
> After that you had to kill pulseaudio with kill -9.
So you're right of course. However, the problem itself predates the
patch, so I'd like to decouple the review of this with fixing the issues
of sink and source being on the same thread.
I'm not sure what we should be doing there. Maybe a check to compare
asyncmsgq of the sink input with pa_thread_mq_get(), and if they're the
same, call an _in_thread() version of get sink latency snapshot.
-- Arun
