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 2ac53736650907ca4d54cfb1e1ccc42646f1116a..f1dfe214b632df65daf1952e898e21b5737432c0 100644
--- a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
+++ b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
@@ -29,48 +29,63 @@ 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,
 };
 
 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;
-        }
-      }
+  if (inst == NULL)

[kwiberg-webrtc, 2015/10/29 16:35:21]

if (!inst)

[minyue-webrtc, 2015/10/30 14:06:46]

Done.

+    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);
+  OpusEncInst* state = (OpusEncInst*)calloc(1, sizeof(OpusEncInst));

[kwiberg-webrtc, 2015/10/29 16:35:21]

You don't need to cast. C allows implicit conversion from void* to any pointer
type.

[minyue-webrtc, 2015/10/30 14:06:46]

Done.

+  if (!state)
+    return -1;
+
+  // Allocate zero counters.
+  state->zero_counts = (size_t*)calloc(channels, sizeof(size_t));

[kwiberg-webrtc, 2015/10/29 16:35:21]

No need to cast.

[minyue-webrtc, 2015/10/30 14:06:46]

Done.

+  if (!state->zero_counts) {
+    free(state);
+    return -1;

[kwiberg-webrtc, 2015/10/29 16:35:21]

Don't try to handle malloc failures. Just assert if you feel you have to do
anything.

[minyue-webrtc, 2015/10/30 14:06:46]

Done.

+  }
+
+  int opus_app;
+  switch (application) {
+    case 0: {
+      opus_app = OPUS_APPLICATION_VOIP;
+      break;
+    }
+    case 1: {
+      opus_app = OPUS_APPLICATION_AUDIO;
+      break;
+    }
+    default: {
+      WebRtcOpus_EncoderFree(state);

[minyue-webrtc, 2015/10/30 14:06:46]

I found switch() {} can be placed before state gets allocated, to avoid line 65.

+      return -1;
     }
   }
-  return -1;
+
+  int error;
+  state->encoder = opus_encoder_create(48000, channels, opus_app,
+                                       &error);
+  if (error != OPUS_OK || state->encoder == NULL) {

[kwiberg-webrtc, 2015/10/29 16:35:21]

!state->encoder

[minyue-webrtc, 2015/10/30 14:06:46]

Done.

+    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 +99,35 @@ int WebRtcOpus_Encode(OpusEncInst* inst,
                       size_t length_encoded_buffer,
                       uint8_t* encoded) {
   int res;
+  int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs];
 
   if (samples > 48 * kWebRtcOpusMaxEncodeFrameSizeMs) {
     return -1;
   }
 
+  const int channels = inst->channels;
+  int16_t* pointer = buffer;
+  // Break long consecutive zeros by forcing a "1" every |kZeroBreakCount|
+  // samples.
+  memcpy(pointer, audio_in, samples * channels * sizeof(int16_t));
+  if (inst->in_dtx_mode) {
+    for (size_t i = 0; i < samples; ++i) {
+      for (int c = 0; c < channels; ++c, ++pointer) {
+        if (*pointer == 0) {
+          ++inst->zero_counts[c];
+          if (inst->zero_counts[c] == kZeroBreakCount) {
+            *pointer = 1;
+            inst->zero_counts[c] = 0;
+          }
+        } else {
+          inst->zero_counts[c] = 0;
+        }
+      }
+    }
+  }

[kwiberg-webrtc, 2015/10/29 16:35:21]

It would've been easier to read this loop if you'd used buffer[i *
channels + c] instead of *pointer.

Also, you pay the cost of copying whether you need it or not. If long
runs of zeros aren't common, this can get expensive. You could copy on
demand instead. Before the loop, do

  const int16_t* input = audio_in;
  int16_t* writable_input = NULL;

Then read from input[i * channels + c], and at the one place where you
write, do

  if (!writable_input) {
    memcpy(buffer, audio_in, samples * channels * sizeof(int16_t));
    input = writable_input = buffer;
  }
  writable_input[i * channels + c] = 1;

[minyue-webrtc, 2015/10/30 14:06:46]

Yes, I agree that "input[i * channels + c]" and I have changed it. But will the
compiler do "i * channel + c" as efficient as "++"?

Your idea of avoiding memcpy is nice and I have made a change. It is slightly
different from yours, just for maybe a little bit better readability.

[kwiberg-webrtc, 2015/11/01 02:01:55]

Yes. I tried these two:

void f1(int a, int b) {
  for (int i = 0; i < a; ++i) {
    for (int j = 0; j < b; ++j) {
      g(i * b + j);
      h(i * b + j);
    }
  }
}

void f2(int a, int b) {
  int ij = 0;
  for (int i = 0; i < a; ++i) {
    for (int j = 0; j < b; ++j, ++ij) {
      g(ij);
      h(ij);
    }
  }
}

clang -O2 didn't generate identical code for them, but close enough as to make
no difference. Modern compilers know to make this optimization on their own.

+
   res = opus_encode(inst->encoder,
-                    (const opus_int16*)audio_in,
+                    buffer,
                     (int)samples,
                     encoded,
                     (opus_int32)length_encoded_buffer);