1268675 - Audio dropouts from mic on Plantronics 628 headset

Status: RESOLVED DUPLICATE of bug 1269692

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dropouts.wav

After some period of time (minutes) with a Plantronics 628 headset on Windows, I start seeing dropouts in the mic stream (full-duplex - Nightly 4/28).  It will appear, stay for some time (minutes), then disappear again.

Capturing an AEC log (note: use a non-e10s window for the getUserMedia test and about:webrtc or you'll crash) shows periodic 10ms dropouts (all 0's).

I dont' see this with a Logitech 540 headset.

The Plantronic (unlike the Logitech) appears to be fixed at stereo, 44100Hz.  The Logitech can do 48KHz, as well as 44100, 32000, etc.

Comment 1

[Maire Reavy [:mreavy]]

Paul, Can you take this one or are you swamped?

Comment 2

[Paul Adenot (:padenot)]

I can take it, I'm asking if the Paris office has a pair locally, otherwise I'll order one.

Comment 3

[Paul Adenot (:padenot)]

Our backend, in duplex mode, taking the numeric values from my testing with a plantronics 648 that exhibits the same issue, goes like this:

- Get notified by the OS that we need to refill
- Get all the input frames we can (usually, this is 970, occasionally, when the sound is bad, this is 441)
- Query the number of available output buffer space in the output buffer (where we'd write the data to output). This is always 970, because we're output driven.
- Notice we're missing input frames, insert some silence

What we could do instead:

- Get all the input frames we can, return the value we got
- Query the number of available output buffer space in the output buffer, but only _request_ a number of frames equal to the number of input frames returned above.

This means we'll probably get called a bit more often, but at this point, we'll have more input data, so that's fine. 

Also another thing we could do is to buffer the input in a queue, instead of ignoring the input available events and only pull from the output.

Comment 4

[Paul Adenot (:padenot)]

Attachment: patch

This is kind of experimental, but I think it fixes the issue. Of course it's a bit hard to tell because the glitches happen after 4-5 min or so, but with this patch, I don't hear glitches anymore.

Perceptual loop-back latency (using the gUM page) does not change much it seems.

Basically, we try to re-sync the two stream by returning early and buffering the input. We know a stream is late because the number of available frames is null. If that's the case, either we've grabbed some input, and we use the input next time, letting it sit in the buffer, or we don't have anything and we just return.

In any case, we're releasing any output buffer space we've claimed, so that we get called back properly. On the next callback, we have enough input and output buffer space to satisfy the request, so we proceed as usual.

If that's not enough, there are more thing we can do, but this patch is simple enough I think.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8747719 [details] [diff] [review]
patch

Review of attachment 8747719 [details] [diff] [review]:
-----------------------------------------------------------------

drive-bay.  Looks pretty good to me; question is the caller's reaction to this, which apparently is good.

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ -46,5 @@
>  #ifndef PKEY_Device_InstanceId
>  DEFINE_PROPERTYKEY(PKEY_Device_InstanceId,      0x78c34fc8, 0x104a, 0x4aca, 0x9e, 0xa4, 0x52, 0x4d, 0x52, 0x99, 0x6e, 0x57, 0x00000100); //    VT_LPWSTR
>  #endif
>  
> -// #define LOGGING_ENABLED

Obviously don't commit this

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8747719 [details] [diff] [review]
patch

Review of attachment 8747719 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +645,5 @@
>      }
>      offset += packet_size * input_channel_count;
>    }
>  
> +  assert(stm->linear_input_buffer.length() >= total_available_input);

This makes the assert less useful because it's verifying a different property of the code now.

@@ +708,5 @@
>      return rv;
>    }
>  
> +  input_frames = stm->linear_input_buffer.length() / stm->input_stream_params.channels;
> +

No newline.

@@ +785,5 @@
>    size_t output_frames;
>  
>    XASSERT(!has_input(stm) && has_output(stm));
>  
> +  rv = get_output_buffer(stm, output_frames, output_buffer, output_frames);

output_frames is uninitialized here, so the behaviour of

    frame_count = std::min(max_frames, stm->output_buffer_frame_count - padding_out);

in get_output_buffer is going to be undefined.

Comment 7

[Marcia Knous [:marcia]]

Tracking for 49 so as to not lose track of it.

Comment 8

[Maire Reavy [:mreavy]]

Fixed by Bug 1269692