From 6b19b560ac2270bfd146b96bb9c2eab47a155339 Mon Sep 17 00:00:00 2001 From: kwiberg Date: Tue, 20 Sep 2016 04:02:25 -0700 Subject: [PATCH] AcmReceiver: Ask NetEq to delete all decoders at once instead of one by one It requires a new NetEq method, but it can no longer fail. And we no longer need to use AcmReceiver::decoders_, which we're trying to eliminate. (This is a re-land of https://codereview.webrtc.org/2342313002.) BUG=webrtc:5801 Review-Url: https://codereview.webrtc.org/2348233002 Cr-Commit-Position: refs/heads/master@{#14304} --- .../modules/audio_coding/acm2/acm_receiver.cc | 21 +++---------------- .../modules/audio_coding/acm2/acm_receiver.h | 2 +- .../audio_coding/acm2/audio_coding_module.cc | 6 ++---- .../audio_coding/neteq/decoder_database.cc | 6 ++++++ .../audio_coding/neteq/decoder_database.h | 3 +++ .../neteq/decoder_database_unittest.cc | 15 +++++++++++++ .../audio_coding/neteq/include/neteq.h | 3 +++ .../neteq/mock/mock_decoder_database.h | 1 + .../modules/audio_coding/neteq/neteq_impl.cc | 5 +++++ .../modules/audio_coding/neteq/neteq_impl.h | 2 ++ .../audio_coding/neteq/neteq_impl_unittest.cc | 6 ++++++ 11 files changed, 47 insertions(+), 23 deletions(-) diff --git a/webrtc/modules/audio_coding/acm2/acm_receiver.cc b/webrtc/modules/audio_coding/acm2/acm_receiver.cc index 641818db3b..cbf3492955 100644 --- a/webrtc/modules/audio_coding/acm2/acm_receiver.cc +++ b/webrtc/modules/audio_coding/acm2/acm_receiver.cc @@ -246,27 +246,12 @@ void AcmReceiver::FlushBuffers() { neteq_->FlushBuffers(); } -// If failed in removing one of the codecs, this method continues to remove as -// many as it can. -int AcmReceiver::RemoveAllCodecs() { - int ret_val = 0; +void AcmReceiver::RemoveAllCodecs() { rtc::CritScope lock(&crit_sect_); - for (auto it = decoders_.begin(); it != decoders_.end(); ) { - auto cur = it; - ++it; // it will be valid even if we erase cur - if (neteq_->RemovePayloadType(cur->second.payload_type) == 0) { - decoders_.erase(cur); - } else { - LOG_F(LS_ERROR) << "Cannot remove payload " - << static_cast(cur->second.payload_type); - ret_val = -1; - } - } - - // No codec is registered, invalidate last audio decoder. + neteq_->RemoveAllPayloadTypes(); + decoders_.clear(); last_audio_decoder_ = rtc::Optional(); last_packet_sample_rate_hz_ = rtc::Optional(); - return ret_val; } int AcmReceiver::RemoveCodec(uint8_t payload_type) { diff --git a/webrtc/modules/audio_coding/acm2/acm_receiver.h b/webrtc/modules/audio_coding/acm2/acm_receiver.h index d39581ed4c..53def2b164 100644 --- a/webrtc/modules/audio_coding/acm2/acm_receiver.h +++ b/webrtc/modules/audio_coding/acm2/acm_receiver.h @@ -186,7 +186,7 @@ class AcmReceiver { // // Remove all registered codecs. // - int RemoveAllCodecs(); + void RemoveAllCodecs(); // Returns the RTP timestamp for the last sample delivered by GetAudio(). // The return value will be empty if no valid timestamp is available. diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module.cc index 4784a8721f..73f03a47ad 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module.cc +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module.cc @@ -948,10 +948,8 @@ int AudioCodingModuleImpl::InitializeReceiverSafe() { // If the receiver is already initialized then we want to destroy any // existing decoders. After a call to this function, we should have a clean // start-up. - if (receiver_initialized_) { - if (receiver_.RemoveAllCodecs() < 0) - return -1; - } + if (receiver_initialized_) + receiver_.RemoveAllCodecs(); receiver_.ResetInitialDelay(); receiver_.SetMinimumDelay(0); receiver_.SetMaximumDelay(0); diff --git a/webrtc/modules/audio_coding/neteq/decoder_database.cc b/webrtc/modules/audio_coding/neteq/decoder_database.cc index b189e4bb56..f5fbad3446 100644 --- a/webrtc/modules/audio_coding/neteq/decoder_database.cc +++ b/webrtc/modules/audio_coding/neteq/decoder_database.cc @@ -169,6 +169,12 @@ int DecoderDatabase::Remove(uint8_t rtp_payload_type) { return kOK; } +void DecoderDatabase::RemoveAll() { + decoders_.clear(); + active_decoder_type_ = -1; // No active decoder. + active_cng_decoder_type_ = -1; // No active CNG decoder. +} + const DecoderDatabase::DecoderInfo* DecoderDatabase::GetDecoderInfo( uint8_t rtp_payload_type) const { DecoderMap::const_iterator it = decoders_.find(rtp_payload_type); diff --git a/webrtc/modules/audio_coding/neteq/decoder_database.h b/webrtc/modules/audio_coding/neteq/decoder_database.h index 6d277c95c9..3728d1da90 100644 --- a/webrtc/modules/audio_coding/neteq/decoder_database.h +++ b/webrtc/modules/audio_coding/neteq/decoder_database.h @@ -136,6 +136,9 @@ class DecoderDatabase { // Returns kDecoderNotFound or kOK depending on the outcome of the operation. virtual int Remove(uint8_t rtp_payload_type); + // Remove all entries. + virtual void RemoveAll(); + // Returns a pointer to the DecoderInfo struct for |rtp_payload_type|. If // no decoder is registered with that |rtp_payload_type|, NULL is returned. virtual const DecoderInfo* GetDecoderInfo(uint8_t rtp_payload_type) const; diff --git a/webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc b/webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc index 380e719d1d..39c000c05e 100644 --- a/webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc @@ -47,6 +47,21 @@ TEST(DecoderDatabase, InsertAndRemove) { EXPECT_TRUE(db.Empty()); } +TEST(DecoderDatabase, InsertAndRemoveAll) { + DecoderDatabase db(new rtc::RefCountedObject); + const std::string kCodecName1 = "Robert\'); DROP TABLE Students;"; + const std::string kCodecName2 = "https://xkcd.com/327/"; + EXPECT_EQ(DecoderDatabase::kOK, + db.RegisterPayload(0, NetEqDecoder::kDecoderPCMu, kCodecName1)); + EXPECT_EQ(DecoderDatabase::kOK, + db.RegisterPayload(1, NetEqDecoder::kDecoderPCMa, kCodecName2)); + EXPECT_EQ(2, db.Size()); + EXPECT_FALSE(db.Empty()); + db.RemoveAll(); + EXPECT_EQ(0, db.Size()); + EXPECT_TRUE(db.Empty()); +} + TEST(DecoderDatabase, GetDecoderInfo) { rtc::scoped_refptr factory( new rtc::RefCountedObject); diff --git a/webrtc/modules/audio_coding/neteq/include/neteq.h b/webrtc/modules/audio_coding/neteq/include/neteq.h index 22d994e3aa..952ab238eb 100644 --- a/webrtc/modules/audio_coding/neteq/include/neteq.h +++ b/webrtc/modules/audio_coding/neteq/include/neteq.h @@ -183,6 +183,9 @@ class NetEq { // -1 on failure. virtual int RemovePayloadType(uint8_t rtp_payload_type) = 0; + // Removes all payload types from the codec database. + virtual void RemoveAllPayloadTypes() = 0; + // Sets a minimum delay in millisecond for packet buffer. The minimum is // maintained unless a higher latency is dictated by channel condition. // Returns true if the minimum is successfully applied, otherwise false is diff --git a/webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h b/webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h index 1f6d3bc058..7347959e8c 100644 --- a/webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h +++ b/webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h @@ -42,6 +42,7 @@ class MockDecoderDatabase : public DecoderDatabase { AudioDecoder* decoder)); MOCK_METHOD1(Remove, int(uint8_t rtp_payload_type)); + MOCK_METHOD0(RemoveAll, void()); MOCK_CONST_METHOD1(GetDecoderInfo, const DecoderInfo*(uint8_t rtp_payload_type)); MOCK_CONST_METHOD1(GetRtpPayloadType, diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc index 27d47c104b..ca20e5ba6d 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -293,6 +293,11 @@ int NetEqImpl::RemovePayloadType(uint8_t rtp_payload_type) { return kFail; } +void NetEqImpl::RemoveAllPayloadTypes() { + rtc::CritScope lock(&crit_sect_); + decoder_database_->RemoveAll(); +} + bool NetEqImpl::SetMinimumDelay(int delay_ms) { rtc::CritScope lock(&crit_sect_); if (delay_ms >= 0 && delay_ms < 10000) { diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.h b/webrtc/modules/audio_coding/neteq/neteq_impl.h index 8795ef5210..7903ba665c 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.h +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.h @@ -123,6 +123,8 @@ class NetEqImpl : public webrtc::NetEq { // -1 on failure. int RemovePayloadType(uint8_t rtp_payload_type) override; + void RemoveAllPayloadTypes() override; + bool SetMinimumDelay(int delay_ms) override; bool SetMaximumDelay(int delay_ms) override; diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc index 1b8ee82b25..4f98005cbb 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc @@ -230,6 +230,12 @@ TEST_F(NetEqImplTest, RemovePayloadType) { EXPECT_EQ(NetEq::kFail, neteq_->RemovePayloadType(rtp_payload_type)); } +TEST_F(NetEqImplTest, RemoveAllPayloadTypes) { + CreateInstance(); + EXPECT_CALL(*mock_decoder_database_, RemoveAll()).WillOnce(Return()); + neteq_->RemoveAllPayloadTypes(); +} + TEST_F(NetEqImplTest, InsertPacket) { CreateInstance(); const size_t kPayloadLength = 100;