We will be promoting M57 to Stable soon. Will this be a blocker for M57 Stable release? If so, please apply "ReleaseBlock-Stable" label.

 Issue 696846  has been merged into this issue.

+tlegrand, can someone in your team look at this?

I listened to the recording, and the clicks are present all the time, but much louder when the person on the other end is talking. When he mutes, they are completely gone.

Niklas, what codec is used? Can you get an RTCeventlog as well?

Fredrik, do you know of anything in current Beta that could explain this?

Opus is used, will try to get a log.

Event log attached, and here's the corresponding AEC dump: https://drive.google.com/a/google.com/file/d/0B4bp6vyHtaURNmIzcmhXbEpwVkE/view?usp=sharing 

It's possible to repro this locally if you need to, contact me to get Webex credentials.

Deleted: event_log.22.10 480 KB

event_log.22.10 480 KB Download

Attaching an rtp file that should show the problem. I decoded using an old version of audiotool, and then I didn't get any clicks.

Deleted: out2.rtp 0 bytes

out2.rtp 0 bytes Download

When I listen to this I get the sense that it's a processing issue. Looking closely at the speaker waveform I can reliably spot sudden jumps, as well as discontinuities that are 44 samples long (sample rate is 44.1 kHz) - perhaps with the waveform continuing right after.

Which Chrome rev is the latest that doesn't exhibit the behavior?
Has Opus been updated for M57?

Assigning to Henrik for input on other things that may have changed in the processing chain.

Chrome 56 is fine.

Actually the earlier rtp file is pretty noisy, this one is better. I built neteq_rtp_play from ToT and the clicks are not present there(!). You can hear a small glitch but I think that's packet loss.

Deleted: out_new.rtp 85.6 KB

out_new.rtp 85.6 KB Download

aleloi@, can you try to reproduce this locally on your Linux machine?

Reproduced on local linux machines.

Me and ivoc@ tried to debug this. Here are some partial results:

1. Lots of peer connections created from the thinclient page. We can
   only do event logs for the first 5. As a consequence, there is no
   audio in the log. We patched a chrome and uploaded attached event
   log with audio. The event log in #7 doesn't have audio.
2. Several PCs are active, but only one downloads something.
3. There is one send and one receive stream for every PC, but only one
   send stream for one of the PCs has graphs updating in real time. The 
   receive stream for the PC that receives data looks inactive.
3. The SSRC that shows up in WireShark doesn't show up in
   the list of PCs.
4. The AudioMixer in WebRTC contains 6 sources (that's strange, there's 
   typically only one in our apps)

Updated event log with audio
https://drive.google.com/open?id=0B0ohpnzd-DTsMnZ6NGZMVkMzZ2s

Lots of peer connections:
https://drive.google.com/open?id=0B0ohpnzd-DTsTm43U3FmWmRRdDA

Alex, you saw that I attached an rtp dump right? Doesn't that tell us something?

Re #14: I wanted an RtcEventLog since it also reveals the timing of the audio output thread (calls to NetEq::GetAudio).

+olka@ - Do you know if any Chrome change for M57 could be causing this? Note that the glitches are visible in the aecdump, so this is before renderer->browser IPC. Rebuffering?

Re #16: since it shows up in aecdump, it should be before Chrome audio rendering pipeline kicks in, right? 

Before IPC yes, but we still do rebuffering around the APM since it consumes 10ms buffers.

If nothing has changed with regards to rebuffering/fifoing between Chrome's APM and WebRTC, it seems the most likely components to look at are (in decending order):

1. The new AudioMixer
2. AudioTransportProxy (which does resampling)
3. Handling of the new APM modules.

Alex appears the best person to dig in - let me know if you need any help!

Alex is currently bisecting Chrome revisions to pinpoint where the regression happened. Bumping prio to 1.

