From 86fd9ed6f9e2a38aa343db8c62764659633231fa Mon Sep 17 00:00:00 2001 From: sprang Date: Tue, 29 Sep 2015 04:45:43 -0700 Subject: [PATCH] Set RtcpSender transport at construction. BUG= Review URL: https://codereview.webrtc.org/1365043002 Cr-Commit-Position: refs/heads/master@{#10106} --- webrtc/modules/modules.gyp | 1 + .../source/rtcp_format_remb_unittest.cc | 9 ++++--- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 27 +++++-------------- webrtc/modules/rtp_rtcp/source/rtcp_sender.h | 8 +++--- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 10 +++---- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 6 ++--- .../modules/rtp_rtcp/test/testAPI/test_api.cc | 3 +++ webrtc/video/video_send_stream_tests.cc | 18 ++++++------- 8 files changed, 33 insertions(+), 49 deletions(-) diff --git a/webrtc/modules/modules.gyp b/webrtc/modules/modules.gyp index 38c1c0471a..ee5fe13f93 100644 --- a/webrtc/modules/modules.gyp +++ b/webrtc/modules/modules.gyp @@ -93,6 +93,7 @@ '<(webrtc_root)/test/test.gyp:frame_generator', '<(webrtc_root)/test/test.gyp:rtp_test_utils', '<(webrtc_root)/test/test.gyp:test_support_main', + '<(webrtc_root)/test/webrtc_test_common.gyp:webrtc_test_common', '<(webrtc_root)/tools/tools.gyp:agc_test_utils', ], 'sources': [ 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 3f0b047ef3..9652a53487 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc @@ -18,6 +18,7 @@ #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h" #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" +#include "webrtc/test/null_transport.h" #include "webrtc/typedefs.h" namespace { @@ -79,6 +80,7 @@ class RtcpFormatRembTest : public ::testing::Test { RTCPSender* rtcp_sender_; RTCPReceiver* rtcp_receiver_; TestTransport* test_transport_; + test::NullTransport null_transport_; MockRemoteBitrateObserver remote_bitrate_observer_; rtc::scoped_ptr remote_bitrate_estimator_; }; @@ -88,14 +90,13 @@ void RtcpFormatRembTest::SetUp() { configuration.audio = false; configuration.clock = system_clock_; configuration.remote_bitrate_estimator = remote_bitrate_estimator_.get(); + configuration.outgoing_transport = &null_transport_; dummy_rtp_rtcp_impl_ = new ModuleRtpRtcpImpl(configuration); - rtcp_sender_ = - new RTCPSender(false, system_clock_, receive_statistics_.get(), nullptr); rtcp_receiver_ = new RTCPReceiver(system_clock_, false, nullptr, nullptr, nullptr, nullptr, dummy_rtp_rtcp_impl_); test_transport_ = new TestTransport(rtcp_receiver_); - - EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(test_transport_)); + rtcp_sender_ = new RTCPSender(false, system_clock_, receive_statistics_.get(), + nullptr, test_transport_); } void RtcpFormatRembTest::TearDown() { diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 111757b7af..f250e29af7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -135,13 +135,12 @@ RTCPSender::RTCPSender( bool audio, Clock* clock, ReceiveStatistics* receive_statistics, - RtcpPacketTypeCounterObserver* packet_type_counter_observer) + RtcpPacketTypeCounterObserver* packet_type_counter_observer, + Transport* outgoing_transport) : audio_(audio), clock_(clock), method_(kRtcpOff), - critical_section_transport_( - CriticalSectionWrapper::CreateCriticalSection()), - cbTransport_(nullptr), + transport_(outgoing_transport), critical_section_rtcp_sender_( CriticalSectionWrapper::CreateCriticalSection()), @@ -173,6 +172,7 @@ RTCPSender::RTCPSender( packet_type_counter_observer_(packet_type_counter_observer) { memset(last_send_report_, 0, sizeof(last_send_report_)); memset(last_rtcp_time_, 0, sizeof(last_rtcp_time_)); + RTC_DCHECK(transport_ != nullptr); builders_[kRtcpSr] = &RTCPSender::BuildSR; builders_[kRtcpRr] = &RTCPSender::BuildRR; @@ -196,12 +196,6 @@ RTCPSender::RTCPSender( RTCPSender::~RTCPSender() { } -int32_t RTCPSender::RegisterSendTransport(Transport* outgoingTransport) { - CriticalSectionScoped lock(critical_section_transport_.get()); - cbTransport_ = outgoingTransport; - return 0; -} - RTCPMethod RTCPSender::Status() const { CriticalSectionScoped lock(critical_section_rtcp_sender_.get()); return method_; @@ -1115,11 +1109,8 @@ bool RTCPSender::PrepareReport(const FeedbackState& feedback_state, } int32_t RTCPSender::SendToNetwork(const uint8_t* dataBuffer, size_t length) { - CriticalSectionScoped lock(critical_section_transport_.get()); - if (cbTransport_) { - if (cbTransport_->SendRtcp(dataBuffer, length)) - return 0; - } + if (transport_->SendRtcp(dataBuffer, length)) + return 0; return -1; } @@ -1210,10 +1201,6 @@ bool RTCPSender::AllVolatileFlagsConsumed() const { } bool RTCPSender::SendFeedbackPacket(const rtcp::TransportFeedback& packet) { - CriticalSectionScoped lock(critical_section_transport_.get()); - if (!cbTransport_) - return false; - class Sender : public rtcp::RtcpPacket::PacketReadyCallback { public: Sender(Transport* transport) @@ -1226,7 +1213,7 @@ bool RTCPSender::SendFeedbackPacket(const rtcp::TransportFeedback& packet) { Transport* const transport_; bool send_failure_; - } sender(cbTransport_); + } sender(transport_); uint8_t buffer[IP_PACKET_SIZE]; return packet.BuildExternalBuffer(buffer, IP_PACKET_SIZE, &sender) && diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h index 2051102a2d..2f2798995e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h @@ -74,11 +74,10 @@ public: RTCPSender(bool audio, Clock* clock, ReceiveStatistics* receive_statistics, - RtcpPacketTypeCounterObserver* packet_type_counter_observer); + RtcpPacketTypeCounterObserver* packet_type_counter_observer, + Transport* outgoing_transport); virtual ~RTCPSender(); - int32_t RegisterSendTransport(Transport* outgoingTransport); - RTCPMethod Status() const; void SetRTCPStatus(RTCPMethod method); @@ -228,8 +227,7 @@ private: Clock* const clock_; RTCPMethod method_ GUARDED_BY(critical_section_rtcp_sender_); - rtc::scoped_ptr critical_section_transport_; - Transport* cbTransport_ GUARDED_BY(critical_section_transport_); + Transport* const transport_; rtc::scoped_ptr critical_section_rtcp_sender_; bool using_nack_ GUARDED_BY(critical_section_rtcp_sender_); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 33c3f84236..19dbc4dcf4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -230,11 +230,10 @@ class RtcpSenderTest : public ::testing::Test { configuration.outgoing_transport = &test_transport_; rtp_rtcp_impl_.reset(new ModuleRtpRtcpImpl(configuration)); - rtcp_sender_.reset( - new RTCPSender(false, &clock_, receive_statistics_.get(), nullptr)); + rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, &test_transport_)); rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); - EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(&test_transport_)); } void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { @@ -667,10 +666,9 @@ TEST_F(RtcpSenderTest, TestSendTimeOfXrRrtr) { TEST_F(RtcpSenderTest, TestRegisterRtcpPacketTypeObserver) { RtcpPacketTypeCounterObserverImpl observer; - rtcp_sender_.reset( - new RTCPSender(false, &clock_, receive_statistics_.get(), &observer)); + rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + &observer, &test_transport_)); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); - EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(&test_transport_)); rtcp_sender_->SetRTCPStatus(kRtcpNonCompound); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli)); EXPECT_EQ(1, parser()->pli()->num_packets()); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index ae4caf7bde..2e6545251d 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -72,7 +72,8 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) rtcp_sender_(configuration.audio, configuration.clock, configuration.receive_statistics, - configuration.rtcp_packet_type_counter_observer), + configuration.rtcp_packet_type_counter_observer, + configuration.outgoing_transport), rtcp_receiver_(configuration.clock, configuration.receiver_only, configuration.rtcp_packet_type_counter_observer, @@ -99,9 +100,6 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) rtt_ms_(0) { send_video_codec_.codecType = kVideoCodecUnknown; - // TODO(pwestin) move to constructors of each rtp/rtcp sender/receiver object. - rtcp_sender_.RegisterSendTransport(configuration.outgoing_transport); - // Make sure that RTCP objects are aware of our SSRC. uint32_t SSRC = rtp_sender_.SSRC(); rtcp_sender_.SetSSRC(SSRC); diff --git a/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc b/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc index 1540e5542e..f75113afe4 100644 --- a/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc +++ b/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc @@ -9,6 +9,7 @@ */ #include "webrtc/modules/rtp_rtcp/test/testAPI/test_api.h" +#include "webrtc/test/null_transport.h" #include #include @@ -90,6 +91,7 @@ class RtpRtcpAPITest : public ::testing::Test { RtpRtcp::Configuration configuration; configuration.audio = true; configuration.clock = &fake_clock_; + configuration.outgoing_transport = &null_transport_; module_.reset(RtpRtcp::CreateRtpRtcp(configuration)); rtp_payload_registry_.reset(new RTPPayloadRegistry( RTPPayloadStrategy::CreateStrategy(true))); @@ -105,6 +107,7 @@ class RtpRtcpAPITest : public ::testing::Test { uint16_t test_sequence_number_; std::vector test_csrcs_; SimulatedClock fake_clock_; + test::NullTransport null_transport_; }; TEST_F(RtpRtcpAPITest, Basic) { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 688278eadc..f4fabdeb7f 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -329,8 +329,8 @@ TEST_F(VideoSendStreamTest, SupportsFec) { FakeReceiveStatistics lossy_receive_stats( kSendSsrcs[0], header.sequenceNumber, send_count_ / 2, 127); RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), - &lossy_receive_stats, nullptr); - EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + &lossy_receive_stats, nullptr, + &transport_adapter_); rtcp_sender.SetRTCPStatus(kRtcpNonCompound); rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]); @@ -411,8 +411,7 @@ void VideoSendStreamTest::TestNackRetransmission( nacked_sequence_number_ = nack_sequence_number; NullReceiveStatistics null_stats; RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), &null_stats, - nullptr); - EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + nullptr, &transport_adapter_); rtcp_sender.SetRTCPStatus(kRtcpNonCompound); rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]); @@ -598,8 +597,8 @@ void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, FakeReceiveStatistics lossy_receive_stats( kSendSsrcs[0], header.sequenceNumber, packet_count_ / 2, 127); RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), - &lossy_receive_stats, nullptr); - EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + &lossy_receive_stats, nullptr, + &transport_adapter_); rtcp_sender.SetRTCPStatus(kRtcpNonCompound); rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]); @@ -828,8 +827,8 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { EXCLUSIVE_LOCKS_REQUIRED(crit_) { FakeReceiveStatistics receive_stats( kSendSsrcs[0], last_sequence_number_, rtp_count_, 0); - RTCPSender rtcp_sender(false, clock_, &receive_stats, nullptr); - EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + RTCPSender rtcp_sender(false, clock_, &receive_stats, nullptr, + &transport_adapter_); rtcp_sender.SetRTCPStatus(kRtcpNonCompound); rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]); @@ -888,8 +887,7 @@ TEST_F(VideoSendStreamTest, NoPaddingWhenVideoIsMuted) { // Receive statistics reporting having lost 50% of the packets. FakeReceiveStatistics receive_stats(kSendSsrcs[0], 1, 1, 0); RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), &receive_stats, - nullptr); - EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + nullptr, &transport_adapter_); rtcp_sender.SetRTCPStatus(kRtcpNonCompound); rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]);