Opus mono/stereo on the same payloadtype, and fix of memory bug

During call setup Opus should always be signaled as a 48000 Hz stereo codec, not depending on what we plan to send, or how we plan to decode received packets.
The previous implementation had different payload types for mono and stereo, which breaks the proposed standard.

While working on this CL I ran in to the problem reported earlier, that we could get a crash related to deleting decoder memory. This should now be solved in Patch Set 3.

BUG=issue1013, issue1112

Review URL: https://webrtc-codereview.appspot.com/933022

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3177 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h

@@ -62,6 +62,13 @@ int16_t WebRtcOpus_SetBitRate(OpusEncInst* inst, int32_t rate);
 int16_t WebRtcOpus_DecoderCreate(OpusDecInst** inst, int channels);
 int16_t WebRtcOpus_DecoderFree(OpusDecInst* inst);
 
+/****************************************************************************
+ * WebRtcOpus_DecoderChannels(...)
+ *
+ * This function returns the number of channels created for Opus decoder.
+ */
+int WebRtcOpus_DecoderChannels(OpusDecInst* inst);
+
 /****************************************************************************
  * WebRtcOpus_DecoderInit(...)
  *

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

@@ -55,6 +55,7 @@ int16_t WebRtcOpus_EncoderCreate(OpusEncInst** inst, int32_t channels) {
 
 int16_t WebRtcOpus_EncoderFree(OpusEncInst* inst) {
   opus_encoder_destroy(inst->encoder);
+  free(inst);
   return 0;
 }
 
@@ -90,23 +91,36 @@ struct WebRtcOpusDecInst {
 };
 
 int16_t WebRtcOpus_DecoderCreate(OpusDecInst** inst, int channels) {
+  int error_l;
+  int error_r;
   OpusDecInst* state;
+
+  // Create Opus decoder memory.
   state = (OpusDecInst*) calloc(1, sizeof(OpusDecInst));
-  if (state) {
-    int error_l;
-    int error_r;
-    // Always create a 48000 Hz Opus decoder.
-    state->decoder_left = opus_decoder_create(48000, channels, &error_l);
-    state->decoder_right = opus_decoder_create(48000, channels, &error_r);
-    if (error_l == OPUS_OK && error_r == OPUS_OK &&
-        state->decoder_left != NULL && state->decoder_right != NULL) {
-      state->channels = channels;
-      *inst = state;
-      return 0;
-    }
-    free(state);
-    state = NULL;
+  if (state == NULL) {
+    return -1;
   }
+
+  // Create new memory for left and right channel, always at 48000 Hz.
+  state->decoder_left = opus_decoder_create(48000, channels, &error_l);
+  state->decoder_right = opus_decoder_create(48000, channels, &error_r);
+  if (error_l == OPUS_OK && error_r == OPUS_OK && state->decoder_left != NULL
+      && state->decoder_right != NULL) {
+    // Creation of memory all ok.
+    state->channels = channels;
+    *inst = state;
+    return 0;
+  }
+
+  // If memory allocation was unsuccessful, free the entire state.
+  if (state->decoder_left) {
+    opus_decoder_destroy(state->decoder_left);
+  }
+  if (state->decoder_right) {
+    opus_decoder_destroy(state->decoder_right);
+  }
+  free(state);
+  state = NULL;
   return -1;
 }
 
@@ -117,6 +131,10 @@ int16_t WebRtcOpus_DecoderFree(OpusDecInst* inst) {
   return 0;
 }
 
+int WebRtcOpus_DecoderChannels(OpusDecInst* inst) {
+  return inst->channels;
+}
+
 int16_t WebRtcOpus_DecoderInit(OpusDecInst* inst) {
   int error = opus_decoder_ctl(inst->decoder_left, OPUS_RESET_STATE);
   if (error == OPUS_OK) {

webrtc/modules/audio_coding/main/source/acm_codec_database.cc

@@ -190,10 +190,8 @@ const CodecInst ACMCodecDB::database_[] = {
 #endif
 #ifdef WEBRTC_CODEC_OPUS
   // Opus internally supports 48, 24, 16, 12, 8 kHz.
-  // Mono
-  {120, "opus", 48000, 960, 1, 32000},
-  // Stereo
-  {121, "opus", 48000, 960, 2, 32000},
+  // Mono and stereo.
+  {120, "opus", 48000, 960, 2, 32000},
 #endif
 #ifdef WEBRTC_CODEC_SPEEX
   {kDynamicPayloadtypes[count_database++], "speex", 8000, 160, 1, 11000},
@@ -285,9 +283,7 @@ const ACMCodecDB::CodecSettings ACMCodecDB::codec_settings_[] = {
 #ifdef WEBRTC_CODEC_OPUS
   // Opus supports frames shorter than 10ms,
   // but it doesn't help us to use them.
-  // Mono
-  {1, {960}, 0, 2},
-  // Stereo
+  // Mono and stereo.
   {1, {960}, 0, 2},
 #endif
 #ifdef WEBRTC_CODEC_SPEEX
@@ -375,10 +371,8 @@ const WebRtcNetEQDecoder ACMCodecDB::neteq_decoders_[] = {
   kDecoderGSMFR,
 #endif
 #ifdef WEBRTC_CODEC_OPUS
-  // Mono
+  // Mono and stereo.
   kDecoderOpus,
-  // Stereo
-  kDecoderOpus_2ch,
 #endif
 #ifdef WEBRTC_CODEC_SPEEX
   kDecoderSPEEX_8,
@@ -571,7 +565,13 @@ int ACMCodecDB::CodecId(const char* payload_name, int frequency, int channels) {
     // always treated as true, like for RED.
     name_match = (STR_CASE_CMP(database_[id].plname, payload_name) == 0);
     frequency_match = (frequency == database_[id].plfreq) || (frequency == -1);
-    channels_match = (channels == database_[id].channels);
+    // The number of channels must match for all codecs but Opus.
+    if (STR_CASE_CMP(payload_name, "opus") != 0) {
+      channels_match = (channels == database_[id].channels);
+    } else {
+      // For opus we just check that number of channels is valid.
+      channels_match = (channels == 1 || channels == 2);
+    }
 
     if (name_match && frequency_match && channels_match) {
       // We have found a matching codec in the list.
@@ -767,11 +767,7 @@ ACMGenericCodec* ACMCodecDB::CreateCodecInstance(const CodecInst* codec_inst) {
 #endif
   } else if (!STR_CASE_CMP(codec_inst->plname, "opus")) {
 #ifdef WEBRTC_CODEC_OPUS
-    if (codec_inst->channels == 1) {
-      return new ACMOpus(kOpus);
-    } else {
-      return new ACMOpus(kOpus_2ch);
-    }
+    return new ACMOpus(kOpus);
 #endif
   } else if (!STR_CASE_CMP(codec_inst->plname, "speex")) {
 #ifdef WEBRTC_CODEC_SPEEX

webrtc/modules/audio_coding/main/source/acm_codec_database.h

@@ -92,10 +92,8 @@ class ACMCodecDB {
     , kGSMFR
 #endif
 #ifdef WEBRTC_CODEC_OPUS
-    // Mono
+    // Mono and stereo
     , kOpus
-    // Stereo
-    , kOpus_2ch
 #endif
 #ifdef WEBRTC_CODEC_SPEEX
     , kSPEEX8
@@ -178,10 +176,8 @@ class ACMCodecDB {
   enum {kSPEEX16 = -1};
 #endif
 #ifndef WEBRTC_CODEC_OPUS
-  // Mono
+  // Mono and stereo
   enum {kOpus = -1};
-  // Stereo
-  enum {kOpus_2ch = -1};
 #endif
 #ifndef WEBRTC_CODEC_AVT
   enum {kAVT = -1};

webrtc/modules/audio_coding/main/source/acm_opus.cc

@@ -111,7 +111,7 @@ ACMOpus::ACMOpus(int16_t codecID)
   // Opus has internal DTX, but we dont use it for now.
   _hasInternalDTX = false;
 
-  if ((_codecID != ACMCodecDB::kOpus) && (_codecID != ACMCodecDB::kOpus_2ch)) {
+  if (_codecID != ACMCodecDB::kOpus) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, _uniqueID,
                  "Wrong codec id for Opus.");
     _sampleFreq = -1;
@@ -190,15 +190,17 @@ int16_t ACMOpus::InternalInitEncoder(WebRtcACMCodecParams* codecParams) {
 }
 
 int16_t ACMOpus::InternalInitDecoder(WebRtcACMCodecParams* codecParams) {
-  if (_decoderInstPtr != NULL) {
-    WebRtcOpus_DecoderFree(_decoderInstPtr);
-    _decoderInstPtr = NULL;
-  }
-  if (WebRtcOpus_DecoderCreate(&_decoderInstPtr,
-                               codecParams->codecInstant.channels) < 0) {
-    return -1;
+  if(_decoderInstPtr == NULL) {
+    if (WebRtcOpus_DecoderCreate(&_decoderInstPtr,
+                                 codecParams->codecInstant.channels) < 0) {
+      return -1;
+    }
   }
 
+  // Number of channels in decoder should match the number in |codecParams|.
+  assert(codecParams->codecInstant.channels ==
+      WebRtcOpus_DecoderChannels(_decoderInstPtr));
+
   if (WebRtcOpus_DecoderInit(_decoderInstPtr) < 0) {
     return -1;
   }
@@ -221,13 +223,8 @@ int32_t ACMOpus::CodecDef(WebRtcNetEQ_CodecDef& codecDef,
   // TODO(tlegrand): Decoder is registered in NetEQ as a 32 kHz decoder, which
   // is true until we have a full 48 kHz system, and remove the downsampling
   // in the Opus decoder wrapper.
-  if (codecInst.channels == 1) {
-    SET_CODEC_PAR(codecDef, kDecoderOpus, codecInst.pltype, _decoderInstPtr,
-                  32000);
-  } else {
-    SET_CODEC_PAR(codecDef, kDecoderOpus_2ch, codecInst.pltype,
-                  _decoderInstPtr, 32000);
-  }
+  SET_CODEC_PAR(codecDef, kDecoderOpus, codecInst.pltype,
+                _decoderInstPtr, 32000);
 
   // If this is the master of NetEQ, regular decoder will be added, otherwise
   // the slave decoder will be used.

webrtc/modules/audio_coding/main/source/audio_coding_module.cc

@@ -59,6 +59,10 @@ WebRtc_Word32 AudioCodingModule::Codec(const char* payload_name,
   // Get default codec settings.
   ACMCodecDB::Codec(codec_id, &codec);
 
+  // Keep the number of channels from the function call. For most codecs it
+  // will be the same value as in defaul codec settings, but not for all.
+  codec.channels = channels;
+
   return 0;
 }
 

webrtc/modules/audio_coding/main/source/audio_coding_module_impl.cc

@@ -1285,7 +1285,7 @@ WebRtc_Word32 AudioCodingModuleImpl::PlayoutFrequency() const {
   return _netEq.CurrentSampFreqHz();
 }
 
-// Register possible reveive codecs, can be called multiple times,
+// Register possible receive codecs, can be called multiple times,
 // for codecs, CNG (NB, WB and SWB), DTMF, RED.
 WebRtc_Word32 AudioCodingModuleImpl::RegisterReceiveCodec(
     const CodecInst& receive_codec) {
@@ -1394,14 +1394,30 @@ WebRtc_Word32 AudioCodingModuleImpl::RegisterReceiveCodec(
 
     if (!_stereoReceive[codec_id]
         && (_lastRecvAudioCodecPlType == receive_codec.pltype)) {
-      // The last received payload type is the same as the current one, but
-      // was marked as mono. Reset to avoid problems.
+      // The last received payload type is the same as the one we are
+      // registering. Expected number of channels to receive is one (mono),
+      // but we are now registering the receiving codec as stereo (number of
+      // channels is 2).
+      // Set |_lastRecvAudioCodedPlType| to invalid value to trigger a flush in
+      // NetEq, and a reset of expected number of channels next time a packet is
+      // received in AudioCodingModuleImpl::IncomingPacket().
       _lastRecvAudioCodecPlType = -1;
     }
 
     _stereoReceive[codec_id] = true;
     _stereoReceiveRegistered = true;
   } else {
+    if (_lastRecvAudioCodecPlType == receive_codec.pltype &&
+        _expected_channels == 2) {
+      // The last received payload type is the same as the one we are
+      // registering. Expected number of channels to receive is two (stereo),
+      // but we are now registering the receiving codec as mono (number of
+      // channels is 1).
+      // Set |_lastRecvAudioCodedPlType| to invalid value to trigger a flush in
+      // NetEq, and a reset of expected number of channels next time a packet is
+      // received in AudioCodingModuleImpl::IncomingPacket().
+      _lastRecvAudioCodecPlType = -1;
+    }
     _stereoReceive[codec_id] = false;
   }
 

webrtc/modules/audio_coding/main/test/TestAllCodecs.cc

@@ -147,6 +147,9 @@ void TestAllCodecs::Perform() {
   CodecInst my_codec_param;
   for (uint8_t n = 0; n < num_encoders; n++) {
     acm_b_->Codec(n, my_codec_param);
+    if (!strcmp(my_codec_param.plname, "opus")) {
+      my_codec_param.channels = 1;
+    }
     acm_b_->RegisterReceiveCodec(my_codec_param);
   }
 
@@ -635,6 +638,7 @@ void TestAllCodecs::Perform() {
   Run(channel_a_to_b_);
   RegisterSendCodec('A', codec_opus, 48000, 500000, 960, -1);
   Run(channel_a_to_b_);
+  outfile_b_.Close();
 #endif
   if (test_mode_ != 0) {
     printf("===============================================================\n");

webrtc/modules/audio_coding/main/test/TestStereo.cc

@@ -550,12 +550,18 @@ void TestStereo::Perform() {
     printf("Test number: %d\n",test_cntr_ + 1);
     printf("Test type: Mono-to-stereo\n");
   }
+
+  // Keep encode and decode in stereo.
   test_cntr_++;
   channel_a2b_->set_codec_mode(kStereo);
   OpenOutFile(test_cntr_);
   RegisterSendCodec('A', codec_opus, 48000, 64000, 960, codec_channels,
                     opus_pltype_);
   Run(channel_a2b_, audio_channels, codec_channels);
+
+  // Encode in mono, decode in stereo mode.
+  RegisterSendCodec('A', codec_opus, 48000, 64000, 960, 1, opus_pltype_);
+  Run(channel_a2b_, audio_channels, codec_channels);
   out_file_.Close();
 #endif
 
@@ -661,10 +667,63 @@ void TestStereo::Perform() {
   }
   test_cntr_++;
   OpenOutFile(test_cntr_);
+  // Encode and decode in mono.
   RegisterSendCodec('A', codec_opus, 48000, 32000, 960, codec_channels,
                      opus_pltype_);
+  CodecInst opus_codec_param;
+  for (WebRtc_UWord8 n = 0; n < num_encoders; n++) {
+    EXPECT_EQ(0, acm_b_->Codec(n, opus_codec_param));
+    if (!strcmp(opus_codec_param.plname, "opus")) {
+      opus_codec_param.channels = 1;
+      EXPECT_EQ(0, acm_b_->RegisterReceiveCodec(opus_codec_param));
+      break;
+    }
+  }
+  Run(channel_a2b_, audio_channels, codec_channels);
+
+  // Encode in stereo, decode in mono.
+  RegisterSendCodec('A', codec_opus, 48000, 32000, 960, 2, opus_pltype_);
+  Run(channel_a2b_, audio_channels, codec_channels);
+
+  out_file_.Close();
+
+  // Test switching between decoding mono and stereo for Opus.
+
+  // Decode in mono.
+  test_cntr_++;
+  OpenOutFile(test_cntr_);
+  if (test_mode_ != 0) {
+    // Print out codec and settings
+    printf("Test number: %d\nCodec: Opus Freq: 48000 Rate :32000 PackSize: 960"
+        " Decode: mono\n", test_cntr_);
+  }
   Run(channel_a2b_, audio_channels, codec_channels);
   out_file_.Close();
+  // Decode in stereo.
+  test_cntr_++;
+  OpenOutFile(test_cntr_);
+  if (test_mode_ != 0) {
+    // Print out codec and settings
+    printf("Test number: %d\nCodec: Opus Freq: 48000 Rate :32000 PackSize: 960"
+        " Decode: stereo\n", test_cntr_);
+  }
+  opus_codec_param.channels = 2;
+  EXPECT_EQ(0, acm_b_->RegisterReceiveCodec(opus_codec_param));
+  Run(channel_a2b_, audio_channels, 2);
+  out_file_.Close();
+  // Decode in mono.
+  test_cntr_++;
+  OpenOutFile(test_cntr_);
+  if (test_mode_ != 0) {
+    // Print out codec and settings
+    printf("Test number: %d\nCodec: Opus Freq: 48000 Rate :32000 PackSize: 960"
+        " Decode: mono\n", test_cntr_);
+  }
+  opus_codec_param.channels = 1;
+  EXPECT_EQ(0, acm_b_->RegisterReceiveCodec(opus_codec_param));
+  Run(channel_a2b_, audio_channels, codec_channels);
+  out_file_.Close();
+
 #endif
 
   // Print out which codecs were tested, and which were not, in the run.
@@ -679,6 +738,9 @@ void TestStereo::Perform() {
     printf("   G.711\n");
 #ifdef WEBRTC_CODEC_CELT
     printf("   CELT\n");
+#endif
+#ifdef WEBRTC_CODEC_OPUS
+    printf("   Opus\n");
 #endif
     printf("\nTo complete the test, listen to the %d number of output "
            "files.\n",

webrtc/modules/audio_coding/main/test/TestVADDTX.cc

@@ -83,6 +83,10 @@ void TestVADDTX::Perform()
         {
             printf("%s\n", myCodecParam.plname);
         }
+        if (!strcmp(myCodecParam.plname, "opus")) {
+          // Use mono decoding for Opus in the VAD/DTX test.
+          myCodecParam.channels = 1;
+        }
         _acmB->RegisterReceiveCodec(myCodecParam);
     }
 
@@ -337,6 +341,8 @@ WebRtc_Word16 TestVADDTX::RegisterSendCodec(char side,
         }
     }
 
+    // We only allow VAD/DTX when sending mono.
+    myCodecParam.channels = 1;
     CHECK_ERROR(myACM->RegisterSendCodec(myCodecParam));
 
     // initialization was succesful

webrtc/modules/audio_coding/neteq/codec_db.c

@@ -117,7 +117,6 @@ int WebRtcNetEQ_DbAdd(CodecDbInst_t *inst, enum WebRtcNetEQDecoder codec,
 #endif
 #ifdef NETEQ_OPUS_CODEC
         case kDecoderOpus :
-        case kDecoderOpus_2ch :
 #endif
 #ifdef NETEQ_G722_CODEC
         case kDecoderG722 :
@@ -466,7 +465,6 @@ int WebRtcNetEQ_DbGetSplitInfo(SplitInfo_t *inst, enum WebRtcNetEQDecoder codecI
 #endif
 #ifdef NETEQ_OPUS_CODEC
         case kDecoderOpus:
-        case kDecoderOpus_2ch :
 #endif
 #ifdef NETEQ_ARBITRARY_CODEC
         case kDecoderArbitrary:

webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h

@@ -63,7 +63,6 @@ enum WebRtcNetEQDecoder
     kDecoderG722_1C_32,
     kDecoderG722_1C_48,
     kDecoderOpus,
-    kDecoderOpus_2ch,
     kDecoderSPEEX_8,
     kDecoderSPEEX_16,
     kDecoderCELT_32,

webrtc/modules/audio_coding/neteq/packet_buffer.c

@@ -621,8 +621,7 @@ int WebRtcNetEQ_GetDefaultCodecSettings(const enum WebRtcNetEQDecoder *codecID,
             codecBytes = 1560; /* 240ms @ 52kbps (30ms frames) */
             codecBuffers = 8;
         }
-        else if ((codecID[i] == kDecoderOpus)  ||
-            (codecID[i] == kDecoderOpus_2ch))
+        else if (codecID[i] == kDecoderOpus)
         {
             codecBytes = 15300; /* 240ms @ 510kbps (60ms frames) */
             codecBuffers = 30;  /* Replicating the value for PCMu/a */

webrtc/modules/audio_coding/neteq/recin.c

@@ -381,7 +381,6 @@ int WebRtcNetEQ_GetTimestampScaling(MCUInst_t *MCU_inst, int rtpPayloadType)
             break;
         }
         case kDecoderOpus:
-        case kDecoderOpus_2ch:
         {
             /* We resample Opus internally to 32 kHz, but timestamps
              * are counted at 48 kHz. So there are two output samples