From 49ff40e32e408bc77e8c9bec6090f6aa2e445173 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Thu, 13 Nov 2014 14:42:37 +0000 Subject: [PATCH] Make SetREMBData accept vector of SSRCs. BUG= R=pbos@webrtc.org, stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/32049004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7697 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h | 3 +- webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 4 +-- .../source/rtcp_format_remb_unittest.cc | 7 ++-- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 32 +++++------------ webrtc/modules/rtp_rtcp/source/rtcp_sender.h | 7 ++-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 ++- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 +- webrtc/video/rampup_tests.cc | 6 ++-- webrtc/video/video_send_stream_tests.cc | 5 +-- webrtc/video_engine/vie_remb.cc | 3 +- webrtc/video_engine/vie_remb_unittest.cc | 34 +++++++++---------- 11 files changed, 43 insertions(+), 66 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h index 7b0a4f8a4..e117b234f 100644 --- a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h @@ -540,8 +540,7 @@ class RtpRtcp : public Module { virtual int32_t SetREMBStatus(const bool enable) = 0; virtual int32_t SetREMBData(const uint32_t bitrate, - const uint8_t numberOfSSRC, - const uint32_t* SSRC) = 0; + const std::vector& ssrcs) = 0; /* * (IJ) Extended jitter report. diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 2caa631f6..7e187c75a 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -196,8 +196,8 @@ class MockRtpRtcp : public RtpRtcp { bool()); MOCK_METHOD1(SetREMBStatus, int32_t(const bool enable)); - MOCK_METHOD3(SetREMBData, - int32_t(const uint32_t bitrate, const uint8_t numberOfSSRC, const uint32_t* SSRC)); + MOCK_METHOD2(SetREMBData, + int32_t(const uint32_t bitrate, const std::vector& ssrcs)); MOCK_CONST_METHOD0(IJ, bool()); MOCK_METHOD1(SetIJStatus, diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc index c055fd487..17671b70c 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc @@ -116,13 +116,13 @@ TEST_F(RtcpFormatRembTest, TestBasicAPI) { EXPECT_EQ(0, rtcp_sender_->SetREMBStatus(false)); EXPECT_FALSE(rtcp_sender_->REMB()); - EXPECT_EQ(0, rtcp_sender_->SetREMBData(1234, 0, NULL)); + EXPECT_EQ(0, rtcp_sender_->SetREMBData(1234, std::vector())); } TEST_F(RtcpFormatRembTest, TestNonCompund) { uint32_t SSRC = 456789; EXPECT_EQ(0, rtcp_sender_->SetRTCPStatus(kRtcpNonCompound)); - EXPECT_EQ(0, rtcp_sender_->SetREMBData(1234, 1, &SSRC)); + EXPECT_EQ(0, rtcp_sender_->SetREMBData(1234, std::vector(1, SSRC))); RTCPSender::FeedbackState feedback_state = dummy_rtp_rtcp_impl_->GetFeedbackState(); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state, kRtcpRemb)); @@ -131,7 +131,8 @@ TEST_F(RtcpFormatRembTest, TestNonCompund) { TEST_F(RtcpFormatRembTest, TestCompund) { uint32_t SSRCs[2] = {456789, 98765}; EXPECT_EQ(0, rtcp_sender_->SetRTCPStatus(kRtcpCompound)); - EXPECT_EQ(0, rtcp_sender_->SetREMBData(1234, 2, SSRCs)); + EXPECT_EQ(0, rtcp_sender_->SetREMBData( + 1234, std::vector(SSRCs, SSRCs + 2))); RTCPSender::FeedbackState feedback_state = dummy_rtp_rtcp_impl_->GetFeedbackState(); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state, kRtcpRemb)); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index afe801955..1752ab952 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -120,9 +120,6 @@ RTCPSender::RTCPSender(const int32_t id, _sequenceNumberFIR(0), - _lengthRembSSRC(0), - _sizeRembSSRC(0), - _rembSSRC(NULL), _rembBitrate(0), _tmmbrHelp(), @@ -145,7 +142,6 @@ RTCPSender::RTCPSender(const int32_t id, } RTCPSender::~RTCPSender() { - delete [] _rembSSRC; delete [] _appData; while (!internal_report_blocks_.empty()) { @@ -251,24 +247,12 @@ RTCPSender::SetREMBStatus(const bool enable) int32_t RTCPSender::SetREMBData(const uint32_t bitrate, - const uint8_t numberOfSSRC, - const uint32_t* SSRC) + const std::vector& ssrcs) { CriticalSectionScoped lock(_criticalSectionRTCPSender); _rembBitrate = bitrate; + remb_ssrcs_ = ssrcs; - if(_sizeRembSSRC < numberOfSSRC) - { - delete [] _rembSSRC; - _rembSSRC = new uint32_t[numberOfSSRC]; - _sizeRembSSRC = numberOfSSRC; - } - - _lengthRembSSRC = numberOfSSRC; - for (int i = 0; i < numberOfSSRC; i++) - { - _rembSSRC[i] = SSRC[i]; - } _sendREMB = true; // Send a REMB immediately if we have a new REMB. The frequency of REMBs is // throttled by the caller. @@ -1048,7 +1032,7 @@ int32_t RTCPSender::BuildREMB(uint8_t* rtcpbuffer, int& pos) { // sanity - if(pos + 20 + 4 * _lengthRembSSRC >= IP_PACKET_SIZE) + if(pos + 20 + 4 * remb_ssrcs_.size() >= IP_PACKET_SIZE) { return -2; } @@ -1058,7 +1042,7 @@ RTCPSender::BuildREMB(uint8_t* rtcpbuffer, int& pos) rtcpbuffer[pos++]=(uint8_t)206; rtcpbuffer[pos++]=(uint8_t)0; - rtcpbuffer[pos++]=_lengthRembSSRC + 4; + rtcpbuffer[pos++]=remb_ssrcs_.size() + 4; // Add our own SSRC RtpUtility::AssignUWord32ToBuffer(rtcpbuffer + pos, _SSRC); @@ -1073,7 +1057,7 @@ RTCPSender::BuildREMB(uint8_t* rtcpbuffer, int& pos) rtcpbuffer[pos++]='M'; rtcpbuffer[pos++]='B'; - rtcpbuffer[pos++] = _lengthRembSSRC; + rtcpbuffer[pos++] = remb_ssrcs_.size(); // 6 bit Exp // 18 bit mantissa uint8_t brExp = 0; @@ -1090,10 +1074,10 @@ RTCPSender::BuildREMB(uint8_t* rtcpbuffer, int& pos) rtcpbuffer[pos++]=(uint8_t)(brMantissa >> 8); rtcpbuffer[pos++]=(uint8_t)(brMantissa); - for (int i = 0; i < _lengthRembSSRC; i++) + for (size_t i = 0; i < remb_ssrcs_.size(); i++) { - RtpUtility::AssignUWord32ToBuffer(rtcpbuffer + pos, _rembSSRC[i]); - pos += 4; + RtpUtility::AssignUWord32ToBuffer(rtcpbuffer + pos, remb_ssrcs_[i]); + pos += 4; } return 0; } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h index 06c373b6c..668c7c772 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h @@ -133,8 +133,7 @@ public: int32_t SetREMBStatus(const bool enable); int32_t SetREMBData(const uint32_t bitrate, - const uint8_t numberOfSSRC, - const uint32_t* SSRC); + const std::vector& ssrcs); /* * TMMBR @@ -334,10 +333,8 @@ private: uint8_t _sequenceNumberFIR GUARDED_BY(_criticalSectionRTCPSender); // REMB - uint8_t _lengthRembSSRC GUARDED_BY(_criticalSectionRTCPSender); - uint8_t _sizeRembSSRC GUARDED_BY(_criticalSectionRTCPSender); - uint32_t* _rembSSRC GUARDED_BY(_criticalSectionRTCPSender); uint32_t _rembBitrate GUARDED_BY(_criticalSectionRTCPSender); + std::vector remb_ssrcs_ GUARDED_BY(_criticalSectionRTCPSender); TMMBRHelp _tmmbrHelp GUARDED_BY(_criticalSectionRTCPSender); uint32_t _tmmbr_Send GUARDED_BY(_criticalSectionRTCPSender); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 69b5bb424..f8713e275 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -873,9 +873,8 @@ int32_t ModuleRtpRtcpImpl::SetREMBStatus(const bool enable) { } int32_t ModuleRtpRtcpImpl::SetREMBData(const uint32_t bitrate, - const uint8_t number_of_ssrc, - const uint32_t* ssrc) { - return rtcp_sender_.SetREMBData(bitrate, number_of_ssrc, ssrc); + const std::vector& ssrcs) { + return rtcp_sender_.SetREMBData(bitrate, ssrcs); } // (IJ) Extended jitter report. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index a015a3e38..497581280 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -205,8 +205,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp { virtual int32_t SetREMBStatus(const bool enable) OVERRIDE; virtual int32_t SetREMBData(const uint32_t bitrate, - const uint8_t number_of_ssrc, - const uint32_t* ssrc) OVERRIDE; + const std::vector& ssrcs) OVERRIDE; // (IJ) Extended jitter report. virtual bool IJ() const OVERRIDE; diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc index 011ef6af5..323870401 100644 --- a/webrtc/video/rampup_tests.cc +++ b/webrtc/video/rampup_tests.cc @@ -108,8 +108,7 @@ void StreamObserver::OnReceiveBitrateChanged( TriggerTestDone(); } } - rtp_rtcp_->SetREMBData( - bitrate, static_cast(ssrcs.size()), &ssrcs[0]); + rtp_rtcp_->SetREMBData(bitrate, ssrcs); rtp_rtcp_->Process(); } @@ -235,8 +234,7 @@ void LowRateStreamObserver::OnReceiveBitrateChanged( const std::vector& ssrcs, unsigned int bitrate) { CriticalSectionScoped lock(crit_.get()); - rtp_rtcp_->SetREMBData( - bitrate, static_cast(ssrcs.size()), &ssrcs[0]); + rtp_rtcp_->SetREMBData(bitrate, ssrcs); rtp_rtcp_->Process(); last_remb_bps_ = bitrate; } diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index b5f5d8851..8aae7031c 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -829,7 +829,7 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]); if (remb_value > 0) { rtcp_sender.SetREMBStatus(true); - rtcp_sender.SetREMBData(remb_value, 0, NULL); + rtcp_sender.SetREMBData(remb_value, std::vector()); } RTCPSender::FeedbackState feedback_state; EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr)); @@ -1059,7 +1059,8 @@ TEST_F(VideoSendStreamTest, MinTransmitBitrateRespectsRemb) { "bps", false); if (total_bitrate_bps > kHighBitrateBps) { - rtp_rtcp_->SetREMBData(kRembBitrateBps, 1, &header.ssrc); + rtp_rtcp_->SetREMBData(kRembBitrateBps, + std::vector(1, header.ssrc)); rtp_rtcp_->Process(); bitrate_capped_ = true; } else if (bitrate_capped_ && diff --git a/webrtc/video_engine/vie_remb.cc b/webrtc/video_engine/vie_remb.cc index d04f0a30a..2cf794c5d 100644 --- a/webrtc/video_engine/vie_remb.cc +++ b/webrtc/video_engine/vie_remb.cc @@ -136,8 +136,7 @@ void VieRemb::OnReceiveBitrateChanged(const std::vector& ssrcs, list_crit_->Leave(); if (sender) { - // TODO(holmer): Change RTP module API to take a const vector reference. - sender->SetREMBData(bitrate_, ssrcs.size(), &ssrcs[0]); + sender->SetREMBData(bitrate_, ssrcs); } } diff --git a/webrtc/video_engine/vie_remb_unittest.cc b/webrtc/video_engine/vie_remb_unittest.cc index 1f0b70c51..272833ddc 100644 --- a/webrtc/video_engine/vie_remb_unittest.cc +++ b/webrtc/video_engine/vie_remb_unittest.cc @@ -53,12 +53,12 @@ TEST_F(ViERembTest, OneModuleTestForSendingRemb) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, 1, _)) + EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Lower bitrate to send another REMB packet. - EXPECT_CALL(rtp, SetREMBData(bitrate_estimate - 100, 1, _)) + EXPECT_CALL(rtp, SetREMBData(bitrate_estimate - 100, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100); @@ -78,14 +78,14 @@ TEST_F(ViERembTest, LowerEstimateToSendRemb) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, 1, _)) + EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Lower the estimate with more than 3% to trigger a call to SetREMBData right // away. bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, 1, _)) + EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); } @@ -104,7 +104,7 @@ TEST_F(ViERembTest, VerifyIncreasingAndDecreasing) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]); // Call OnReceiveBitrateChanged twice to get a first estimate. - EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate[0], 2, _)) + EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate[0], ssrcs)) .Times(1); TickTime::AdvanceFakeClock(1000); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]); @@ -112,7 +112,7 @@ TEST_F(ViERembTest, VerifyIncreasingAndDecreasing) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1] + 100); // Lower the estimate to trigger a callback. - EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate[1], 2, _)) + EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate[1], ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1]); @@ -135,17 +135,17 @@ TEST_F(ViERembTest, NoRembForIncreasedBitrate) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, 2, _)) + EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Increased estimate shouldn't trigger a callback right away. - EXPECT_CALL(rtp_0, SetREMBData(_, _, _)) + EXPECT_CALL(rtp_0, SetREMBData(_, _)) .Times(0); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate + 1); // Decreasing the estimate less than 3% shouldn't trigger a new callback. - EXPECT_CALL(rtp_0, SetREMBData(_, _, _)) + EXPECT_CALL(rtp_0, SetREMBData(_, _)) .Times(0); int lower_estimate = bitrate_estimate * 98 / 100; vie_remb_->OnReceiveBitrateChanged(ssrcs, lower_estimate); @@ -169,13 +169,13 @@ TEST_F(ViERembTest, ChangeSendRtpModule) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, 2, _)) + EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Decrease estimate to trigger a REMB. bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, 2, _)) + EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); @@ -186,7 +186,7 @@ TEST_F(ViERembTest, ChangeSendRtpModule) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp_1, SetREMBData(bitrate_estimate, 2, _)) + EXPECT_CALL(rtp_1, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); @@ -205,18 +205,18 @@ TEST_F(ViERembTest, OnlyOneRembForDoubleProcess) { vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp, SetREMBData(_, _, _)) + EXPECT_CALL(rtp, SetREMBData(_, _)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Lower the estimate, should trigger a call to SetREMBData right away. bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, 1, _)) + EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged again, this should not trigger a new callback. - EXPECT_CALL(rtp, SetREMBData(_, _, _)) + EXPECT_CALL(rtp, SetREMBData(_, _)) .Times(0); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); vie_remb_->RemoveReceiveChannel(&rtp); @@ -237,13 +237,13 @@ TEST_F(ViERembTest, NoSendingRtpModule) { // Call OnReceiveBitrateChanged twice to get a first estimate. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(rtp, SetREMBData(_, _, _)) + EXPECT_CALL(rtp, SetREMBData(_, _)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Lower the estimate to trigger a new packet REMB packet. bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp, SetREMBData(_, _, _)) + EXPECT_CALL(rtp, SetREMBData(_, _)) .Times(1); vie_remb_->OnReceiveBitrateChanged(ssrcs, bitrate_estimate); }