Prevent Opus DTX from generating intermittent noise during silence

webrtc/modules/audio_coding/codecs/opus/opus_interface.c

Index: webrtc/modules/audio_coding/codecs/opus/opus_interface.c
diff --git a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
index 1a632422c5e150facf1ceb4d072b2df70ba3fd4c..9eee89f1327fc0e841d2448ff288ac4deae19aee 100644
--- a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
+++ b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
@@ -11,6 +11,7 @@
 #include "webrtc/modules/audio_coding/codecs/opus/include/opus_interface.h"
 #include "webrtc/modules/audio_coding/codecs/opus/opus_inst.h"
 
+#include <assert.h>

[the sun, 2015/11/09 12:41:00]

Why is this a .c file? Can we make it a .cc and use RTC_DCHECK() instead of
assert?

[minyue-webrtc, 2015/11/09 15:13:00]

Good idea. but it might be a outreach for this CL.

 #include <stdlib.h>
 #include <string.h>
 
@@ -29,48 +30,61 @@ enum {
 
   /* Default frame size, 20 ms @ 48 kHz, in samples (for one channel). */
   kWebRtcOpusDefaultFrameSize = 960,
+
+  // Maximum number of consecutive zeros, beyond or equal to which DTX can fail.
+  kZeroBreakCount = 157,
+
+#if defined(OPUS_FIXED_POINT)
+  kZeroBreakValue = 10,
+#else
+  kZeroBreakValue = 1,
+#endif
 };
 
 int16_t WebRtcOpus_EncoderCreate(OpusEncInst** inst,
                                  int32_t channels,
                                  int32_t application) {
-  OpusEncInst* state;
-  if (inst != NULL) {
-    state = (OpusEncInst*) calloc(1, sizeof(OpusEncInst));
-    if (state) {
-      int opus_app;
-      switch (application) {
-        case 0: {
-          opus_app = OPUS_APPLICATION_VOIP;
-          break;
-        }
-        case 1: {
-          opus_app = OPUS_APPLICATION_AUDIO;
-          break;
-        }
-        default: {
-          free(state);
-          return -1;
-        }
-      }
+  int opus_app;
+  if (!inst)
+    return -1;
 
-      int error;
-      state->encoder = opus_encoder_create(48000, channels, opus_app,
-                                           &error);
-      state->in_dtx_mode = 0;
-      if (error == OPUS_OK && state->encoder != NULL) {
-        *inst = state;
-        return 0;
-      }
-      free(state);
-    }
+  switch (application) {
+    case 0:
+      opus_app = OPUS_APPLICATION_VOIP;
+      break;
+    case 1:
+      opus_app = OPUS_APPLICATION_AUDIO;
+      break;
+    default:
+      return -1;
   }
-  return -1;
+
+  OpusEncInst* state = calloc(1, sizeof(OpusEncInst));
+  assert(state);

[the sun, 2015/11/09 12:41:00]

This should really be an RTC_CHECK() as it is a possible run-time error.

[minyue-webrtc, 2015/11/09 15:13:00]

c file made this a limit. may consider in a separate CL I think

+
+  // Allocate zero counters.
+  state->zero_counts = calloc(channels, sizeof(size_t));
+  assert(state->zero_counts);
+
+  int error;
+  state->encoder = opus_encoder_create(48000, channels, opus_app,
+                                       &error);
+  if (error != OPUS_OK || !state->encoder) {
+    WebRtcOpus_EncoderFree(state);
+    return -1;
+  }
+
+  state->in_dtx_mode = 0;
+  state->channels = channels;
+
+  *inst = state;
+  return 0;
 }
 
 int16_t WebRtcOpus_EncoderFree(OpusEncInst* inst) {
   if (inst) {
     opus_encoder_destroy(inst->encoder);
+    free(inst->zero_counts);
     free(inst);
     return 0;
   } else {
@@ -84,13 +98,42 @@ int WebRtcOpus_Encode(OpusEncInst* inst,
                       size_t length_encoded_buffer,
                       uint8_t* encoded) {
   int res;
+  size_t i;

[the sun, 2015/11/09 12:41:00]

declare when these are used; even C99 supports that.

[minyue-webrtc, 2015/11/09 15:13:00]

Karl wanted this be pre-C99 compatible.

But, true, ironically, we have define-before-use here and there anyway.

I do not mind moving them back to line 116. But Karl, WDYT?

[kwiberg-webrtc, 2015/11/09 19:15:05]

It's not that I *want* C89, it's just that the last time I did nontrivial work
in our C code, some bots (I don't recall which, but I'll hazard a guess and say
Windows...) were still not accepting C99. If it works now, then great.

+  int c;
+
+  int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs];

[the sun, 2015/11/09 12:41:00]

what if inst->channels > 2 ?

[minyue-webrtc, 2015/11/09 15:13:00]

Emm, we really wanted this be statically allocated to by most efficient.

[the sun, 2015/11/09 15:29:56]

The problem is if channels > 2 we will get buffer overwrite below.

[minyue-webrtc, 2015/11/09 16:03:02]

Yes, it can in principle happen, but the creation of an encoder should have
failed if there are more than 2 channels.

 
   if (samples > 48 * kWebRtcOpusMaxEncodeFrameSizeMs) {
     return -1;
   }
 
+  const int channels = inst->channels;
+  int use_buffer = 0;
+
+  // Break long consecutive zeros by forcing a "1" every |kZeroBreakCount|
+  // samples.
+  if (inst->in_dtx_mode) {
+    for (i = 0; i < samples; ++i) {
+      for (c = 0; c < channels; ++c) {
+        if (audio_in[i * channels + c] == 0) {
+          ++inst->zero_counts[c];
+          if (inst->zero_counts[c] == kZeroBreakCount) {
+            if (!use_buffer) {
+              memcpy(buffer, audio_in, samples * channels * sizeof(int16_t));

[the sun, 2015/11/09 12:41:00]

this looks like it will break if the count is reached more than once, i.e.:

...
check
check
reached zerobreakcount for c=0; copy audio_in -> buffer; set zerobreakvalue in
buffer
...
check
check
reached zerobreakcount for c=1 (or again for c=0); copy audio_in -> buffer
(overwriting); set zerobreakvalue in buffer

can you add a unit test for this case?

[minyue-webrtc, 2015/11/09 15:13:00]

Doesn't use_buffer prevent multi-entry? Regarding unittest, the current test
OpusTest.OpusDtxOn already covers this. The problem you proposed will change the
frequency of 1s, and fail that test. I just tried removing line 123, and that
failed.

[the sun, 2015/11/09 15:29:56]

Ah, my bad! You're quite right, should work like it is implemented.

+              use_buffer = 1;
+            }
+            buffer[i * channels + c] = kZeroBreakValue;
+            inst->zero_counts[c] = 0;
+          }
+        } else {
+          inst->zero_counts[c] = 0;
+        }
+      }
+    }
+  }
+
   res = opus_encode(inst->encoder,
-                    (const opus_int16*)audio_in,
+                    use_buffer ? buffer : audio_in,
                     (int)samples,
                     encoded,
                     (opus_int32)length_encoded_buffer);