From 63643357b4d70fe703468acc4cd9089a27c314cf Mon Sep 17 00:00:00 2001 From: Jakob Ivarsson Date: Thu, 16 Mar 2023 22:09:39 +0100 Subject: [PATCH] Remove CNG state tracking from NetEq decision logic. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It seems like this is legacy and not useful. A comment mentions transitioning between CNG and DTMF modes, but there is no way this can happen currently. Bug: webrtc:13322 Change-Id: I9e4706cb6ee145ee37a9e11e7cab6ea4ff697dc4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297980 Reviewed-by: Henrik Lundin Commit-Queue: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#39590} --- api/neteq/neteq_controller.h | 7 -- modules/audio_coding/neteq/decision_logic.cc | 38 +++---- modules/audio_coding/neteq/decision_logic.h | 11 -- .../neteq/mock/mock_neteq_controller.h | 3 - modules/audio_coding/neteq/neteq_impl.cc | 8 +- .../audio_coding/neteq/neteq_impl_unittest.cc | 101 ------------------ 6 files changed, 15 insertions(+), 153 deletions(-) diff --git a/api/neteq/neteq_controller.h b/api/neteq/neteq_controller.h index f0101d3d1a..35112796f8 100644 --- a/api/neteq/neteq_controller.h +++ b/api/neteq/neteq_controller.h @@ -144,13 +144,6 @@ class NetEqController { virtual bool SetBaseMinimumDelay(int delay_ms) = 0; virtual int GetBaseMinimumDelay() const = 0; - // These methods test the `cng_state_` for different conditions. - virtual bool CngRfc3389On() const = 0; - virtual bool CngOff() const = 0; - - // Resets the `cng_state_` to kCngOff. - virtual void SetCngOff() = 0; - // Reports back to DecisionLogic whether the decision to do expand remains or // not. Note that this is necessary, since an expand decision can be changed // to kNormal in NetEqImpl::GetDecision if there is still enough data in the diff --git a/modules/audio_coding/neteq/decision_logic.cc b/modules/audio_coding/neteq/decision_logic.cc index 45402d9a83..144b4e7e9e 100644 --- a/modules/audio_coding/neteq/decision_logic.cc +++ b/modules/audio_coding/neteq/decision_logic.cc @@ -129,15 +129,6 @@ void DecisionLogic::SetSampleRate(int fs_hz, size_t output_size_samples) { NetEq::Operation DecisionLogic::GetDecision(const NetEqStatus& status, bool* reset_decoder) { - // If last mode was CNG (or Expand, since this could be covering up for - // a lost CNG packet), remember that CNG is on. This is needed if comfort - // noise is interrupted by DTMF. - if (status.last_mode == NetEq::Mode::kRfc3389Cng) { - cng_state_ = kCngRfc3389On; - } else if (status.last_mode == NetEq::Mode::kCodecInternalCng) { - cng_state_ = kCngInternalOn; - } - if (!IsExpand(status.last_mode) && !IsCng(status.last_mode)) { last_playout_delay_ms_ = GetPlayoutDelayMs(status); } @@ -311,22 +302,21 @@ NetEq::Operation DecisionLogic::CngOperation( } NetEq::Operation DecisionLogic::NoPacket(NetEqController::NetEqStatus status) { - if (cng_state_ == kCngRfc3389On) { - // Keep on playing comfort noise. - return NetEq::Operation::kRfc3389CngNoPacket; - } else if (cng_state_ == kCngInternalOn) { - // Stop CNG after a timeout. - if (config_.cng_timeout_ms && - status.generated_noise_samples > - static_cast(*config_.cng_timeout_ms * sample_rate_khz_)) { - return NetEq::Operation::kExpand; + switch (status.last_mode) { + case NetEq::Mode::kRfc3389Cng: + return NetEq::Operation::kRfc3389CngNoPacket; + case NetEq::Mode::kCodecInternalCng: { + // Stop CNG after a timeout. + if (config_.cng_timeout_ms && + status.generated_noise_samples > + static_cast(*config_.cng_timeout_ms * sample_rate_khz_)) { + return NetEq::Operation::kExpand; + } + return NetEq::Operation::kCodecInternalCng; } - return NetEq::Operation::kCodecInternalCng; - } else if (status.play_dtmf) { - return NetEq::Operation::kDtmf; - } else { - // Nothing to play, do expand. - return NetEq::Operation::kExpand; + default: + return status.play_dtmf ? NetEq::Operation::kDtmf + : NetEq::Operation::kExpand; } } diff --git a/modules/audio_coding/neteq/decision_logic.h b/modules/audio_coding/neteq/decision_logic.h index 41c62c4dde..008679d07f 100644 --- a/modules/audio_coding/neteq/decision_logic.h +++ b/modules/audio_coding/neteq/decision_logic.h @@ -58,13 +58,6 @@ class DecisionLogic : public NetEqController { NetEq::Operation GetDecision(const NetEqController::NetEqStatus& status, bool* reset_decoder) override; - // These methods test the `cng_state_` for different conditions. - bool CngRfc3389On() const override { return cng_state_ == kCngRfc3389On; } - bool CngOff() const override { return cng_state_ == kCngOff; } - - // Resets the `cng_state_` to kCngOff. - void SetCngOff() override { cng_state_ = kCngOff; } - void ExpandDecision(NetEq::Operation operation) override {} // Adds `value` to `sample_memory_`. @@ -111,8 +104,6 @@ class DecisionLogic : public NetEqController { // The value 5 sets maximum time-stretch rate to about 100 ms/s. static const int kMinTimescaleInterval = 5; - enum CngState { kCngOff, kCngRfc3389On, kCngInternalOn }; - // Updates the `buffer_level_filter_` with the current buffer level // `buffer_size_samples`. void FilterBufferLevel(size_t buffer_size_samples); @@ -178,8 +169,6 @@ class DecisionLogic : public NetEqController { const TickTimer* tick_timer_; int sample_rate_khz_; size_t output_size_samples_; - CngState cng_state_ = kCngOff; // Remember if comfort noise is interrupted by - // other event (e.g., DTMF). size_t noise_fast_forward_ = 0; size_t packet_length_samples_ = 0; int sample_memory_ = 0; diff --git a/modules/audio_coding/neteq/mock/mock_neteq_controller.h b/modules/audio_coding/neteq/mock/mock_neteq_controller.h index 6d88e09216..dc5cab4b16 100644 --- a/modules/audio_coding/neteq/mock/mock_neteq_controller.h +++ b/modules/audio_coding/neteq/mock/mock_neteq_controller.h @@ -36,9 +36,6 @@ class MockNetEqController : public NetEqController { MOCK_METHOD(bool, SetMinimumDelay, (int delay_ms), (override)); MOCK_METHOD(bool, SetBaseMinimumDelay, (int delay_ms), (override)); MOCK_METHOD(int, GetBaseMinimumDelay, (), (const, override)); - MOCK_METHOD(bool, CngRfc3389On, (), (const, override)); - MOCK_METHOD(bool, CngOff, (), (const, override)); - MOCK_METHOD(void, SetCngOff, (), (override)); MOCK_METHOD(void, ExpandDecision, (NetEq::Operation operation), (override)); MOCK_METHOD(void, AddSampleMemory, (int32_t value), (override)); MOCK_METHOD(int, TargetLevelMs, (), (const, override)); diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc index 6a6367d045..2c19e7e213 100644 --- a/modules/audio_coding/neteq/neteq_impl.cc +++ b/modules/audio_coding/neteq/neteq_impl.cc @@ -1046,7 +1046,7 @@ int NetEqImpl::GetDecision(Operation* operation, controller_->noise_fast_forward() : 0; - if (controller_->CngRfc3389On() || last_mode_ == Mode::kRfc3389Cng) { + if (last_mode_ == Mode::kRfc3389Cng) { // Because of timestamp peculiarities, we have to "manually" disallow using // a CNG packet with the same timestamp as the one that was last played. // This can happen when using redundancy and will cause the timing to shift. @@ -1280,12 +1280,6 @@ int NetEqImpl::GetDecision(Operation* operation, int extracted_samples = 0; if (packet) { sync_buffer_->IncreaseEndTimestamp(packet->timestamp - end_timestamp); - - if (*operation != Operation::kRfc3389Cng) { - // We are about to decode and use a non-CNG packet. - controller_->SetCngOff(); - } - extracted_samples = ExtractPackets(required_samples, packet_list); if (extracted_samples < 0) { return kPacketBufferCorruption; diff --git a/modules/audio_coding/neteq/neteq_impl_unittest.cc b/modules/audio_coding/neteq/neteq_impl_unittest.cc index ce2be656ef..6d68fab7fd 100644 --- a/modules/audio_coding/neteq/neteq_impl_unittest.cc +++ b/modules/audio_coding/neteq/neteq_impl_unittest.cc @@ -1377,106 +1377,6 @@ TEST_F(NetEqImplTest, DecodingError) { EXPECT_CALL(mock_decoder, Die()); } -// This test checks the behavior of NetEq when audio decoder fails during CNG. -TEST_F(NetEqImplTest, DecodingErrorDuringInternalCng) { - UseNoMocks(); - - // Create a mock decoder object. - MockAudioDecoder mock_decoder; - CreateInstance( - rtc::make_ref_counted(&mock_decoder)); - - const uint8_t kPayloadType = 17; // Just an arbitrary number. - const int kSampleRateHz = 8000; - const int kDecoderErrorCode = -97; // Any negative number. - - // We let decoder return 5 ms each time, and therefore, 2 packets make 10 ms. - const size_t kFrameLengthSamples = - static_cast(5 * kSampleRateHz / 1000); - - const size_t kPayloadLengthBytes = 1; // This can be arbitrary. - - uint8_t payload[kPayloadLengthBytes] = {0}; - - RTPHeader rtp_header; - rtp_header.payloadType = kPayloadType; - rtp_header.sequenceNumber = 0x1234; - rtp_header.timestamp = 0x12345678; - rtp_header.ssrc = 0x87654321; - - EXPECT_CALL(mock_decoder, Reset()).WillRepeatedly(Return()); - EXPECT_CALL(mock_decoder, SampleRateHz()) - .WillRepeatedly(Return(kSampleRateHz)); - EXPECT_CALL(mock_decoder, Channels()).WillRepeatedly(Return(1)); - EXPECT_CALL(mock_decoder, PacketDuration(_, _)) - .WillRepeatedly(Return(rtc::checked_cast(kFrameLengthSamples))); - EXPECT_CALL(mock_decoder, ErrorCode()).WillOnce(Return(kDecoderErrorCode)); - int16_t dummy_output[kFrameLengthSamples] = {0}; - - { - InSequence sequence; // Dummy variable. - // Mock decoder works normally the first 2 times. - EXPECT_CALL(mock_decoder, - DecodeInternal(_, kPayloadLengthBytes, kSampleRateHz, _, _)) - .Times(2) - .WillRepeatedly( - DoAll(SetArrayArgument<3>(dummy_output, - dummy_output + kFrameLengthSamples), - SetArgPointee<4>(AudioDecoder::kComfortNoise), - Return(rtc::checked_cast(kFrameLengthSamples)))) - .RetiresOnSaturation(); - - // Then mock decoder fails. A common reason for failure can be buffer being - // too short - EXPECT_CALL(mock_decoder, DecodeInternal(nullptr, 0, kSampleRateHz, _, _)) - .WillOnce(Return(-1)) - .RetiresOnSaturation(); - - // Mock decoder finally returns to normal. - EXPECT_CALL(mock_decoder, DecodeInternal(nullptr, 0, kSampleRateHz, _, _)) - .Times(2) - .WillRepeatedly( - DoAll(SetArrayArgument<3>(dummy_output, - dummy_output + kFrameLengthSamples), - SetArgPointee<4>(AudioDecoder::kComfortNoise), - Return(rtc::checked_cast(kFrameLengthSamples)))); - } - - EXPECT_TRUE(neteq_->RegisterPayloadType(kPayloadType, - SdpAudioFormat("l16", 8000, 1))); - - // Insert 2 packets. This will make netEq into codec internal CNG mode. - for (int i = 0; i < 2; ++i) { - rtp_header.sequenceNumber += 1; - rtp_header.timestamp += kFrameLengthSamples; - EXPECT_EQ(NetEq::kOK, neteq_->InsertPacket(rtp_header, payload)); - } - - // Pull audio. - const size_t kMaxOutputSize = static_cast(10 * kSampleRateHz / 1000); - AudioFrame output; - bool muted; - EXPECT_EQ(NetEq::kOK, neteq_->GetAudio(&output, &muted)); - EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_); - EXPECT_EQ(1u, output.num_channels_); - EXPECT_EQ(AudioFrame::kCNG, output.speech_type_); - - // Pull audio again. Decoder fails. - EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&output, &muted)); - EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_); - EXPECT_EQ(1u, output.num_channels_); - // We are not expecting anything for output.speech_type_, since an error was - // returned. - - // Pull audio again, should resume codec CNG. - EXPECT_EQ(NetEq::kOK, neteq_->GetAudio(&output, &muted)); - EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_); - EXPECT_EQ(1u, output.num_channels_); - EXPECT_EQ(AudioFrame::kCNG, output.speech_type_); - - EXPECT_CALL(mock_decoder, Die()); -} - // Tests that the return value from last_output_sample_rate_hz() is equal to the // configured inital sample rate. TEST_F(NetEqImplTest, InitialLastOutputSampleRate) { @@ -1783,7 +1683,6 @@ TEST_F(NetEqImplTest120ms, Merge) { Register120msCodec(AudioDecoder::kSpeech); CreateInstanceWithDelayManagerMock(); - EXPECT_CALL(*mock_neteq_controller_, CngOff()).WillRepeatedly(Return(true)); InsertPacket(first_timestamp()); GetFirstPacket();