From c4590580e8de143443b88fde53518b5ed8c9ce76 Mon Sep 17 00:00:00 2001 From: "tina.legrand@webrtc.org" Date: Wed, 28 Nov 2012 12:23:29 +0000 Subject: [PATCH] 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 --- .../codecs/opus/interface/opus_interface.h | 7 +++ .../audio_coding/codecs/opus/opus_interface.c | 46 +++++++++----- .../main/source/acm_codec_database.cc | 28 ++++----- .../main/source/acm_codec_database.h | 8 +-- .../audio_coding/main/source/acm_opus.cc | 27 ++++---- .../main/source/audio_coding_module.cc | 4 ++ .../main/source/audio_coding_module_impl.cc | 22 ++++++- .../audio_coding/main/test/TestAllCodecs.cc | 4 ++ .../audio_coding/main/test/TestStereo.cc | 62 +++++++++++++++++++ .../audio_coding/main/test/TestVADDTX.cc | 6 ++ webrtc/modules/audio_coding/neteq/codec_db.c | 2 - .../neteq/interface/webrtc_neteq.h | 1 - .../audio_coding/neteq/packet_buffer.c | 3 +- webrtc/modules/audio_coding/neteq/recin.c | 1 - 14 files changed, 161 insertions(+), 60 deletions(-) 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 697121e50..5068c7f45 100644 --- a/webrtc/modules/audio_coding/codecs/opus/interface/opus_interface.h +++ b/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(...) * diff --git a/webrtc/modules/audio_coding/codecs/opus/opus_interface.c b/webrtc/modules/audio_coding/codecs/opus/opus_interface.c index 867262d0b..7da5df8a7 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_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) { 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 338dce139..19bccb649 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 @@ 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 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 60578db0b..482215395 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 @@ 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}; diff --git a/webrtc/modules/audio_coding/main/source/acm_opus.cc b/webrtc/modules/audio_coding/main/source/acm_opus.cc index ca2892161..4e9e5a25c 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 @@ 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. 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 0efc8b7c1..12ebf00db 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 @@ 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; } 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 0a399ba3f..f586c224b 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 @@ 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; } diff --git a/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc b/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc index 89a9829a8..d4d74c215 100644 --- a/webrtc/modules/audio_coding/main/test/TestAllCodecs.cc +++ b/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"); diff --git a/webrtc/modules/audio_coding/main/test/TestStereo.cc b/webrtc/modules/audio_coding/main/test/TestStereo.cc index 97d6c2ebd..3a4b65237 100644 --- a/webrtc/modules/audio_coding/main/test/TestStereo.cc +++ b/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", diff --git a/webrtc/modules/audio_coding/main/test/TestVADDTX.cc b/webrtc/modules/audio_coding/main/test/TestVADDTX.cc index fcca3748d..567903b26 100644 --- a/webrtc/modules/audio_coding/main/test/TestVADDTX.cc +++ b/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 diff --git a/webrtc/modules/audio_coding/neteq/codec_db.c b/webrtc/modules/audio_coding/neteq/codec_db.c index fd5988699..10277d506 100644 --- a/webrtc/modules/audio_coding/neteq/codec_db.c +++ b/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: diff --git a/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h b/webrtc/modules/audio_coding/neteq/interface/webrtc_neteq.h index 5af201e04..9621036dc 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 @@ enum WebRtcNetEQDecoder 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 e8ee40c91..c4c301cd0 100644 --- a/webrtc/modules/audio_coding/neteq/packet_buffer.c +++ b/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 */ diff --git a/webrtc/modules/audio_coding/neteq/recin.c b/webrtc/modules/audio_coding/neteq/recin.c index 24e1eeed2..00c8f81da 100644 --- a/webrtc/modules/audio_coding/neteq/recin.c +++ b/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