From 180e516bef1f2929ef22bc7324861cfe18227ed2 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Fri, 11 Jul 2014 15:36:26 +0000 Subject: [PATCH] Thread annotate RTCPSender. Also fixes data races in RTCPSender::SetCSRCStatus() and RTCPSender::SetStartTimestamp(). BUG= R=tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/20919004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6666 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../source/rtcp_format_remb_unittest.cc | 1 - webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 67 +------ webrtc/modules/rtp_rtcp/source/rtcp_sender.h | 169 ++++++++++-------- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 1 - .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +- 5 files changed, 100 insertions(+), 143 deletions(-) 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 88463e471..0514277f5 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc @@ -97,7 +97,6 @@ void RtcpFormatRembTest::SetUp() { rtcp_receiver_ = new RTCPReceiver(0, system_clock_, dummy_rtp_rtcp_impl_); test_transport_ = new TestTransport(rtcp_receiver_); - EXPECT_EQ(0, rtcp_sender_->Init()); EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(test_transport_)); } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index fa129abe0..2cf7e1cbc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -187,61 +187,6 @@ RTCPSender::~RTCPSender() { delete _criticalSectionRTCPSender; } -int32_t -RTCPSender::Init() -{ - CriticalSectionScoped lock(_criticalSectionRTCPSender); - - _method = kRtcpOff; - _cbTransport = NULL; - _usingNack = false; - _sending = false; - _sendTMMBN = false; - _TMMBR = false; - _IJ = false; - _REMB = false; - _sendREMB = false; - last_rtp_timestamp_ = 0; - last_frame_capture_time_ms_ = -1; - start_timestamp_ = -1; - _SSRC = 0; - _remoteSSRC = 0; - _cameraDelayMS = 0; - _sequenceNumberFIR = 0; - _tmmbr_Send = 0; - _packetOH_Send = 0; - _nextTimeToSendRTCP = 0; - _CSRCs = 0; - _appSend = false; - _appSubType = 0; - - if(_appData) - { - delete [] _appData; - _appData = NULL; - } - _appLength = 0; - - xrSendReceiverReferenceTimeEnabled_ = false; - - _xrSendVoIPMetric = false; - - memset(&_xrVoIPMetric, 0, sizeof(_xrVoIPMetric)); - memset(_CNAME, 0, sizeof(_CNAME)); - memset(_lastSendReport, 0, sizeof(_lastSendReport)); - memset(_lastRTCPTime, 0, sizeof(_lastRTCPTime)); - last_xr_rr_.clear(); - - memset(&packet_type_counter_, 0, sizeof(packet_type_counter_)); - return 0; -} - -void -RTCPSender::ChangeUniqueId(const int32_t id) -{ - _id = id; -} - int32_t RTCPSender::RegisterSendTransport(Transport* outgoingTransport) { @@ -381,6 +326,7 @@ RTCPSender::SetIJStatus(const bool enable) } void RTCPSender::SetStartTimestamp(uint32_t start_timestamp) { + CriticalSectionScoped lock(_criticalSectionRTCPSender); start_timestamp_ = start_timestamp; } @@ -686,13 +632,9 @@ int32_t RTCPSender::BuildSR(const FeedbackState& feedback_state, // the frame being captured at this moment. We are calculating that // timestamp as the last frame's timestamp + the time since the last frame // was captured. - { - // Needs protection since this method is called on the process thread. - CriticalSectionScoped lock(_criticalSectionRTCPSender); - RTPtime = start_timestamp_ + last_rtp_timestamp_ + ( - _clock->TimeInMilliseconds() - last_frame_capture_time_ms_) * - (feedback_state.frequency_hz / 1000); - } + RTPtime = start_timestamp_ + last_rtp_timestamp_ + + (_clock->TimeInMilliseconds() - last_frame_capture_time_ms_) * + (feedback_state.frequency_hz / 1000); // Add sender data // Save for our length field @@ -2102,6 +2044,7 @@ RTCPSender::SendToNetwork(const uint8_t* dataBuffer, int32_t RTCPSender::SetCSRCStatus(const bool include) { + CriticalSectionScoped lock(_criticalSectionRTCPSender); _includeCSRCs = include; return 0; } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h index 71d92c8ea..fad3b5e33 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h @@ -23,6 +23,7 @@ #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" #include "webrtc/modules/rtp_rtcp/source/tmmbr_help.h" #include "webrtc/system_wrappers/interface/scoped_ptr.h" +#include "webrtc/system_wrappers/interface/thread_annotations.h" #include "webrtc/typedefs.h" namespace webrtc { @@ -74,10 +75,6 @@ public: ReceiveStatistics* receive_statistics); virtual ~RTCPSender(); - void ChangeUniqueId(const int32_t id); - - int32_t Init(); - int32_t RegisterSendTransport(Transport* outgoingTransport); RTCPMethod Status() const; @@ -184,13 +181,12 @@ public: private: int32_t SendToNetwork(const uint8_t* dataBuffer, const uint16_t length); - void UpdatePacketRate(); - int32_t WriteAllReportBlocksToBuffer(uint8_t* rtcpbuffer, int pos, uint8_t& numberOfReportBlocks, const uint32_t NTPsec, - const uint32_t NTPfrac); + const uint32_t NTPfrac) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t WriteReportBlocksToBuffer( uint8_t* rtcpbuffer, @@ -211,12 +207,14 @@ private: uint8_t* rtcpbuffer, int& pos, uint32_t NTPsec, - uint32_t NTPfrac); + uint32_t NTPfrac) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t BuildRR(uint8_t* rtcpbuffer, int& pos, const uint32_t NTPsec, - const uint32_t NTPfrac); + const uint32_t NTPfrac) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int PrepareRTCP( const FeedbackState& feedback_state, @@ -233,117 +231,136 @@ private: int32_t BuildExtendedJitterReport( uint8_t* rtcpbuffer, int& pos, - const uint32_t jitterTransmissionTimeOffset); + const uint32_t jitterTransmissionTimeOffset) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); - int32_t BuildSDEC(uint8_t* rtcpbuffer, int& pos); - int32_t BuildPLI(uint8_t* rtcpbuffer, int& pos); - int32_t BuildREMB(uint8_t* rtcpbuffer, int& pos); - int32_t BuildTMMBR(ModuleRtpRtcpImpl* module, - uint8_t* rtcpbuffer, - int& pos); - int32_t BuildTMMBN(uint8_t* rtcpbuffer, int& pos); - int32_t BuildAPP(uint8_t* rtcpbuffer, int& pos); - int32_t BuildVoIPMetric(uint8_t* rtcpbuffer, int& pos); - int32_t BuildBYE(uint8_t* rtcpbuffer, int& pos); - int32_t BuildFIR(uint8_t* rtcpbuffer, int& pos, bool repeat); - int32_t BuildSLI(uint8_t* rtcpbuffer, - int& pos, - const uint8_t pictureID); + int32_t BuildSDEC(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildPLI(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildREMB(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildTMMBR(ModuleRtpRtcpImpl* module, uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildTMMBN(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildAPP(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildVoIPMetric(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildBYE(uint8_t* rtcpbuffer, int& pos) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildFIR(uint8_t* rtcpbuffer, int& pos, bool repeat) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); + int32_t BuildSLI(uint8_t* rtcpbuffer, int& pos, const uint8_t pictureID) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t BuildRPSI(uint8_t* rtcpbuffer, int& pos, const uint64_t pictureID, - const uint8_t payloadType); + const uint8_t payloadType) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t BuildNACK(uint8_t* rtcpbuffer, int& pos, const int32_t nackSize, const uint16_t* nackList, - std::string* nackString); - + std::string* nackString) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t BuildReceiverReferenceTime(uint8_t* buffer, int& pos, uint32_t ntp_sec, - uint32_t ntp_frac); + uint32_t ntp_frac) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); int32_t BuildDlrr(uint8_t* buffer, int& pos, - const RtcpReceiveTimeInfo& info); + const RtcpReceiveTimeInfo& info) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPSender); private: - int32_t _id; + const int32_t _id; const bool _audio; - Clock* _clock; - RTCPMethod _method; + Clock* const _clock; + RTCPMethod _method GUARDED_BY(_criticalSectionRTCPSender); CriticalSectionWrapper* _criticalSectionTransport; - Transport* _cbTransport; + Transport* _cbTransport GUARDED_BY(_criticalSectionTransport); CriticalSectionWrapper* _criticalSectionRTCPSender; - bool _usingNack; - bool _sending; - bool _sendTMMBN; - bool _REMB; - bool _sendREMB; - bool _TMMBR; - bool _IJ; + bool _usingNack GUARDED_BY(_criticalSectionRTCPSender); + bool _sending GUARDED_BY(_criticalSectionRTCPSender); + bool _sendTMMBN GUARDED_BY(_criticalSectionRTCPSender); + bool _REMB GUARDED_BY(_criticalSectionRTCPSender); + bool _sendREMB GUARDED_BY(_criticalSectionRTCPSender); + bool _TMMBR GUARDED_BY(_criticalSectionRTCPSender); + bool _IJ GUARDED_BY(_criticalSectionRTCPSender); - int64_t _nextTimeToSendRTCP; + int64_t _nextTimeToSendRTCP GUARDED_BY(_criticalSectionRTCPSender); - uint32_t start_timestamp_; - uint32_t last_rtp_timestamp_; - int64_t last_frame_capture_time_ms_; - uint32_t _SSRC; - uint32_t _remoteSSRC; // SSRC that we receive on our RTP channel - char _CNAME[RTCP_CNAME_SIZE]; + uint32_t start_timestamp_ GUARDED_BY(_criticalSectionRTCPSender); + uint32_t last_rtp_timestamp_ GUARDED_BY(_criticalSectionRTCPSender); + int64_t last_frame_capture_time_ms_ GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _SSRC GUARDED_BY(_criticalSectionRTCPSender); + // SSRC that we receive on our RTP channel + uint32_t _remoteSSRC GUARDED_BY(_criticalSectionRTCPSender); + char _CNAME[RTCP_CNAME_SIZE] GUARDED_BY(_criticalSectionRTCPSender); + ReceiveStatistics* receive_statistics_ + GUARDED_BY(_criticalSectionRTCPSender); + std::map internal_report_blocks_ + GUARDED_BY(_criticalSectionRTCPSender); + std::map external_report_blocks_ + GUARDED_BY(_criticalSectionRTCPSender); + std::map _csrcCNAMEs + GUARDED_BY(_criticalSectionRTCPSender); - ReceiveStatistics* receive_statistics_; - std::map internal_report_blocks_; - std::map external_report_blocks_; - std::map _csrcCNAMEs; - - int32_t _cameraDelayMS; + int32_t _cameraDelayMS GUARDED_BY(_criticalSectionRTCPSender); // Sent - uint32_t _lastSendReport[RTCP_NUMBER_OF_SR]; // allow packet loss and RTT above 1 sec - uint32_t _lastRTCPTime[RTCP_NUMBER_OF_SR]; + uint32_t _lastSendReport[RTCP_NUMBER_OF_SR] GUARDED_BY( + _criticalSectionRTCPSender); // allow packet loss and RTT above 1 sec + uint32_t _lastRTCPTime[RTCP_NUMBER_OF_SR] GUARDED_BY( + _criticalSectionRTCPSender); // Sent XR receiver reference time report. // . - std::map last_xr_rr_; + std::map last_xr_rr_ + GUARDED_BY(_criticalSectionRTCPSender); // send CSRCs - uint8_t _CSRCs; - uint32_t _CSRC[kRtpCsrcSize]; - bool _includeCSRCs; + uint8_t _CSRCs GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _CSRC[kRtpCsrcSize] GUARDED_BY(_criticalSectionRTCPSender); + bool _includeCSRCs GUARDED_BY(_criticalSectionRTCPSender); // Full intra request - uint8_t _sequenceNumberFIR; + uint8_t _sequenceNumberFIR GUARDED_BY(_criticalSectionRTCPSender); // REMB - uint8_t _lengthRembSSRC; - uint8_t _sizeRembSSRC; - uint32_t* _rembSSRC; - uint32_t _rembBitrate; + uint8_t _lengthRembSSRC GUARDED_BY(_criticalSectionRTCPSender); + uint8_t _sizeRembSSRC GUARDED_BY(_criticalSectionRTCPSender); + uint32_t* _rembSSRC GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _rembBitrate GUARDED_BY(_criticalSectionRTCPSender); - TMMBRHelp _tmmbrHelp; - uint32_t _tmmbr_Send; - uint32_t _packetOH_Send; + TMMBRHelp _tmmbrHelp GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _tmmbr_Send GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _packetOH_Send GUARDED_BY(_criticalSectionRTCPSender); // APP - bool _appSend; - uint8_t _appSubType; - uint32_t _appName; - uint8_t* _appData; - uint16_t _appLength; + bool _appSend GUARDED_BY(_criticalSectionRTCPSender); + uint8_t _appSubType GUARDED_BY(_criticalSectionRTCPSender); + uint32_t _appName GUARDED_BY(_criticalSectionRTCPSender); + uint8_t* _appData GUARDED_BY(_criticalSectionRTCPSender); + uint16_t _appLength GUARDED_BY(_criticalSectionRTCPSender); // True if sending of XR Receiver reference time report is enabled. - bool xrSendReceiverReferenceTimeEnabled_; + bool xrSendReceiverReferenceTimeEnabled_ + GUARDED_BY(_criticalSectionRTCPSender); // XR VoIP metric - bool _xrSendVoIPMetric; - RTCPVoIPMetric _xrVoIPMetric; + bool _xrSendVoIPMetric GUARDED_BY(_criticalSectionRTCPSender); + RTCPVoIPMetric _xrVoIPMetric GUARDED_BY(_criticalSectionRTCPSender); - RtcpPacketTypeCounter packet_type_counter_; + RtcpPacketTypeCounter packet_type_counter_ + GUARDED_BY(_criticalSectionRTCPSender); }; } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index dfb655c51..cba1c3463 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -304,7 +304,6 @@ class RtcpSenderTest : public ::testing::Test { rtcp_receiver_ = new RTCPReceiver(0, &clock_, rtp_rtcp_impl_); test_transport_->SetRTCPReceiver(rtcp_receiver_); // Initialize - EXPECT_EQ(0, rtcp_sender_->Init()); EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(test_transport_)); } ~RtcpSenderTest() { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 0c771c510..34a08a8e4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -47,13 +47,12 @@ RtpRtcp* RtpRtcp::CreateRtpRtcp(const RtpRtcp::Configuration& configuration) { if (configuration.clock) { return new ModuleRtpRtcpImpl(configuration); } else { + // No clock implementation provided, use default clock. RtpRtcp::Configuration configuration_copy; memcpy(&configuration_copy, &configuration, sizeof(RtpRtcp::Configuration)); configuration_copy.clock = Clock::GetRealTimeClock(); - ModuleRtpRtcpImpl* rtp_rtcp_instance = - new ModuleRtpRtcpImpl(configuration_copy); - return rtp_rtcp_instance; + return new ModuleRtpRtcpImpl(configuration_copy); } }