From d09d0748270f40c35330837069523245839b7258 Mon Sep 17 00:00:00 2001 From: "andresp@webrtc.org" Date: Wed, 26 Mar 2014 14:27:34 +0000 Subject: [PATCH] Protect write of send_target_bitrate. This issue was catch by tsan bot. BUG=3065 R=stefan@webrtc.org, andrew Review URL: https://webrtc-codereview.appspot.com/10619004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5790 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 38 ++++++++++++++------ webrtc/modules/rtp_rtcp/source/rtp_sender.h | 11 +++++- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 5027e738d4..29e4616e17 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -56,7 +56,6 @@ RTPSender::RTPSender(const int32_t id, transport_(transport), sending_media_(true), // Default to sending media. max_payload_length_(IP_PACKET_SIZE - 28), // Default is IP-v4/UDP. - target_send_bitrate_(0), packet_over_head_(28), payload_type_(-1), payload_type_map_(), @@ -88,7 +87,9 @@ RTPSender::RTPSender(const int32_t id, csrcs_(), include_csrcs_(true), rtx_(kRtxOff), - payload_type_rtx_(-1) { + payload_type_rtx_(-1), + target_bitrate_critsect_(CriticalSectionWrapper::CreateCriticalSection()), + target_bitrate_kbps_(0) { memset(nack_byte_count_times_, 0, sizeof(nack_byte_count_times_)); memset(nack_byte_count_, 0, sizeof(nack_byte_count_)); memset(csrcs_, 0, sizeof(csrcs_)); @@ -130,7 +131,7 @@ RTPSender::~RTPSender() { } void RTPSender::SetTargetSendBitrate(const uint32_t bits) { - target_send_bitrate_ = static_cast(bits / 1000); + SetTargetBitrateKbps(static_cast(bits / 1000)); } uint16_t RTPSender::ActualSendBitrateKbit() const { @@ -477,7 +478,8 @@ bool RTPSender::SendPaddingAccordingToBitrate( // Current bitrate since last estimate(1 second) averaged with the // estimate since then, to get the most up to date bitrate. uint32_t current_bitrate = bitrate_sent_.BitrateNow(); - int bitrate_diff = target_send_bitrate_ * 1000 - current_bitrate; + uint16_t target_bitrate_kbps = GetTargetBitrateKbps(); + int bitrate_diff = target_bitrate_kbps * 1000 - current_bitrate; if (bitrate_diff <= 0) { return true; } @@ -488,7 +490,7 @@ bool RTPSender::SendPaddingAccordingToBitrate( } else { bytes = (bitrate_diff / 8); // Cap at 200 ms of target send data. - int bytes_cap = target_send_bitrate_ * 25; // 1000 / 8 / 5. + int bytes_cap = target_bitrate_kbps * 25; // 1000 / 8 / 5. if (bytes > bytes_cap) { bytes = bytes_cap; } @@ -670,12 +672,15 @@ void RTPSender::OnReceivedNACK( "num_seqnum", nack_sequence_numbers.size(), "avg_rtt", avg_rtt); const int64_t now = clock_->TimeInMilliseconds(); uint32_t bytes_re_sent = 0; + uint16_t target_bitrate_kbps = GetTargetBitrateKbps(); // Enough bandwidth to send NACK? if (!ProcessNACKBitRate(now)) { - WEBRTC_TRACE(kTraceStream, kTraceRtpRtcp, id_, + WEBRTC_TRACE(kTraceStream, + kTraceRtpRtcp, + id_, "NACK bitrate reached. Skip sending NACK response. Target %d", - target_send_bitrate_); + target_bitrate_kbps); return; } @@ -696,10 +701,10 @@ void RTPSender::OnReceivedNACK( break; } // Delay bandwidth estimate (RTT * BW). - if (target_send_bitrate_ != 0 && avg_rtt) { + if (target_bitrate_kbps != 0 && avg_rtt) { // kbits/s * ms = bits => bits/8 = bytes uint32_t target_bytes = - (static_cast(target_send_bitrate_) * avg_rtt) >> 3; + (static_cast(target_bitrate_kbps) * avg_rtt) >> 3; if (bytes_re_sent > target_bytes) { break; // Ignore the rest of the packets in the list. } @@ -716,10 +721,11 @@ bool RTPSender::ProcessNACKBitRate(const uint32_t now) { uint32_t num = 0; int32_t byte_count = 0; const uint32_t avg_interval = 1000; + uint16_t target_bitrate_kbps = GetTargetBitrateKbps(); CriticalSectionScoped cs(send_critsect_); - if (target_send_bitrate_ == 0) { + if (target_bitrate_kbps == 0) { return true; } for (num = 0; num < NACK_BYTECOUNT_SIZE; ++num) { @@ -739,7 +745,7 @@ bool RTPSender::ProcessNACKBitRate(const uint32_t now) { time_interval = avg_interval; } } - return (byte_count * 8) < (target_send_bitrate_ * time_interval); + return (byte_count * 8) < (target_bitrate_kbps * time_interval); } void RTPSender::UpdateNACKBitRate(const uint32_t bytes, @@ -1699,4 +1705,14 @@ void RTPSender::BitrateUpdated(const BitrateStatistics& stats) { bitrate_callback_->Notify(stats, ssrc_); } } + +void RTPSender::SetTargetBitrateKbps(uint16_t bitrate_kbps) { + CriticalSectionScoped cs(target_bitrate_critsect_.get()); + target_bitrate_kbps_ = bitrate_kbps; +} + +uint16_t RTPSender::GetTargetBitrateKbps() { + CriticalSectionScoped cs(target_bitrate_critsect_.get()); + return target_bitrate_kbps_; +} } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 5cd21be609..fc2c8217f4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -326,6 +326,9 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer { bool is_retransmit); bool IsFecPacket(const uint8_t* buffer, const RTPHeader& header) const; + void SetTargetBitrateKbps(uint16_t bitrate_kbps); + uint16_t GetTargetBitrateKbps(); + Clock* clock_; Bitrate bitrate_sent_; @@ -341,7 +344,6 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer { bool sending_media_ GUARDED_BY(send_critsect_); uint16_t max_payload_length_; - uint16_t target_send_bitrate_; uint16_t packet_over_head_; int8_t payload_type_ GUARDED_BY(send_critsect_); @@ -388,6 +390,13 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer { int rtx_; uint32_t ssrc_rtx_; int payload_type_rtx_; + + // Note: Don't access this variable directly, always go through + // SetTargetBitrateKbps or GetTargetBitrateKbps. Also remember + // that by the time the function returns there is no guarantee + // that the target bitrate is still valid. + scoped_ptr target_bitrate_critsect_; + uint16_t target_bitrate_kbps_ GUARDED_BY(target_bitrate_critsect_); }; } // namespace webrtc