I've done some bisecting: it works fine in Google Chrome 57.0.2926.0 beta but not in Google Chrome 57.0.2929.0 beta. That's a range of 3 days, during which ~600 commits were made. One of them happens to have been made by me turning the new mixer on. That one also includes the AudioTransportProxy change (#2 in the list by solenberg@). So some part of those two is likely the culpit...

I'll try to debug it tomorrow.

Solved, I think. By solved I mean 'found the cause'. We will still have to decide what we have to do about this.

The attached diagrams shows what streams are mixed, and what their energy is. Filled square means mixed, triangle shape means not. The colors are the same between the two pictures. Only the green stream has any energy, but the purple one wakes up to non-muted state 2-3 times per second.

I extracted the actual data for the individual streams and listened to it. There were no clicks. Then I listened to the output of the mixer. The clicks were there.

The problem probably lies with the limiter used in FrameCombiner. It operates in different modes when there are different numbers of mixed streams. When zero to one streams are present, it outputs zeroes or the audio from the single stream. When there are two or more streams, it passes them to a limiter to avoid clipping.

If the number of streams alternates between <=1 and >=2, the limiter will see a patchy view of the data. It seems it can't handle that and introduces glitches.

The change from earlier is something that was meant as an optimization: a stream is only mixed if it's not 'muted'. Previously IIRC, the limiter only got involved when the number of streams was >= 1, now it's the number of un-muted streams.

Possible ways to quickly fix it is:

1. remove the limiter all together. There is a AudioMixerImpl::Create method that takes a 'use_limiter' argument which could be set to false. 
2. Do not change between <= 1 and >= 2 muted streams
3. Change the mixer code to go back to the old behavior.

FrameCombiner:
 https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_mixer/frame_combiner.h?q=FrameCombiner+package:%5Echromium$&l=22

Deleted: mixed_streams_energy.png 103 KB

	mixed_streams_energy.png 103 KB View Download

Deleted: what_streams_are_mixed.png 91.6 KB

	what_streams_are_mixed.png 91.6 KB View Download

Sorry, my diagrams were from different recordings. The colors don't match. Here is the right one.

Deleted: mixed_streams.png 79.8 KB

	mixed_streams.png 79.8 KB View Download

Good find!

Option #1 - If use_limiter=false, we're just adding the streams together right? So we may have some clipping. Perhaps this is the best short term fix. hlundin@ implied that the sonic artifacts might be tolerable. Certainly better than clicks anyway.

Then, in the long term, what about
Option #4 - Write a new simple limiter which is always applied with >1 stream. Works on floats, handles N channels, sample rate agnostic. Given #1 is ok quality wise we won't even have to oversample.

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/2c9306ed50190a60be4dfa037d7c8781b0004a60

commit 2c9306ed50190a60be4dfa037d7c8781b0004a60
Author: aleloi <aleloi@webrtc.org>
Date: Wed Mar 29 11:25:16 2017

Send data from mixer to APM limiter more often.

Before this change, the APM limiter used in FrameCombiner (a
sub-component of AudioMixer) only gets to process the data when the
number of non-muted streams is >1. If this number varies between <=1
and >1, the limiter's view of the data will have gaps during the
periods with <= 1 active stream.

This leads to discontinuities in the applied gain. These
discontinuities cause clicks in the output audio. This change
activates APM limiter processing based on the number of audio streams,
independently of their mutedness status.

BUG= chromium:695993 

Review-Url: https://codereview.webrtc.org/2776113002
Cr-Commit-Position: refs/heads/master@{#17442}

[modify] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/BUILD.gn
[modify] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/audio_mixer_impl.cc
[modify] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/frame_combiner.cc
[modify] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/frame_combiner.h
[modify] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/frame_combiner_unittest.cc
[add] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/gain_change_calculator.cc
[add] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/gain_change_calculator.h
[add] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/sine_wave_generator.cc
[add] https://crrev.com/2c9306ed50190a60be4dfa037d7c8781b0004a60/webrtc/modules/audio_mixer/sine_wave_generator.h

Alex, can you verify Canary tmrw morning and request a merge to 58?

Just had a meeting with Cisco and was able to confirm that latest canary solves the problem.

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099

commit c4a49c0af18b5014bb4439c2e2c70bf5c76ea099
Author: Niklas Enbom <niklas.enbom@webrtc.org>
Date: Thu Mar 30 16:06:33 2017

Send data from mixer to APM limiter more often.

Before this change, the APM limiter used in FrameCombiner (a
sub-component of AudioMixer) only gets to process the data when the
number of non-muted streams is >1. If this number varies between <=1
and >1, the limiter's view of the data will have gaps during the
periods with <= 1 active stream.

This leads to discontinuities in the applied gain. These
discontinuities cause clicks in the output audio. This change
activates APM limiter processing based on the number of audio streams,
independently of their mutedness status.

BUG= chromium:695993 
TBR=aleloi@webrtc.org

Review-Url: https://codereview.webrtc.org/2776113002
Cr-Original-Commit-Position: refs/heads/master@{#17442}
Review-Url: https://codereview.webrtc.org/2788683002 .
Cr-Commit-Position: refs/branch-heads/58@{#11}
Cr-Branched-From: f31969a584bcafe9406c214a9d4c3afb49d19650-refs/heads/master@{#16937}

[modify] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/BUILD.gn
[modify] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/audio_mixer_impl.cc
[modify] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/frame_combiner.cc
[modify] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/frame_combiner.h
[modify] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/frame_combiner_unittest.cc
[add] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/gain_change_calculator.cc
[add] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/gain_change_calculator.h
[add] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/sine_wave_generator.cc
[add] https://crrev.com/c4a49c0af18b5014bb4439c2e2c70bf5c76ea099/webrtc/modules/audio_mixer/sine_wave_generator.h

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

What's the current status here?