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
diff --git a/webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h b/webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h
index 697121e..5068c7f 100644
--- a/webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h
+++ b/webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h
@@ -63,6 +63,13 @@
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(...)
*
* This function resets state of the decoder.
diff --git a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
index 867262d..7da5df8 100644
--- a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
+++ b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c
@@ -55,6 +55,7 @@
int16_t WebRtcOpus_EncoderFree(OpusEncInst* inst) {
opus_encoder_destroy(inst->encoder);
+ free(inst);
return 0;
}
@@ -90,23 +91,36 @@
};
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 @@
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) {
diff --git a/webrtc/modules/audio_coding/main/source/acm_codec_database.cc b/webrtc/modules/audio_coding/main/source/acm_codec_database.cc
index 338dce1..19bccb6 100644
--- a/webrtc/modules/audio_coding/main/source/acm_codec_database.cc
+++ b/webrtc/modules/audio_coding/main/source/acm_codec_database.cc
@@ -190,10 +190,8 @@
#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 @@
#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 @@
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 @@
// 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 @@
#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
diff --git a/webrtc/modules/audio_coding/main/source/acm_codec_database.h b/webrtc/modules/audio_coding/main/source/acm_codec_database.h
index 60578db..4822153 100644
--- a/webrtc/modules/audio_coding/main/source/acm_codec_database.h
+++ b/webrtc/modules/audio_coding/main/source/acm_codec_database.h
@@ -92,10 +92,8 @@
, kGSMFR
#endif
#ifdef WEBRTC_CODEC_OPUS
- // Mono
+ // Mono and stereo
, kOpus
- // Stereo
- , kOpus_2ch
#endif
#ifdef WEBRTC_CODEC_SPEEX
, kSPEEX8
@@ -178,10 +176,8 @@
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};
diff --git a/webrtc/modules/audio_coding/main/source/acm_opus.cc b/webrtc/modules/audio_coding/main/source/acm_opus.cc
index ca28921..4e9e5a2 100644
--- a/webrtc/modules/audio_coding/main/source/acm_opus.cc
+++ b/webrtc/modules/audio_coding/main/source/acm_opus.cc
@@ -111,7 +111,7 @@
// 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,14 +190,16 @@
}
int16_t ACMOpus::InternalInitDecoder(WebRtcACMCodecParams* codecParams) {
- if (_decoderInstPtr != NULL) {
- WebRtcOpus_DecoderFree(_decoderInstPtr);
- _decoderInstPtr = NULL;
+ if(_decoderInstPtr == NULL) {
+ if (WebRtcOpus_DecoderCreate(&_decoderInstPtr,
+ codecParams->codecInstant.channels) < 0) {
+ return -1;
+ }
}
- 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 @@
// 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.
diff --git a/webrtc/modules/audio_coding/main/source/audio_coding_module.cc b/webrtc/modules/audio_coding/main/source/audio_coding_module.cc
index 0efc8b7..12ebf00 100644
--- a/webrtc/modules/audio_coding/main/source/audio_coding_module.cc
+++ b/webrtc/modules/audio_coding/main/source/audio_coding_module.cc
@@ -59,6 +59,10 @@
// 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;
}
diff --git a/webrtc/modules/audio_coding/main/source/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/main/source/audio_coding_module_impl.cc
index 0a399ba..f586c22 100644
--- a/webrtc/modules/audio_coding/main/source/audio_coding_module_impl.cc
+++ b/webrtc/modules/audio_coding/main/source/audio_coding_module_impl.cc
@@ -1285,7 +1285,7 @@
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 @@
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;
}
diff --git a/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc b/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc
index 89a9829..d4d74c2 100644
--- a/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc
+++ b/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc
@@ -147,6 +147,9 @@
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 @@
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");
diff --git a/webrtc/modules/audio_coding/main/test/TestStereo.cc b/webrtc/modules/audio_coding/main/test/TestStereo.cc
index 97d6c2e..3a4b652 100644
--- a/webrtc/modules/audio_coding/main/test/TestStereo.cc
+++ b/webrtc/modules/audio_coding/main/test/TestStereo.cc
@@ -550,12 +550,18 @@
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 @@
}
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.
@@ -680,6 +739,9 @@
#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",
test_cntr_);
diff --git a/webrtc/modules/audio_coding/main/test/TestVADDTX.cc b/webrtc/modules/audio_coding/main/test/TestVADDTX.cc
index fcca374..567903b 100644
--- a/webrtc/modules/audio_coding/main/test/TestVADDTX.cc
+++ b/webrtc/modules/audio_coding/main/test/TestVADDTX.cc
@@ -83,6 +83,10 @@
{
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 @@
}
}
+ // We only allow VAD/DTX when sending mono.
+ myCodecParam.channels = 1;
CHECK_ERROR(myACM->RegisterSendCodec(myCodecParam));
// initialization was succesful
diff --git a/webrtc/modules/audio_coding/neteq/codec_db.c b/webrtc/modules/audio_coding/neteq/codec_db.c
index fd59886..10277d5 100644
--- a/webrtc/modules/audio_coding/neteq/codec_db.c
+++ b/webrtc/modules/audio_coding/neteq/codec_db.c
@@ -117,7 +117,6 @@
#endif
#ifdef NETEQ_OPUS_CODEC
case kDecoderOpus :
- case kDecoderOpus_2ch :
#endif
#ifdef NETEQ_G722_CODEC
case kDecoderG722 :
@@ -466,7 +465,6 @@
#endif
#ifdef NETEQ_OPUS_CODEC
case kDecoderOpus:
- case kDecoderOpus_2ch :
#endif
#ifdef NETEQ_ARBITRARY_CODEC
case kDecoderArbitrary:
diff --git a/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h b/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h
index 5af201e..9621036 100644
--- a/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h
+++ b/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h
@@ -63,7 +63,6 @@
kDecoderG722_1C_32,
kDecoderG722_1C_48,
kDecoderOpus,
- kDecoderOpus_2ch,
kDecoderSPEEX_8,
kDecoderSPEEX_16,
kDecoderCELT_32,
diff --git a/webrtc/modules/audio_coding/neteq/packet_buffer.c b/webrtc/modules/audio_coding/neteq/packet_buffer.c
index e8ee40c..c4c301c 100644
--- a/webrtc/modules/audio_coding/neteq/packet_buffer.c
+++ b/webrtc/modules/audio_coding/neteq/packet_buffer.c
@@ -621,8 +621,7 @@
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 */
diff --git a/webrtc/modules/audio_coding/neteq/recin.c b/webrtc/modules/audio_coding/neteq/recin.c
index 24e1eee..00c8f81 100644
--- a/webrtc/modules/audio_coding/neteq/recin.c
+++ b/webrtc/modules/audio_coding/neteq/recin.c
@@ -381,7 +381,6 @@
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