diff --git a/webrtc/video/encoder_state_feedback.cc b/webrtc/video/encoder_state_feedback.cc index f0b03a13fe..0240e487a6 100644 --- a/webrtc/video/encoder_state_feedback.cc +++ b/webrtc/video/encoder_state_feedback.cc @@ -10,113 +10,75 @@ #include "webrtc/video/encoder_state_feedback.h" -#include - #include "webrtc/base/checks.h" -#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "webrtc/video/vie_encoder.h" namespace webrtc { -// Helper class registered at the RTP module relaying callbacks to -// EncoderStatFeedback. -class EncoderStateFeedbackObserver : public RtcpIntraFrameObserver { - public: - explicit EncoderStateFeedbackObserver(EncoderStateFeedback* owner) - : owner_(owner) {} - ~EncoderStateFeedbackObserver() {} +EncoderStateFeedback::EncoderStateFeedback() : vie_encoder_(nullptr) {} - // Implements RtcpIntraFrameObserver. - virtual void OnReceivedIntraFrameRequest(uint32_t ssrc) { - owner_->OnReceivedIntraFrameRequest(ssrc); - } - virtual void OnReceivedSLI(uint32_t ssrc, uint8_t picture_id) { - owner_->OnReceivedSLI(ssrc, picture_id); - } - virtual void OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id) { - owner_->OnReceivedRPSI(ssrc, picture_id); - } - - virtual void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) { - owner_->OnLocalSsrcChanged(old_ssrc, new_ssrc); - } - - private: - EncoderStateFeedback* owner_; -}; - -EncoderStateFeedback::EncoderStateFeedback() - : observer_(new EncoderStateFeedbackObserver(this)) {} - -EncoderStateFeedback::~EncoderStateFeedback() { - assert(encoders_.empty()); -} - -void EncoderStateFeedback::AddEncoder(const std::vector& ssrcs, - ViEEncoder* encoder) { +void EncoderStateFeedback::Init(const std::vector& ssrcs, + ViEEncoder* encoder) { RTC_DCHECK(!ssrcs.empty()); rtc::CritScope lock(&crit_); - for (uint32_t ssrc : ssrcs) { - RTC_DCHECK(encoders_.find(ssrc) == encoders_.end()); - encoders_[ssrc] = encoder; - } + ssrcs_ = ssrcs; + vie_encoder_ = encoder; } -void EncoderStateFeedback::RemoveEncoder(const ViEEncoder* encoder) { +void EncoderStateFeedback::TearDown() { rtc::CritScope lock(&crit_); - SsrcEncoderMap::iterator it = encoders_.begin(); - while (it != encoders_.end()) { - if (it->second == encoder) { - encoders_.erase(it++); - } else { - ++it; - } - } + RTC_DCHECK(vie_encoder_); + ssrcs_.clear(); + vie_encoder_ = nullptr; } -RtcpIntraFrameObserver* EncoderStateFeedback::GetRtcpIntraFrameObserver() { - return observer_.get(); +bool EncoderStateFeedback::HasSsrc(uint32_t ssrc) { + for (uint32_t registered_ssrc : ssrcs_) { + if (registered_ssrc == ssrc) + return true; + } + return false; } void EncoderStateFeedback::OnReceivedIntraFrameRequest(uint32_t ssrc) { rtc::CritScope lock(&crit_); - SsrcEncoderMap::iterator it = encoders_.find(ssrc); - if (it == encoders_.end()) + if (!HasSsrc(ssrc)) return; + RTC_DCHECK(vie_encoder_); - it->second->OnReceivedIntraFrameRequest(ssrc); + vie_encoder_->OnReceivedIntraFrameRequest(ssrc); } void EncoderStateFeedback::OnReceivedSLI(uint32_t ssrc, uint8_t picture_id) { rtc::CritScope lock(&crit_); - SsrcEncoderMap::iterator it = encoders_.find(ssrc); - if (it == encoders_.end()) + if (!HasSsrc(ssrc)) return; + RTC_DCHECK(vie_encoder_); - it->second->OnReceivedSLI(ssrc, picture_id); + vie_encoder_->OnReceivedSLI(ssrc, picture_id); } void EncoderStateFeedback::OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id) { rtc::CritScope lock(&crit_); - SsrcEncoderMap::iterator it = encoders_.find(ssrc); - if (it == encoders_.end()) + if (!HasSsrc(ssrc)) return; + RTC_DCHECK(vie_encoder_); - it->second->OnReceivedRPSI(ssrc, picture_id); + vie_encoder_->OnReceivedRPSI(ssrc, picture_id); } +// Sending SSRCs for this encoder should never change since they are configured +// once and not reconfigured. void EncoderStateFeedback::OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) { - rtc::CritScope lock(&crit_); - SsrcEncoderMap::iterator it = encoders_.find(old_ssrc); - if (it == encoders_.end() || encoders_.find(new_ssrc) != encoders_.end()) { + if (!RTC_DCHECK_IS_ON) return; - } - - ViEEncoder* encoder = it->second; - encoders_.erase(it); - encoders_[new_ssrc] = encoder; - encoder->OnLocalSsrcChanged(old_ssrc, new_ssrc); + rtc::CritScope lock(&crit_); + if (ssrcs_.empty()) // Encoder not yet attached (or detached for teardown). + return; + // SSRC shouldn't change to something we haven't already registered with the + // encoder. + RTC_DCHECK(HasSsrc(new_ssrc)); } } // namespace webrtc diff --git a/webrtc/video/encoder_state_feedback.h b/webrtc/video/encoder_state_feedback.h index d0161daf7b..6326bed9c1 100644 --- a/webrtc/video/encoder_state_feedback.h +++ b/webrtc/video/encoder_state_feedback.h @@ -14,56 +14,40 @@ #ifndef WEBRTC_VIDEO_ENCODER_STATE_FEEDBACK_H_ #define WEBRTC_VIDEO_ENCODER_STATE_FEEDBACK_H_ -#include #include -#include "webrtc/base/constructormagic.h" #include "webrtc/base/criticalsection.h" -#include "webrtc/base/scoped_ptr.h" +#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "webrtc/typedefs.h" namespace webrtc { -class EncoderStateFeedbackObserver; -class RtcpIntraFrameObserver; class ViEEncoder; -class EncoderStateFeedback { +class EncoderStateFeedback : public RtcpIntraFrameObserver { public: - friend class EncoderStateFeedbackObserver; - EncoderStateFeedback(); - ~EncoderStateFeedback(); // Adds an encoder to receive feedback for a set of SSRCs. - void AddEncoder(const std::vector& ssrc, ViEEncoder* encoder); + void Init(const std::vector& ssrc, ViEEncoder* encoder); - // Removes a registered ViEEncoder. - void RemoveEncoder(const ViEEncoder* encoder); + // Removes the registered encoder. Necessary since RTP modules outlive + // ViEEncoder. + // TODO(pbos): Make sure RTP modules are not running when tearing down + // ViEEncoder, then remove this function. + void TearDown(); - // Returns an observer to register at the requesting class. The observer has - // the same lifetime as the EncoderStateFeedback instance. - RtcpIntraFrameObserver* GetRtcpIntraFrameObserver(); - - protected: - // Called by EncoderStateFeedbackObserver when a new key frame is requested. - void OnReceivedIntraFrameRequest(uint32_t ssrc); - void OnReceivedSLI(uint32_t ssrc, uint8_t picture_id); - void OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id); - void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc); + void OnReceivedIntraFrameRequest(uint32_t ssrc) override; + void OnReceivedSLI(uint32_t ssrc, uint8_t picture_id) override; + void OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id) override; + void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) override; private: - typedef std::map SsrcEncoderMap; - + bool HasSsrc(uint32_t ssrc) EXCLUSIVE_LOCKS_REQUIRED(crit_); rtc::CriticalSection crit_; - // Instance registered at the class requesting new key frames. - rtc::scoped_ptr observer_; - - // Maps a unique ssrc to the given encoder. - SsrcEncoderMap encoders_; - - RTC_DISALLOW_COPY_AND_ASSIGN(EncoderStateFeedback); + std::vector ssrcs_ GUARDED_BY(crit_); + ViEEncoder* vie_encoder_ GUARDED_BY(crit_); }; } // namespace webrtc diff --git a/webrtc/video/encoder_state_feedback_unittest.cc b/webrtc/video/encoder_state_feedback_unittest.cc index de4616f176..be5f0420e5 100644 --- a/webrtc/video/encoder_state_feedback_unittest.cc +++ b/webrtc/video/encoder_state_feedback_unittest.cc @@ -15,13 +15,10 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" -#include "webrtc/base/scoped_ptr.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" #include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/pacing/packet_router.h" -#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "webrtc/modules/utility/include/mock/mock_process_thread.h" -#include "webrtc/video/payload_router.h" #include "webrtc/video/vie_encoder.h" using ::testing::NiceMock; @@ -51,99 +48,33 @@ class MockVieEncoder : public ViEEncoder { void(uint32_t old_ssrc, uint32_t new_ssrc)); }; -class VieKeyRequestTest : public ::testing::Test { - protected: - VieKeyRequestTest() - : pacer_(Clock::GetRealTimeClock(), - &router_, - BitrateController::kDefaultStartBitrateKbps, - PacedSender::kDefaultPaceMultiplier * - BitrateController::kDefaultStartBitrateKbps, - 0) {} - virtual void SetUp() { - process_thread_.reset(new NiceMock); - encoder_state_feedback_.reset(new EncoderStateFeedback()); - } - rtc::scoped_ptr process_thread_; - rtc::scoped_ptr encoder_state_feedback_; - PacketRouter router_; - PacedSender pacer_; -}; +TEST(VieKeyRequestTest, CreateAndTriggerRequests) { + static const uint32_t kSsrc = 1234; + NiceMock process_thread; + PacketRouter router; + PacedSender pacer(Clock::GetRealTimeClock(), &router, + BitrateController::kDefaultStartBitrateKbps, + PacedSender::kDefaultPaceMultiplier * + BitrateController::kDefaultStartBitrateKbps, + 0); + MockVieEncoder encoder(&process_thread, &pacer); -TEST_F(VieKeyRequestTest, CreateAndTriggerRequests) { - const int ssrc = 1234; - MockVieEncoder encoder(process_thread_.get(), &pacer_); - encoder_state_feedback_->AddEncoder(std::vector(1, ssrc), &encoder); + EncoderStateFeedback encoder_state_feedback; + encoder_state_feedback.Init(std::vector(1, kSsrc), &encoder); - EXPECT_CALL(encoder, OnReceivedIntraFrameRequest(ssrc)) + EXPECT_CALL(encoder, OnReceivedIntraFrameRequest(kSsrc)) .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()-> - OnReceivedIntraFrameRequest(ssrc); + encoder_state_feedback.OnReceivedIntraFrameRequest(kSsrc); const uint8_t sli_picture_id = 3; - EXPECT_CALL(encoder, OnReceivedSLI(ssrc, sli_picture_id)) + EXPECT_CALL(encoder, OnReceivedSLI(kSsrc, sli_picture_id)) .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedSLI( - ssrc, sli_picture_id); + encoder_state_feedback.OnReceivedSLI(kSsrc, sli_picture_id); const uint64_t rpsi_picture_id = 9; - EXPECT_CALL(encoder, OnReceivedRPSI(ssrc, rpsi_picture_id)) + EXPECT_CALL(encoder, OnReceivedRPSI(kSsrc, rpsi_picture_id)) .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedRPSI( - ssrc, rpsi_picture_id); - - encoder_state_feedback_->RemoveEncoder(&encoder); -} - -// Register multiple encoders and make sure the request is relayed to correct -// ViEEncoder. -TEST_F(VieKeyRequestTest, MultipleEncoders) { - const int ssrc_1 = 1234; - const int ssrc_2 = 5678; - MockVieEncoder encoder_1(process_thread_.get(), &pacer_); - MockVieEncoder encoder_2(process_thread_.get(), &pacer_); - encoder_state_feedback_->AddEncoder(std::vector(1, ssrc_1), - &encoder_1); - encoder_state_feedback_->AddEncoder(std::vector(1, ssrc_2), - &encoder_2); - - EXPECT_CALL(encoder_1, OnReceivedIntraFrameRequest(ssrc_1)) - .Times(1); - EXPECT_CALL(encoder_2, OnReceivedIntraFrameRequest(ssrc_2)) - .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()-> - OnReceivedIntraFrameRequest(ssrc_1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()-> - OnReceivedIntraFrameRequest(ssrc_2); - - const uint8_t sli_pid_1 = 3; - const uint8_t sli_pid_2 = 4; - EXPECT_CALL(encoder_1, OnReceivedSLI(ssrc_1, sli_pid_1)) - .Times(1); - EXPECT_CALL(encoder_2, OnReceivedSLI(ssrc_2, sli_pid_2)) - .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedSLI( - ssrc_1, sli_pid_1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedSLI( - ssrc_2, sli_pid_2); - - const uint64_t rpsi_pid_1 = 9; - const uint64_t rpsi_pid_2 = 10; - EXPECT_CALL(encoder_1, OnReceivedRPSI(ssrc_1, rpsi_pid_1)) - .Times(1); - EXPECT_CALL(encoder_2, OnReceivedRPSI(ssrc_2, rpsi_pid_2)) - .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedRPSI( - ssrc_1, rpsi_pid_1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()->OnReceivedRPSI( - ssrc_2, rpsi_pid_2); - - encoder_state_feedback_->RemoveEncoder(&encoder_1); - EXPECT_CALL(encoder_2, OnReceivedIntraFrameRequest(ssrc_2)) - .Times(1); - encoder_state_feedback_->GetRtcpIntraFrameObserver()-> - OnReceivedIntraFrameRequest(ssrc_2); - encoder_state_feedback_->RemoveEncoder(&encoder_2); + encoder_state_feedback.OnReceivedRPSI(kSsrc, rpsi_picture_id); } } // namespace webrtc diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 58ed646363..1be17632ed 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -176,7 +176,7 @@ VideoSendStream::VideoSendStream( module_process_thread_, &payload_router_, nullptr, - encoder_feedback_.GetRtcpIntraFrameObserver(), + &encoder_feedback_, congestion_controller_->GetBitrateController() ->CreateRtcpBandwidthObserver(), congestion_controller_->GetTransportFeedbackObserver(), @@ -209,6 +209,7 @@ VideoSendStream::VideoSendStream( RTC_DCHECK(remb_); RTC_CHECK(vie_encoder_.Init()); + encoder_feedback_.Init(config_.rtp.ssrcs, &vie_encoder_); RTC_CHECK(vie_channel_.Init() == 0); vcm_->RegisterProtectionCallback(vie_channel_.vcm_protection_callback()); @@ -288,8 +289,6 @@ VideoSendStream::VideoSendStream( if (config_.suspend_below_min_bitrate) vie_encoder_.SuspendBelowMinBitrate(); - encoder_feedback_.AddEncoder(config_.rtp.ssrcs, &vie_encoder_); - vie_channel_.RegisterSendChannelRtcpStatisticsCallback(&stats_proxy_); vie_channel_.RegisterSendChannelRtpStatisticsCallback(&stats_proxy_); vie_channel_.RegisterRtcpPacketTypeCounterObserver(&stats_proxy_); @@ -319,9 +318,9 @@ VideoSendStream::~VideoSendStream() { rtp_module->SetREMBStatus(false); remb_->RemoveRembSender(rtp_module); - // Remove the feedback, stop all encoding threads and processing. This must be - // done before deleting the channel. - encoder_feedback_.RemoveEncoder(&vie_encoder_); + // ViEChannel outlives ViEEncoder so remove encoder from feedback before + // destruction. + encoder_feedback_.TearDown(); congestion_controller_->GetRemoteBitrateEstimator(false)->RemoveStream( vie_receiver_->GetRemoteSsrc());