From f2c750deee2d3dcda2791ca3186da577b6fbf222 Mon Sep 17 00:00:00 2001 From: "mflodman@webrtc.org" Date: Mon, 24 Sep 2012 16:20:47 +0000 Subject: [PATCH] Hooking up EncoderStateFeedback to handle intra requests instead of trigger ViEEncoder directly. This is one step towards adding send- and receive only channels and getting rid of the default module. BUG=769 Review URL: https://webrtc-codereview.appspot.com/824004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@2812 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../rtp_rtcp/interface/rtp_rtcp_defines.h | 2 + src/modules/rtp_rtcp/source/rtcp_receiver.cc | 27 ++++++++--- src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 1 + .../rtp_rtcp/test/testAPI/test_api_rtcp.cc | 1 + src/video_engine/encoder_state_feedback.cc | 20 ++++++++- src/video_engine/encoder_state_feedback.h | 1 + .../encoder_state_feedback_unittest.cc | 2 + src/video_engine/vie_channel_group.cc | 8 +++- src/video_engine/vie_channel_group.h | 3 ++ src/video_engine/vie_channel_manager.cc | 45 ++++++++++++++----- src/video_engine/vie_channel_manager.h | 1 + src/video_engine/vie_encoder.cc | 3 ++ src/video_engine/vie_encoder.h | 9 ++-- 13 files changed, 97 insertions(+), 26 deletions(-) diff --git a/src/modules/rtp_rtcp/interface/rtp_rtcp_defines.h b/src/modules/rtp_rtcp/interface/rtp_rtcp_defines.h index 7399dc110..6dbf52c93 100644 --- a/src/modules/rtp_rtcp/interface/rtp_rtcp_defines.h +++ b/src/modules/rtp_rtcp/interface/rtp_rtcp_defines.h @@ -225,6 +225,8 @@ class RtcpIntraFrameObserver { virtual void OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id) = 0; + virtual void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) = 0; + virtual ~RtcpIntraFrameObserver() {} }; diff --git a/src/modules/rtp_rtcp/source/rtcp_receiver.cc b/src/modules/rtp_rtcp/source/rtcp_receiver.cc index 631cc1b0e..c734e0c76 100644 --- a/src/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/src/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -133,9 +133,19 @@ void RTCPReceiver::RegisterRtcpObservers( } -void RTCPReceiver::SetSSRC( const WebRtc_UWord32 ssrc) { +void RTCPReceiver::SetSSRC(const WebRtc_UWord32 ssrc) { + WebRtc_UWord32 old_ssrc = 0; + { CriticalSectionScoped lock(_criticalSectionRTCPReceiver); + old_ssrc = _SSRC; _SSRC = ssrc; + } + { + CriticalSectionScoped lock(_criticalSectionFeedbacks); + if (_cbRtcpIntraFrameObserver && old_ssrc != ssrc) { + _cbRtcpIntraFrameObserver->OnLocalSsrcChanged(old_ssrc, ssrc); + } + } } WebRtc_Word32 RTCPReceiver::ResetRTT(const WebRtc_UWord32 remoteSSRC) { @@ -1196,6 +1206,12 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( // Might trigger a OnReceivedBandwidthEstimateUpdate. UpdateTMMBR(); } + unsigned int local_ssrc = 0; + { + // We don't want to hold this critsect when triggering the callbacks below. + CriticalSectionScoped lock(_criticalSectionRTCPReceiver); + local_ssrc = _SSRC; + } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSrReq) { _rtpRtcp.OnRequestSendReport(); } @@ -1228,18 +1244,15 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( "SIG [RTCP] Incoming FIR from SSRC:0x%x", rtcpPacketInformation.remoteSSRC); } - _cbRtcpIntraFrameObserver->OnReceivedIntraFrameRequest( - rtcpPacketInformation.remoteSSRC); + _cbRtcpIntraFrameObserver->OnReceivedIntraFrameRequest(local_ssrc); } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSli) { _cbRtcpIntraFrameObserver->OnReceivedSLI( - rtcpPacketInformation.remoteSSRC, - rtcpPacketInformation.sliPictureId); + local_ssrc, rtcpPacketInformation.sliPictureId); } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRpsi) { _cbRtcpIntraFrameObserver->OnReceivedRPSI( - rtcpPacketInformation.remoteSSRC, - rtcpPacketInformation.rpsiPictureId); + local_ssrc, rtcpPacketInformation.rpsiPictureId); } } if (_cbRtcpBandwidthObserver) { diff --git a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 600297923..9716f8a87 100644 --- a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -106,6 +106,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) // make sure that RTCP objects are aware of our SSRC WebRtc_UWord32 SSRC = _rtpSender.SSRC(); _rtcpSender.SetSSRC(SSRC); + _rtcpReceiver.SetSSRC(SSRC); WEBRTC_TRACE(kTraceMemory, kTraceRtpRtcp, _id, "%s created", __FUNCTION__); } diff --git a/src/modules/rtp_rtcp/test/testAPI/test_api_rtcp.cc b/src/modules/rtp_rtcp/test/testAPI/test_api_rtcp.cc index ce77953cf..833f8675c 100644 --- a/src/modules/rtp_rtcp/test/testAPI/test_api_rtcp.cc +++ b/src/modules/rtp_rtcp/test/testAPI/test_api_rtcp.cc @@ -71,6 +71,7 @@ class RtcpCallback : public RtcpFeedback, public RtcpIntraFrameObserver { uint64_t pictureId) { EXPECT_EQ(kTestPictureId, pictureId); }; + virtual void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) {}; private: RtpRtcp* _rtpRtcpModule; }; diff --git a/src/video_engine/encoder_state_feedback.cc b/src/video_engine/encoder_state_feedback.cc index cd155a627..64e32e2d3 100644 --- a/src/video_engine/encoder_state_feedback.cc +++ b/src/video_engine/encoder_state_feedback.cc @@ -37,6 +37,10 @@ class EncoderStateFeedbackObserver : public RtcpIntraFrameObserver { 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_; }; @@ -51,8 +55,10 @@ EncoderStateFeedback::~EncoderStateFeedback() { bool EncoderStateFeedback::AddEncoder(uint32_t ssrc, ViEEncoder* encoder) { CriticalSectionScoped lock(crit_.get()); - if (encoders_.find(ssrc) != encoders_.end()) + if (encoders_.find(ssrc) != encoders_.end()) { + // Two encoders must not have the same ssrc. return false; + } encoders_[ssrc] = encoder; return true; @@ -98,4 +104,16 @@ void EncoderStateFeedback::OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id) { it->second->OnReceivedRPSI(ssrc, picture_id); } +void EncoderStateFeedback::OnLocalSsrcChanged(uint32_t old_ssrc, + uint32_t new_ssrc) { + CriticalSectionScoped lock(crit_.get()); + SsrcEncoderMap::iterator it = encoders_.find(old_ssrc); + if (it == encoders_.end() || encoders_.find(new_ssrc) != encoders_.end()) { + return; + } + + encoders_[new_ssrc] = it->second; + encoders_.erase(it); +} + } // namespace webrtc diff --git a/src/video_engine/encoder_state_feedback.h b/src/video_engine/encoder_state_feedback.h index 7d063d43e..45db92fba 100644 --- a/src/video_engine/encoder_state_feedback.h +++ b/src/video_engine/encoder_state_feedback.h @@ -49,6 +49,7 @@ class EncoderStateFeedback { 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); private: typedef std::map SsrcEncoderMap; diff --git a/src/video_engine/encoder_state_feedback_unittest.cc b/src/video_engine/encoder_state_feedback_unittest.cc index 3059526fa..07d1663ea 100644 --- a/src/video_engine/encoder_state_feedback_unittest.cc +++ b/src/video_engine/encoder_state_feedback_unittest.cc @@ -45,6 +45,8 @@ class MockVieEncoder : public ViEEncoder { void(uint32_t ssrc, uint8_t picture_id)); MOCK_METHOD2(OnReceivedRPSI, void(uint32_t ssrc, uint64_t picture_id)); + MOCK_METHOD2(OnLocalSsrcChanged, + void(uint32_t old_ssrc, uint32_t new_ssrc)); }; class VieKeyRequestTest : public ::testing::Test { diff --git a/src/video_engine/vie_channel_group.cc b/src/video_engine/vie_channel_group.cc index 6254c21e0..d9830def5 100644 --- a/src/video_engine/vie_channel_group.cc +++ b/src/video_engine/vie_channel_group.cc @@ -13,6 +13,7 @@ #include "modules/bitrate_controller/include/bitrate_controller.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/rtp_rtcp/interface/rtp_rtcp.h" +#include "video_engine/encoder_state_feedback.h" #include "video_engine/vie_channel.h" #include "video_engine/vie_encoder.h" #include "video_engine/vie_remb.h" @@ -25,7 +26,8 @@ ChannelGroup::ChannelGroup(ProcessThread* process_thread, : remb_(new VieRemb(process_thread)), bitrate_controller_(BitrateController::CreateBitrateController()), remote_bitrate_estimator_(RemoteBitrateEstimator::Create(remb_.get(), - options, mode)) { + options, mode)), + encoder_state_feedback_(new EncoderStateFeedback()) { } ChannelGroup::~ChannelGroup() { @@ -57,6 +59,10 @@ RemoteBitrateEstimator* ChannelGroup::GetRemoteBitrateEstimator() { return remote_bitrate_estimator_.get(); } +EncoderStateFeedback* ChannelGroup::GetEncoderStateFeedback() { + return encoder_state_feedback_.get(); +} + bool ChannelGroup::SetChannelRembStatus(int channel_id, bool sender, bool receiver, diff --git a/src/video_engine/vie_channel_group.h b/src/video_engine/vie_channel_group.h index 14b55dbcb..4b6e927c1 100644 --- a/src/video_engine/vie_channel_group.h +++ b/src/video_engine/vie_channel_group.h @@ -19,6 +19,7 @@ namespace webrtc { class BitrateController; +class EncoderStateFeedback; struct OverUseDetectorOptions; class ProcessThread; class ViEChannel; @@ -47,6 +48,7 @@ class ChannelGroup { BitrateController* GetBitrateController(); RemoteBitrateEstimator* GetRemoteBitrateEstimator(); + EncoderStateFeedback* GetEncoderStateFeedback(); private: typedef std::set ChannelSet; @@ -54,6 +56,7 @@ class ChannelGroup { scoped_ptr remb_; scoped_ptr bitrate_controller_; scoped_ptr remote_bitrate_estimator_; + scoped_ptr encoder_state_feedback_; ChannelSet channels_; }; diff --git a/src/video_engine/vie_channel_manager.cc b/src/video_engine/vie_channel_manager.cc index bb4d09fd1..fe0964b9e 100644 --- a/src/video_engine/vie_channel_manager.cc +++ b/src/video_engine/vie_channel_manager.cc @@ -16,6 +16,7 @@ #include "system_wrappers/interface/critical_section_wrapper.h" #include "system_wrappers/interface/map_wrapper.h" #include "system_wrappers/interface/trace.h" +#include "video_engine/encoder_state_feedback.h" #include "video_engine/vie_channel.h" #include "video_engine/vie_defines.h" #include "video_engine/vie_encoder.h" @@ -103,10 +104,14 @@ int ViEChannelManager::CreateChannel(int* channel_id) { bitrate_controller->CreateRtcpBandwidthObserver(); RemoteBitrateEstimator* remote_bitrate_estimator = group->GetRemoteBitrateEstimator(); + EncoderStateFeedback* encoder_state_feedback = + group->GetEncoderStateFeedback(); if (!(vie_encoder->Init() && CreateChannelObject(new_channel_id, vie_encoder, bandwidth_observer, - remote_bitrate_estimator, true))) { + remote_bitrate_estimator, + encoder_state_feedback->GetRtcpIntraFrameObserver(), + true))) { delete vie_encoder; vie_encoder = NULL; ReturnChannelId(new_channel_id); @@ -114,6 +119,11 @@ int ViEChannelManager::CreateChannel(int* channel_id) { return -1; } + // Add ViEEncoder to EncoderFeedBackObserver. + unsigned int ssrc = 0; + channel_map_[new_channel_id]->GetLocalSSRC(&ssrc); + encoder_state_feedback->AddEncoder(ssrc, vie_encoder); + *channel_id = new_channel_id; group->AddChannel(*channel_id); channel_groups_.push_back(group); @@ -141,6 +151,8 @@ int ViEChannelManager::CreateChannel(int* channel_id, bitrate_controller->CreateRtcpBandwidthObserver(); RemoteBitrateEstimator* remote_bitrate_estimator = channel_group->GetRemoteBitrateEstimator(); + EncoderStateFeedback* encoder_state_feedback = + channel_group->GetEncoderStateFeedback(); ViEEncoder* vie_encoder = NULL; if (sender) { @@ -149,18 +161,24 @@ int ViEChannelManager::CreateChannel(int* channel_id, *module_process_thread_, bitrate_controller); if (!(vie_encoder->Init() && - CreateChannelObject(new_channel_id, vie_encoder, - bandwidth_observer, - remote_bitrate_estimator, - sender))) { + CreateChannelObject( + new_channel_id, vie_encoder, bandwidth_observer, + remote_bitrate_estimator, + encoder_state_feedback->GetRtcpIntraFrameObserver(), sender))) { delete vie_encoder; vie_encoder = NULL; } + // Register the ViEEncoder to get key frame requests for this channel. + unsigned int ssrc = 0; + channel_map_[new_channel_id]->GetLocalSSRC(&ssrc); + encoder_state_feedback->AddEncoder(ssrc, vie_encoder); } else { vie_encoder = ViEEncoderPtr(original_channel); assert(vie_encoder); - if (!CreateChannelObject(new_channel_id, vie_encoder, bandwidth_observer, - remote_bitrate_estimator, sender)) { + if (!CreateChannelObject( + new_channel_id, vie_encoder, bandwidth_observer, + remote_bitrate_estimator, + encoder_state_feedback->GetRtcpIntraFrameObserver(), sender)) { vie_encoder = NULL; } } @@ -206,9 +224,13 @@ int ViEChannelManager::DeleteChannel(int channel_id) { group = FindGroup(channel_id); group->SetChannelRembStatus(channel_id, false, false, vie_channel, vie_encoder); - unsigned int ssrc = 0; - vie_channel->GetRemoteSSRC(&ssrc); - group->RemoveChannel(channel_id, ssrc); + unsigned int remote_ssrc = 0; + vie_channel->GetRemoteSSRC(&remote_ssrc); + group->RemoveChannel(channel_id, remote_ssrc); + + unsigned int local_ssrc = 0; + vie_channel->GetLocalSSRC(&local_ssrc); + group->GetEncoderStateFeedback()->RemoveEncoder(local_ssrc); // Check if other channels are using the same encoder. if (ChannelUsingViEEncoder(channel_id)) { @@ -353,6 +375,7 @@ bool ViEChannelManager::CreateChannelObject( ViEEncoder* vie_encoder, RtcpBandwidthObserver* bandwidth_observer, RemoteBitrateEstimator* remote_bitrate_estimator, + RtcpIntraFrameObserver* intra_frame_observer, bool sender) { // Register the channel at the encoder. RtpRtcp* send_rtp_rtcp_module = vie_encoder->SendRtpRtcpModule(); @@ -360,7 +383,7 @@ bool ViEChannelManager::CreateChannelObject( ViEChannel* vie_channel = new ViEChannel(channel_id, engine_id_, number_of_cores_, *module_process_thread_, - vie_encoder, + intra_frame_observer, bandwidth_observer, remote_bitrate_estimator, send_rtp_rtcp_module, diff --git a/src/video_engine/vie_channel_manager.h b/src/video_engine/vie_channel_manager.h index 758cdd31e..24097513f 100644 --- a/src/video_engine/vie_channel_manager.h +++ b/src/video_engine/vie_channel_manager.h @@ -85,6 +85,7 @@ class ViEChannelManager: private ViEManagerBase { bool CreateChannelObject(int channel_id, ViEEncoder* vie_encoder, RtcpBandwidthObserver* bandwidth_observer, RemoteBitrateEstimator* remote_bitrate_estimator, + RtcpIntraFrameObserver* intra_frame_observer, bool sender); // Used by ViEChannelScoped, forcing a manager user to use scoped. diff --git a/src/video_engine/vie_encoder.cc b/src/video_engine/vie_encoder.cc index e56b6b3fc..054ff0fc6 100644 --- a/src/video_engine/vie_encoder.cc +++ b/src/video_engine/vie_encoder.cc @@ -822,6 +822,9 @@ void ViEEncoder::OnReceivedIntraFrameRequest(uint32_t /*ssrc*/) { time_last_intra_request_ms_ = now; } +void ViEEncoder::OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) { +} + // Called from ViEBitrateObserver. void ViEEncoder::OnNetworkChanged(const uint32_t bitrate_bps, const uint8_t fraction_lost, diff --git a/src/video_engine/vie_encoder.h b/src/video_engine/vie_encoder.h index 475c23216..d68706c61 100644 --- a/src/video_engine/vie_encoder.h +++ b/src/video_engine/vie_encoder.h @@ -130,12 +130,9 @@ class ViEEncoder // Implements RtcpIntraFrameObserver. virtual void OnReceivedIntraFrameRequest(uint32_t ssrc); - - virtual void OnReceivedSLI(uint32_t ssrc, - uint8_t picture_id); - - virtual void OnReceivedRPSI(uint32_t ssrc, - uint64_t picture_id); + virtual void OnReceivedSLI(uint32_t ssrc, uint8_t picture_id); + virtual void OnReceivedRPSI(uint32_t ssrc, uint64_t picture_id); + virtual void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc); // Effect filter. WebRtc_Word32 RegisterEffectFilter(ViEEffectFilter* effect_filter);