Prevent Opus DTX from generating intermittent noise during silence

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

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 11 matching lines @@
    * side, we must allow for packets of that size. NetEq is currently limited
    * to 60 ms on the receive side. */
   kWebRtcOpusMaxDecodeFrameSizeMs = 120,
 
   /* Maximum sample count per channel is 48 kHz * maximum frame size in
    * milliseconds. */
   kWebRtcOpusMaxFrameSizePerChannel = 48 * kWebRtcOpusMaxDecodeFrameSizeMs,
 
   /* 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 {
     return -1;
   }
 }
 
 int WebRtcOpus_Encode(OpusEncInst* inst,
                       const int16_t* audio_in,
                       size_t samples,
                       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);
 
   if (res == 1) {
     // Indicates DTX since the packet has nothing but a header. In principle,
     // there is no need to send this packet. However, we do transmit the first
     // occurrence to let the decoder know that the encoder enters DTX mode.
     if (inst->in_dtx_mode) {
       return 0;
@@ skipping 348 matching lines @@
     return 0;
   }
 
   for (n = 0; n < channels; n++) {
     if (frame_data[0][0] & (0x80 >> ((n + 1) * (frames + 1) - 1)))
       return 1;
   }
 
   return 0;
 }