From 85228d6af6c13c0a63ea11d8fe0bf240b752cfd0 Mon Sep 17 00:00:00 2001 From: ivoc Date: Wed, 27 Jul 2016 04:53:47 -0700 Subject: [PATCH] Regression test for issue where Opus DTX status was being forgotten. BUG=webrtc:6020 Review-Url: https://codereview.webrtc.org/2177263002 Cr-Commit-Position: refs/heads/master@{#13539} --- .../audio_coding/acm2/audio_coding_module.cc | 8 ++ .../audio_coding/codecs/audio_encoder.cc | 4 + .../audio_coding/codecs/audio_encoder.h | 4 + .../codecs/opus/audio_encoder_opus.cc | 4 + .../codecs/opus/audio_encoder_opus.h | 2 +- .../include/audio_coding_module.h | 4 + webrtc/voice_engine/channel.cc | 11 ++ webrtc/voice_engine/channel.h | 1 + webrtc/voice_engine/include/voe_codec.h | 6 + webrtc/voice_engine/voe_codec_impl.cc | 17 +++ webrtc/voice_engine/voe_codec_impl.h | 2 + webrtc/voice_engine/voe_codec_unittest.cc | 109 +++++------------- 12 files changed, 92 insertions(+), 80 deletions(-) diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module.cc index b394c59156..3f404f736f 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module.cc +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module.cc @@ -48,6 +48,8 @@ class AudioCodingModuleImpl final : public AudioCodingModule { void ModifyEncoder( FunctionView*)> modifier) override; + void QueryEncoder(FunctionView query) override; + // Get current send codec. rtc::Optional SendCodec() const override; @@ -596,6 +598,12 @@ void AudioCodingModuleImpl::ModifyEncoder( modifier(&encoder_stack_); } +void AudioCodingModuleImpl::QueryEncoder( + FunctionView query) { + rtc::CritScope lock(&acm_crit_sect_); + query(encoder_stack_.get()); +} + // Get current send codec. rtc::Optional AudioCodingModuleImpl::SendCodec() const { rtc::CritScope lock(&acm_crit_sect_); diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.cc b/webrtc/modules/audio_coding/codecs/audio_encoder.cc index 6b7f5f893f..c433dcdd0f 100644 --- a/webrtc/modules/audio_coding/codecs/audio_encoder.cc +++ b/webrtc/modules/audio_coding/codecs/audio_encoder.cc @@ -50,6 +50,10 @@ bool AudioEncoder::SetDtx(bool enable) { return !enable; } +bool AudioEncoder::GetDtx() const { + return false; +} + bool AudioEncoder::SetApplication(Application application) { return false; } diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.h b/webrtc/modules/audio_coding/codecs/audio_encoder.h index ecc28d96a1..f09525f145 100644 --- a/webrtc/modules/audio_coding/codecs/audio_encoder.h +++ b/webrtc/modules/audio_coding/codecs/audio_encoder.h @@ -127,6 +127,10 @@ class AudioEncoder { // supported). virtual bool SetDtx(bool enable); + // Returns the status of codec-internal DTX. The default implementation always + // returns false. + virtual bool GetDtx() const; + // Sets the application mode. Returns true if the codec was able to comply. // The default implementation just returns false. enum class Application { kSpeech, kAudio }; diff --git a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc index edafbbdf39..d03f2d3cc2 100644 --- a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc +++ b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc @@ -152,6 +152,10 @@ bool AudioEncoderOpus::SetDtx(bool enable) { return RecreateEncoderInstance(conf); } +bool AudioEncoderOpus::GetDtx() const { + return config_.dtx_enabled; +} + bool AudioEncoderOpus::SetApplication(Application application) { auto conf = config_; switch (application) { diff --git a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h index 367c9b90ea..48fb494dbe 100644 --- a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h +++ b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h @@ -75,6 +75,7 @@ class AudioEncoderOpus final : public AudioEncoder { // being inactive. During that, it still sends 2 packets (one for content, one // for signaling) about every 400 ms. bool SetDtx(bool enable) override; + bool GetDtx() const override; bool SetApplication(Application application) override; void SetMaxPlaybackRate(int frequency_hz) override; @@ -84,7 +85,6 @@ class AudioEncoderOpus final : public AudioEncoder { // Getters for testing. double packet_loss_rate() const { return packet_loss_rate_; } ApplicationMode application() const { return config_.application; } - bool dtx_enabled() const { return config_.dtx_enabled; } protected: EncodedInfo EncodeImpl(uint32_t rtp_timestamp, diff --git a/webrtc/modules/audio_coding/include/audio_coding_module.h b/webrtc/modules/audio_coding/include/audio_coding_module.h index 5b35bcf7a5..30a17f72ea 100644 --- a/webrtc/modules/audio_coding/include/audio_coding_module.h +++ b/webrtc/modules/audio_coding/include/audio_coding_module.h @@ -250,6 +250,10 @@ class AudioCodingModule { virtual void ModifyEncoder( FunctionView*)> modifier) = 0; + // |modifier| is called exactly once with one argument: a const pointer to the + // current encoder (which is null if there is no current encoder). + virtual void QueryEncoder(FunctionView query) = 0; + // Utility method for simply replacing the existing encoder with a new one. void SetEncoder(std::unique_ptr new_encoder) { ModifyEncoder([&](std::unique_ptr* encoder) { diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 424d291bcb..dcedd757aa 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -1548,6 +1548,17 @@ int Channel::SetOpusDtx(bool enable_dtx) { return 0; } +int Channel::GetOpusDtx(bool* enabled) { + int success = -1; + audio_coding_->QueryEncoder([&](AudioEncoder const* encoder) { + if (encoder) { + *enabled = encoder->GetDtx(); + success = 0; + } + }); + return success; +} + int32_t Channel::RegisterExternalTransport(Transport* transport) { WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId), "Channel::RegisterExternalTransport()"); diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h index 68211a605a..b51b7ec68e 100644 --- a/webrtc/voice_engine/channel.h +++ b/webrtc/voice_engine/channel.h @@ -228,6 +228,7 @@ class Channel int32_t SetSendCNPayloadType(int type, PayloadFrequencies frequency); int SetOpusMaxPlaybackRate(int frequency_hz); int SetOpusDtx(bool enable_dtx); + int GetOpusDtx(bool* enabled); // VoENetwork int32_t RegisterExternalTransport(Transport* transport); diff --git a/webrtc/voice_engine/include/voe_codec.h b/webrtc/voice_engine/include/voe_codec.h index 5d94ac2736..cedf47d6b9 100644 --- a/webrtc/voice_engine/include/voe_codec.h +++ b/webrtc/voice_engine/include/voe_codec.h @@ -131,6 +131,12 @@ class WEBRTC_DLLEXPORT VoECodec { // success, and -1 if failed. virtual int SetOpusDtx(int channel, bool enable_dtx) = 0; + // If send codec is Opus on a specified |channel|, return its DTX status. + // Returns 0 on success, and -1 if failed. + // TODO(ivoc): Make GetOpusDtxStatus() pure virtual when all deriving classes + // are updated. + virtual int GetOpusDtxStatus(int channel, bool* enabled) { return -1; } + protected: VoECodec() {} virtual ~VoECodec() {} diff --git a/webrtc/voice_engine/voe_codec_impl.cc b/webrtc/voice_engine/voe_codec_impl.cc index 19891bc39f..8ac24da4e7 100644 --- a/webrtc/voice_engine/voe_codec_impl.cc +++ b/webrtc/voice_engine/voe_codec_impl.cc @@ -376,6 +376,23 @@ int VoECodecImpl::SetOpusDtx(int channel, bool enable_dtx) { return channelPtr->SetOpusDtx(enable_dtx); } +int VoECodecImpl::GetOpusDtxStatus(int channel, bool* enabled) { + WEBRTC_TRACE(kTraceApiCall, kTraceVoice, VoEId(_shared->instance_id(), -1), + "GetOpusDtx(channel=%d)", channel); + if (!_shared->statistics().Initialized()) { + _shared->SetLastError(VE_NOT_INITED, kTraceError); + return -1; + } + voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel); + voe::Channel* channelPtr = ch.channel(); + if (channelPtr == NULL) { + _shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError, + "GetOpusDtx failed to locate channel"); + return -1; + } + return channelPtr->GetOpusDtx(enabled); +} + #endif // WEBRTC_VOICE_ENGINE_CODEC_API } // namespace webrtc diff --git a/webrtc/voice_engine/voe_codec_impl.h b/webrtc/voice_engine/voe_codec_impl.h index 5519232055..d24bbac2e5 100644 --- a/webrtc/voice_engine/voe_codec_impl.h +++ b/webrtc/voice_engine/voe_codec_impl.h @@ -58,6 +58,8 @@ class VoECodecImpl : public VoECodec { int SetOpusDtx(int channel, bool enable_dtx) override; + int GetOpusDtxStatus(int channel, bool* enabled) override; + protected: VoECodecImpl(voe::SharedData* shared); ~VoECodecImpl() override; diff --git a/webrtc/voice_engine/voe_codec_unittest.cc b/webrtc/voice_engine/voe_codec_unittest.cc index 73e576bf51..c056ba07e3 100644 --- a/webrtc/voice_engine/voe_codec_unittest.cc +++ b/webrtc/voice_engine/voe_codec_unittest.cc @@ -22,85 +22,6 @@ namespace webrtc { namespace voe { namespace { -class VoECodecTest : public ::testing::Test { - protected: - VoECodecTest() - : voe_(VoiceEngine::Create()), - base_(VoEBase::GetInterface(voe_)), - voe_codec_(VoECodec::GetInterface(voe_)), - channel_(-1), - adm_(new FakeAudioDeviceModule), - red_payload_type_(-1) {} - - ~VoECodecTest() {} - - void TearDown() { - base_->DeleteChannel(channel_); - base_->Terminate(); - base_->Release(); - voe_codec_->Release(); - VoiceEngine::Delete(voe_); - } - - void SetUp() { - // Check if all components are valid. - ASSERT_TRUE(voe_ != NULL); - ASSERT_TRUE(base_ != NULL); - ASSERT_TRUE(voe_codec_ != NULL); - ASSERT_TRUE(adm_.get() != NULL); - ASSERT_EQ(0, base_->Init(adm_.get())); - channel_ = base_->CreateChannel(); - ASSERT_NE(-1, channel_); - - CodecInst my_codec; - - bool primary_found = false; - bool valid_secondary_found = false; - bool invalid_secondary_found = false; - - // Find primary and secondary codecs. - int num_codecs = voe_codec_->NumOfCodecs(); - int n = 0; - while (n < num_codecs && - (!primary_found || !valid_secondary_found || - !invalid_secondary_found || red_payload_type_ < 0)) { - EXPECT_EQ(0, voe_codec_->GetCodec(n, my_codec)); - if (!STR_CASE_CMP(my_codec.plname, "isac") && my_codec.plfreq == 16000) { - memcpy(&valid_secondary_, &my_codec, sizeof(my_codec)); - valid_secondary_found = true; - } else if (!STR_CASE_CMP(my_codec.plname, "isac") && - my_codec.plfreq == 32000) { - memcpy(&invalid_secondary_, &my_codec, sizeof(my_codec)); - invalid_secondary_found = true; - } else if (!STR_CASE_CMP(my_codec.plname, "L16") && - my_codec.plfreq == 16000) { - memcpy(&primary_, &my_codec, sizeof(my_codec)); - primary_found = true; - } else if (!STR_CASE_CMP(my_codec.plname, "RED")) { - red_payload_type_ = my_codec.pltype; - } - n++; - } - - EXPECT_TRUE(primary_found); - EXPECT_TRUE(valid_secondary_found); - EXPECT_TRUE(invalid_secondary_found); - EXPECT_NE(-1, red_payload_type_); - } - - VoiceEngine* voe_; - VoEBase* base_; - VoECodec* voe_codec_; - int channel_; - CodecInst primary_; - CodecInst valid_secondary_; - std::unique_ptr adm_; - - // A codec which is not valid to be registered as secondary codec. - CodecInst invalid_secondary_; - int red_payload_type_; -}; - TEST(VoECodecInst, TestCompareCodecInstances) { CodecInst codec1, codec2; memset(&codec1, 0, sizeof(CodecInst)); @@ -151,6 +72,36 @@ TEST(VoECodecInst, TestCompareCodecInstances) { EXPECT_FALSE(codec1 == codec2); } +// This is a regression test for +// https://bugs.chromium.org/p/webrtc/issues/detail?id=6020 +// The Opus DTX setting was being forgotten after unrelated VoE calls. +TEST(VoECodecInst, RememberOpusDtxAfterSettingChange) { + VoiceEngine* voe(VoiceEngine::Create()); + VoEBase* base(VoEBase::GetInterface(voe)); + VoECodec* voe_codec(VoECodec::GetInterface(voe)); + std::unique_ptr adm(new FakeAudioDeviceModule); + + base->Init(adm.get()); + + CodecInst codec = {111, "opus", 48000, 960, 1, 32000}; + + int channel = base->CreateChannel(); + + bool DTX = false; + + EXPECT_EQ(0, voe_codec->SetSendCodec(channel, codec)); + EXPECT_EQ(0, voe_codec->SetOpusDtx(channel, true)); + EXPECT_EQ(0, voe_codec->SetFECStatus(channel, true)); + EXPECT_EQ(0, voe_codec->GetOpusDtxStatus(channel, &DTX)); + EXPECT_TRUE(DTX); + + base->DeleteChannel(channel); + base->Terminate(); + base->Release(); + voe_codec->Release(); + VoiceEngine::Delete(voe); +} + } // namespace } // namespace voe } // namespace webrtc