From 24bd364d3e006c8e4d3a3f97b0bcc78f50d6b7cd Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Fri, 2 May 2014 12:35:37 +0000 Subject: [PATCH] Change GetEstimatedSend/RecvBandwidth to return the total bandwidth of a channel group instead of splitting it up among channels. This fixes an issue where the user doesn't know which channels are "active" and therefore can't properly sum the estimates for all channels. R=pbos@webrtc.org Review URL: https://webrtc-codereview.appspot.com/12469004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6041 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../auto_test/automated/vie_network_test.cc | 21 +++++++++--- webrtc/video_engine/vie_channel.cc | 5 --- webrtc/video_engine/vie_channel.h | 1 - webrtc/video_engine/vie_channel_manager.cc | 33 +++++++++++++++++-- webrtc/video_engine/vie_channel_manager.h | 7 +++- webrtc/video_engine/vie_encoder.cc | 8 ----- webrtc/video_engine/vie_encoder.h | 2 -- webrtc/video_engine/vie_receiver.cc | 16 --------- webrtc/video_engine/vie_receiver.h | 2 -- webrtc/video_engine/vie_rtp_rtcp_impl.cc | 25 +++----------- 10 files changed, 59 insertions(+), 61 deletions(-) diff --git a/webrtc/video_engine/test/auto_test/automated/vie_network_test.cc b/webrtc/video_engine/test/auto_test/automated/vie_network_test.cc index efb9013d7..c7e441921 100644 --- a/webrtc/video_engine/test/auto_test/automated/vie_network_test.cc +++ b/webrtc/video_engine/test/auto_test/automated/vie_network_test.cc @@ -112,10 +112,6 @@ class ViENetworkTest : public testing::Test { } virtual void TearDown() { - unsigned int bandwidth = 0; - EXPECT_EQ(0, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, - &bandwidth)); - EXPECT_EQ(bandwidth, 0u); EXPECT_EQ(0, vie_.network->DeregisterSendTransport(channel_)); } @@ -156,6 +152,9 @@ TEST_F(ViENetworkTest, ReceiveBWEPacket_NoExtension) { webrtc::SleepMs(kIntervalMs); } EXPECT_FALSE(transport.FindREMBFor(kSsrc1, 0.0)); + unsigned int bandwidth = 0; + EXPECT_EQ(-1, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, + &bandwidth)); } TEST_F(ViENetworkTest, ReceiveBWEPacket_TOF) { @@ -173,6 +172,9 @@ TEST_F(ViENetworkTest, ReceiveBWEPacket_TOF) { webrtc::SleepMs(kIntervalMs); } EXPECT_FALSE(transport.FindREMBFor(kSsrc1, 0.0)); + unsigned int bandwidth = 0; + EXPECT_EQ(-1, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, + &bandwidth)); } TEST_F(ViENetworkTest, ReceiveBWEPacket_AST) { @@ -180,6 +182,10 @@ TEST_F(ViENetworkTest, ReceiveBWEPacket_AST) { 1)); ReceiveASTPacketsForBWE(); EXPECT_TRUE(transport.FindREMBFor(kSsrc1, 100000.0)); + unsigned int bandwidth = 0; + EXPECT_EQ(0, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, + &bandwidth)); + EXPECT_GT(bandwidth, 0u); } TEST_F(ViENetworkTest, ReceiveBWEPacket_ASTx2) { @@ -202,6 +208,10 @@ TEST_F(ViENetworkTest, ReceiveBWEPacket_ASTx2) { } EXPECT_TRUE(transport.FindREMBFor(kSsrc1, 200000.0)); EXPECT_TRUE(transport.FindREMBFor(kSsrc2, 200000.0)); + unsigned int bandwidth = 0; + EXPECT_EQ(0, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, + &bandwidth)); + EXPECT_GT(bandwidth, 0u); } TEST_F(ViENetworkTest, ReceiveBWEPacket_AST_DisabledReceive) { @@ -209,5 +219,8 @@ TEST_F(ViENetworkTest, ReceiveBWEPacket_AST_DisabledReceive) { 1)); ReceiveASTPacketsForBWE(); EXPECT_FALSE(transport.FindREMBFor(kSsrc1, 0.0)); + unsigned int bandwidth = 0; + EXPECT_EQ(-1, vie_.rtp_rtcp->GetEstimatedReceiveBandwidth(channel_, + &bandwidth)); } } // namespace diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index d5e08787f..159236f4a 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -1187,11 +1187,6 @@ void ViEChannel::RegisterSendBitrateObserver( } } -void ViEChannel::GetEstimatedReceiveBandwidth( - uint32_t* estimated_bandwidth) const { - vie_receiver_.EstimatedReceiveBandwidth(estimated_bandwidth); -} - void ViEChannel::GetReceiveBandwidthEstimatorStats( ReceiveBandwidthEstimatorStats* output) const { vie_receiver_.GetReceiveBandwidthEstimatorStats(output); diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index 3233dbff9..5653224da 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -210,7 +210,6 @@ class ViEChannel uint32_t* fec_bitrate_sent, uint32_t* nackBitrateSent) const; bool GetSendSideDelay(int* avg_send_delay, int* max_send_delay) const; - void GetEstimatedReceiveBandwidth(uint32_t* estimated_bandwidth) const; void GetReceiveBandwidthEstimatorStats( ReceiveBandwidthEstimatorStats* output) const; diff --git a/webrtc/video_engine/vie_channel_manager.cc b/webrtc/video_engine/vie_channel_manager.cc index b2591f4b4..bbb7f19b6 100644 --- a/webrtc/video_engine/vie_channel_manager.cc +++ b/webrtc/video_engine/vie_channel_manager.cc @@ -396,6 +396,35 @@ bool ViEChannelManager::SetBandwidthEstimationConfig( return true; } +bool ViEChannelManager::GetEstimatedSendBandwidth( + int channel_id, uint32_t* estimated_bandwidth) const { + CriticalSectionScoped cs(channel_id_critsect_); + ChannelGroup* group = FindGroup(channel_id); + if (!group) { + return false; + } + group->GetBitrateController()->AvailableBandwidth(estimated_bandwidth); + return true; +} + +bool ViEChannelManager::GetEstimatedReceiveBandwidth( + int channel_id, uint32_t* estimated_bandwidth) const { + CriticalSectionScoped cs(channel_id_critsect_); + ChannelGroup* group = FindGroup(channel_id); + if (!group) { + return false; + } + std::vector ssrcs; + if (!group->GetRemoteBitrateEstimator()->LatestEstimate( + &ssrcs, estimated_bandwidth)) { + return false; + } + if (ssrcs.empty()) { + *estimated_bandwidth = 0; + } + return true; +} + bool ViEChannelManager::CreateChannelObject( int channel_id, ViEEncoder* vie_encoder, @@ -479,8 +508,8 @@ void ViEChannelManager::ReturnChannelId(int channel_id) { free_channel_ids_[channel_id - kViEChannelIdBase] = true; } -ChannelGroup* ViEChannelManager::FindGroup(int channel_id) { - for (ChannelGroups::iterator it = channel_groups_.begin(); +ChannelGroup* ViEChannelManager::FindGroup(int channel_id) const { + for (ChannelGroups::const_iterator it = channel_groups_.begin(); it != channel_groups_.end(); ++it) { if ((*it)->HasChannel(channel_id)) { return *it; diff --git a/webrtc/video_engine/vie_channel_manager.h b/webrtc/video_engine/vie_channel_manager.h index 240f1b197..2112dacb2 100644 --- a/webrtc/video_engine/vie_channel_manager.h +++ b/webrtc/video_engine/vie_channel_manager.h @@ -86,6 +86,11 @@ class ViEChannelManager: private ViEManagerBase { bool SetBandwidthEstimationConfig(int channel_id, const webrtc::Config& config); + bool GetEstimatedSendBandwidth(int channel_id, + uint32_t* estimated_bandwidth) const; + bool GetEstimatedReceiveBandwidth(int channel_id, + uint32_t* estimated_bandwidth) const; + private: // Creates a channel object connected to |vie_encoder|. Assumed to be called // protected. @@ -112,7 +117,7 @@ class ViEChannelManager: private ViEManagerBase { void ReturnChannelId(int channel_id); // Returns the iterator to the ChannelGroup containing |channel_id|. - ChannelGroup* FindGroup(int channel_id); + ChannelGroup* FindGroup(int channel_id) const; // Returns true if at least one other channels uses the same ViEEncoder as // channel_id. diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index 957407daa..afb6d0c6e 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -608,14 +608,6 @@ int32_t ViEEncoder::PacerQueuingDelayMs() const { return paced_sender_->QueueInMs(); } -int32_t ViEEncoder::EstimatedSendBandwidth( - uint32_t* available_bandwidth) const { - if (!bitrate_controller_->AvailableBandwidth(available_bandwidth)) { - return -1; - } - return 0; -} - int ViEEncoder::CodecTargetBitrate(uint32_t* bitrate) const { if (vcm_.Bitrate(bitrate) != 0) return -1; diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index 8e22ecf47..3eb5f55a7 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -111,8 +111,6 @@ class ViEEncoder uint32_t* num_delta_frames); int PacerQueuingDelayMs() const; - int32_t EstimatedSendBandwidth( - uint32_t* available_bandwidth) const; int CodecTargetBitrate(uint32_t* bitrate) const; // Loss protection. diff --git a/webrtc/video_engine/vie_receiver.cc b/webrtc/video_engine/vie_receiver.cc index 76228b3af..34cade1fa 100644 --- a/webrtc/video_engine/vie_receiver.cc +++ b/webrtc/video_engine/vie_receiver.cc @@ -460,22 +460,6 @@ int ViEReceiver::StopRTPDump() { return 0; } -// TODO(holmer): To be moved to ViEChannelGroup. -void ViEReceiver::EstimatedReceiveBandwidth( - unsigned int* available_bandwidth) const { - std::vector ssrcs; - - // LatestEstimate returns an error if there is no valid bitrate estimate, but - // ViEReceiver instead returns a zero estimate. - remote_bitrate_estimator_->LatestEstimate(&ssrcs, available_bandwidth); - if (std::find(ssrcs.begin(), ssrcs.end(), rtp_receiver_->SSRC()) != - ssrcs.end()) { - *available_bandwidth /= ssrcs.size(); - } else { - *available_bandwidth = 0; - } -} - void ViEReceiver::GetReceiveBandwidthEstimatorStats( ReceiveBandwidthEstimatorStats* output) const { remote_bitrate_estimator_->GetStats(output); diff --git a/webrtc/video_engine/vie_receiver.h b/webrtc/video_engine/vie_receiver.h index 512a7b656..c85975c56 100644 --- a/webrtc/video_engine/vie_receiver.h +++ b/webrtc/video_engine/vie_receiver.h @@ -85,8 +85,6 @@ class ViEReceiver : public RtpData { const uint16_t payload_size, const WebRtcRTPHeader* rtp_header); - void EstimatedReceiveBandwidth(unsigned int* available_bandwidth) const; - void GetReceiveBandwidthEstimatorStats( ReceiveBandwidthEstimatorStats* output) const; diff --git a/webrtc/video_engine/vie_rtp_rtcp_impl.cc b/webrtc/video_engine/vie_rtp_rtcp_impl.cc index be7cc65e2..5f0d65f2a 100644 --- a/webrtc/video_engine/vie_rtp_rtcp_impl.cc +++ b/webrtc/video_engine/vie_rtp_rtcp_impl.cc @@ -1008,18 +1008,11 @@ int ViERTP_RTCPImpl::GetEstimatedSendBandwidth( WEBRTC_TRACE(kTraceApiCall, kTraceVideo, ViEId(shared_data_->instance_id(), video_channel), "%s(channel: %d)", __FUNCTION__, video_channel); - ViEChannelManagerScoped cs(*(shared_data_->channel_manager())); - ViEEncoder* vie_encoder = cs.Encoder(video_channel); - if (!vie_encoder) { - WEBRTC_TRACE(kTraceError, kTraceVideo, - ViEId(shared_data_->instance_id(), video_channel), - "%s: Could not get encoder for channel %d", __FUNCTION__, - video_channel); - shared_data_->SetLastError(kViERtpRtcpInvalidChannelId); + if (!shared_data_->channel_manager()->GetEstimatedSendBandwidth( + video_channel, estimated_bandwidth)) { return -1; } - return vie_encoder->EstimatedSendBandwidth( - static_cast(estimated_bandwidth)); + return 0; } int ViERTP_RTCPImpl::GetEstimatedReceiveBandwidth( @@ -1028,18 +1021,10 @@ int ViERTP_RTCPImpl::GetEstimatedReceiveBandwidth( WEBRTC_TRACE(kTraceApiCall, kTraceVideo, ViEId(shared_data_->instance_id(), video_channel), "%s(channel: %d)", __FUNCTION__, video_channel); - ViEChannelManagerScoped cs(*(shared_data_->channel_manager())); - ViEChannel* vie_channel = cs.Channel(video_channel); - if (!vie_channel) { - WEBRTC_TRACE(kTraceError, kTraceVideo, - ViEId(shared_data_->instance_id(), video_channel), - "%s: Could not get channel %d", __FUNCTION__, - video_channel); - shared_data_->SetLastError(kViERtpRtcpInvalidChannelId); + if (!shared_data_->channel_manager()->GetEstimatedReceiveBandwidth( + video_channel, estimated_bandwidth)) { return -1; } - vie_channel->GetEstimatedReceiveBandwidth( - static_cast(estimated_bandwidth)); return 0; }