Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid DTX when Silk sees activity #84

Conversation

gustafullberg
Copy link
Contributor

@gustafullberg gustafullberg commented Mar 27, 2018

This PR fixes an issue with periodic clicks or noise-bursts during DTX that we have seen in Chrome, AppRTC and other WebRTC clients.

Sometimes, Opus (in silk-only or hybrid mode) decides to go into DTX even though Silk considers the signal to contain activity. This is because the decide_dtx_mode has a different VAD than Silk.

In DTX, Opus sends an encoded packet every 420 ms. This is in order to give an updated estimation of the background noise. The decoder uses comfort noise (CNG) to produce output during the DTX period.

However, when the silk layer of the "update packets" that arrive every 420 ms contain activity (type TYPE_UNVOICED or TYPE_VOICED instead of TYPE_NO_VOICE_ACTIVITY) the decoder will instead use packet loss concealment (PLC) to produce output instead of CNG. The PLC is then faded out as CNG ramps up.

In cases when several update packets contain activity, the switching back and forth between PLC and CNG will cause an audible click or noise-burst every 420 ms.

This PR fixes the issue by preventing Opus from entering DTX in hybrid or silk-only mode whenever silk considers the signal to be active. Celt-only mode is unaffected.

@gustafullberg
Copy link
Contributor Author

Spectrograms illustrating the issue:

dtx_clicks

@jmvalin
Copy link
Member

jmvalin commented Apr 9, 2018

Unless I missed something (which is quite possible), it seems like the opposite approach would make more sense. By that I mean forcing SILK to consider the signal as inactive when DTX got enabled by Opus. This would have two advantages:

  1. AFAIK the Opus VAD is more reliable than the SILK one
  2. It would ensure the DTX decision is the same in all modes, whereas your patch would make SILK use DTX less than CELT (since it can turn DTX off)
    Does that make sense or are there other reasons you're making the change the other way?

@gustafullberg
Copy link
Contributor Author

That's a good point. I think that could be the right way of solving this.

I created #87 that solves the problem in the way you mention. If your prefer that solution, that PR could replace this one.

The new PR touches a few more lines but is still pretty clean.
This PR is a bit more conservative when it comes to DTX in non-stationary noise.

@jmvalin
Copy link
Member

jmvalin commented May 25, 2018

merged the improved patch

@jmvalin jmvalin closed this May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants