From 3f81fcd2e845505c2873c7ec2edade910213596b Mon Sep 17 00:00:00 2001 From: kwiberg Date: Thu, 23 Jun 2016 03:58:36 -0700 Subject: [PATCH] Don't recreate the speech encoder if we don't have to If the specification for the speech encoder hasn't changed, we should reuse it instead of recreating it. Otherwise, we lose its state. (This problem was originally discovered because AudioEncoderOpus instances would forget that they were supposed to be using DTX.) BUG=webrtc:6020, chromium:622647 Review-Url: https://codereview.webrtc.org/2089183002 Cr-Commit-Position: refs/heads/master@{#13273} --- webrtc/base/array_view.h | 2 + .../audio_coding/acm2/codec_manager.cc | 64 ++++++++++++++++++- .../modules/audio_coding/acm2/codec_manager.h | 22 +------ .../audio_coding/codecs/audio_encoder.cc | 3 + .../audio_coding/codecs/audio_encoder.h | 9 +++ .../codecs/cng/audio_encoder_cng.cc | 5 ++ .../codecs/cng/audio_encoder_cng.h | 2 + .../codecs/red/audio_encoder_copy_red.cc | 5 ++ .../codecs/red/audio_encoder_copy_red.h | 2 + 9 files changed, 93 insertions(+), 21 deletions(-) diff --git a/webrtc/base/array_view.h b/webrtc/base/array_view.h index a7ca66cc95..868009631f 100644 --- a/webrtc/base/array_view.h +++ b/webrtc/base/array_view.h @@ -56,6 +56,7 @@ namespace rtc { // Contains17(arr); // C array // Contains17(arr); // std::vector // Contains17(rtc::ArrayView(arr, size)); // pointer + size +// Contains17(nullptr); // nullptr -> empty ArrayView // ... // // One important point is that ArrayView and ArrayView are @@ -73,6 +74,7 @@ class ArrayView final { public: // Construct an empty ArrayView. ArrayView() : ArrayView(static_cast(nullptr), 0) {} + ArrayView(std::nullptr_t) : ArrayView() {} // Construct an ArrayView for a (pointer,size) pair. template diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.cc b/webrtc/modules/audio_coding/acm2/codec_manager.cc index 81adf81a83..f028c45f99 100644 --- a/webrtc/modules/audio_coding/acm2/codec_manager.cc +++ b/webrtc/modules/audio_coding/acm2/codec_manager.cc @@ -113,7 +113,7 @@ bool CodecManager::RegisterEncoder(const CodecInst& send_codec) { } send_codec_inst_ = rtc::Optional(send_codec); - codec_stack_params_.speech_encoder.reset(); // Caller must recreate it. + recreate_encoder_ = true; // Caller must recreate it. return true; } @@ -190,5 +190,67 @@ bool CodecManager::SetCodecFEC(bool enable_codec_fec) { return true; } +bool CodecManager::MakeEncoder(RentACodec* rac, AudioCodingModule* acm) { + RTC_DCHECK(rac); + RTC_DCHECK(acm); + + if (!recreate_encoder_) { + bool error = false; + // Try to re-use the speech encoder we've given to the ACM. + acm->ModifyEncoder([&](std::unique_ptr* encoder) { + if (!*encoder) { + // There is no existing encoder. + recreate_encoder_ = true; + return; + } + + // Extract the speech encoder from the ACM. + std::unique_ptr enc = std::move(*encoder); + while (true) { + auto sub_enc = enc->ReclaimContainedEncoders(); + if (sub_enc.empty()) { + break; + } + RTC_CHECK_EQ(1u, sub_enc.size()); + + // Replace enc with its sub encoder. We need to put the sub encoder in + // a temporary first, since otherwise the old value of enc would be + // destroyed before the new value got assigned, which would be bad + // since the new value is a part of the old value. + auto tmp_enc = std::move(sub_enc[0]); + enc = std::move(tmp_enc); + } + + // Wrap it in a new encoder stack and put it back. + codec_stack_params_.speech_encoder = std::move(enc); + *encoder = rac->RentEncoderStack(&codec_stack_params_); + if (!*encoder) { + error = true; + } + }); + if (error) { + return false; + } + if (!recreate_encoder_) { + return true; + } + } + + if (!send_codec_inst_) { + // We don't have the information we need to create a new speech encoder. + // (This is not an error.) + return true; + } + + codec_stack_params_.speech_encoder = rac->RentEncoder(*send_codec_inst_); + auto stack = rac->RentEncoderStack(&codec_stack_params_); + if (!stack) { + return false; + } + acm->SetEncoder(std::move(stack)); + recreate_encoder_ = false; + return true; +} + } // namespace acm2 } // namespace webrtc diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.h b/webrtc/modules/audio_coding/acm2/codec_manager.h index f6c6cd46d2..b60b7e7bcb 100644 --- a/webrtc/modules/audio_coding/acm2/codec_manager.h +++ b/webrtc/modules/audio_coding/acm2/codec_manager.h @@ -59,31 +59,13 @@ class CodecManager final { // Uses the provided Rent-A-Codec to create a new encoder stack, if we have a // complete specification; if so, it is then passed to set_encoder. On error, // returns false. - bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm) { - RTC_DCHECK(rac); - RTC_DCHECK(acm); - if (!codec_stack_params_.speech_encoder && send_codec_inst_) { - // We have no speech encoder, but we have a specification for making one. - auto enc = rac->RentEncoder(*send_codec_inst_); - if (!enc) - return false; - codec_stack_params_.speech_encoder = std::move(enc); - } - auto stack = rac->RentEncoderStack(&codec_stack_params_); - if (stack) { - // Give new encoder stack to the ACM. - acm->SetEncoder(std::move(stack)); - } else { - // The specification was good but incomplete, so we have no encoder stack - // to give to the ACM. - } - return true; - } + bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm); private: rtc::ThreadChecker thread_checker_; rtc::Optional send_codec_inst_; RentACodec::StackParameters codec_stack_params_; + bool recreate_encoder_ = true; // Need to recreate encoder? RTC_DISALLOW_COPY_AND_ASSIGN(CodecManager); }; diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.cc b/webrtc/modules/audio_coding/codecs/audio_encoder.cc index 000371d28a..6b7f5f893f 100644 --- a/webrtc/modules/audio_coding/codecs/audio_encoder.cc +++ b/webrtc/modules/audio_coding/codecs/audio_encoder.cc @@ -60,4 +60,7 @@ void AudioEncoder::SetProjectedPacketLossRate(double fraction) {} void AudioEncoder::SetTargetBitrate(int target_bps) {} +rtc::ArrayView> +AudioEncoder::ReclaimContainedEncoders() { return nullptr; } + } // namespace webrtc diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.h b/webrtc/modules/audio_coding/codecs/audio_encoder.h index f4c5794d92..ecc28d96a1 100644 --- a/webrtc/modules/audio_coding/codecs/audio_encoder.h +++ b/webrtc/modules/audio_coding/codecs/audio_encoder.h @@ -149,6 +149,15 @@ class AudioEncoder { // implementation does the latter). virtual void SetTargetBitrate(int target_bps); + // Causes this encoder to let go of any other encoders it contains, and + // returns a pointer to an array where they are stored (which is required to + // live as long as this encoder). Unless the returned array is empty, you may + // not call any methods on this encoder afterwards, except for the + // destructor. The default implementation just returns an empty array. + // NOTE: This method is subject to change. Do not call or override it. + virtual rtc::ArrayView> + ReclaimContainedEncoders(); + protected: // Subclasses implement this to perform the actual encoding. Called by // Encode(). diff --git a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc index 09c25d9616..d2edcb5c26 100644 --- a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc +++ b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc @@ -187,6 +187,11 @@ void AudioEncoderCng::SetTargetBitrate(int bits_per_second) { speech_encoder_->SetTargetBitrate(bits_per_second); } +rtc::ArrayView> +AudioEncoderCng::ReclaimContainedEncoders() { + return rtc::ArrayView>(&speech_encoder_, 1); +} + AudioEncoder::EncodedInfo AudioEncoderCng::EncodePassive( size_t frames_to_encode, rtc::Buffer* encoded) { diff --git a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h index 246e9519da..a895e69de4 100644 --- a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h +++ b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h @@ -63,6 +63,8 @@ class AudioEncoderCng final : public AudioEncoder { void SetMaxPlaybackRate(int frequency_hz) override; void SetProjectedPacketLossRate(double fraction) override; void SetTargetBitrate(int target_bps) override; + rtc::ArrayView> ReclaimContainedEncoders() + override; private: EncodedInfo EncodePassive(size_t frames_to_encode, diff --git a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc index 7c48c0c60e..37fa55a4da 100644 --- a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc +++ b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc @@ -123,4 +123,9 @@ void AudioEncoderCopyRed::SetTargetBitrate(int bits_per_second) { speech_encoder_->SetTargetBitrate(bits_per_second); } +rtc::ArrayView> +AudioEncoderCopyRed::ReclaimContainedEncoders() { + return rtc::ArrayView>(&speech_encoder_, 1); +} + } // namespace webrtc diff --git a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h index ead9fd65a2..a08118364c 100644 --- a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h +++ b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h @@ -51,6 +51,8 @@ class AudioEncoderCopyRed final : public AudioEncoder { void SetMaxPlaybackRate(int frequency_hz) override; void SetProjectedPacketLossRate(double fraction) override; void SetTargetBitrate(int target_bps) override; + rtc::ArrayView> ReclaimContainedEncoders() + override; protected: EncodedInfo EncodeImpl(uint32_t rtp_timestamp,