From 71fead2146b6895d4d04c3f4c2a8eb465b2c5881 Mon Sep 17 00:00:00 2001 From: danilchap Date: Thu, 18 Aug 2016 02:01:49 -0700 Subject: [PATCH] Reland of StartTimestamp generated randomly in RtpSender constructor (patchset #1 id:1 of https://codereview.webrtc.org/2248413002/ ) Reason for revert: Reland: downstream code expectation about rtp_sender timestamp adjusted. Original issue's description: > Revert of StartTimestamp generated randomly in RtpSender constructor (patchset #4 id:60001 of https://codereview.webrtc.org/2241193002/ ) > > Reason for revert: > Breaks downstream code. > > Original issue's description: > > StartTimestamp generated randomly in RtpSender constructor > > instead of not-randomly at SetSendingState(true) > > Renamed to timestamp_offset_ to better match meaning of the variable. > > > > R=asapersson@webrtc.org, terelius@webrtc.org > > > > Committed: https://crrev.com/4466782ae43e1b1125a55ee7e18abd10dd37cede > > Cr-Commit-Position: refs/heads/master@{#13796} > > TBR=asapersson@webrtc.org,terelius@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > > Committed: https://crrev.com/86c96948e340cf8b879bddb0c7293f3b5ad4dad4 > Cr-Commit-Position: refs/heads/master@{#13798} TBR=asapersson@webrtc.org,terelius@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Review-Url: https://codereview.webrtc.org/2257083002 Cr-Commit-Position: refs/heads/master@{#13811} --- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 8 ++--- webrtc/modules/rtp_rtcp/source/rtcp_sender.h | 4 +-- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 4 +-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 18 +++++----- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 34 ++++++------------- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 7 ++-- .../rtp_rtcp/source/rtp_sender_unittest.cc | 1 + 7 files changed, 32 insertions(+), 44 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 5365d19abb..79d946a381 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -161,7 +161,7 @@ RTCPSender::RTCPSender( sending_(false), remb_enabled_(false), next_time_to_send_rtcp_(0), - start_timestamp_(0), + timestamp_offset_(0), last_rtp_timestamp_(0), last_frame_capture_time_ms_(-1), ssrc_(0), @@ -288,9 +288,9 @@ void RTCPSender::SetMaxPayloadLength(size_t max_payload_length) { max_payload_length_ = max_payload_length; } -void RTCPSender::SetStartTimestamp(uint32_t start_timestamp) { +void RTCPSender::SetTimestampOffset(uint32_t timestamp_offset) { rtc::CritScope lock(&critical_section_rtcp_sender_); - start_timestamp_ = start_timestamp; + timestamp_offset_ = timestamp_offset; } void RTCPSender::SetLastRtpTime(uint32_t rtp_timestamp, @@ -444,7 +444,7 @@ std::unique_ptr RTCPSender::BuildSR(const RtcpContext& ctx) { // timestamp as the last frame's timestamp + the time since the last frame // was captured. uint32_t rtp_timestamp = - start_timestamp_ + last_rtp_timestamp_ + + timestamp_offset_ + last_rtp_timestamp_ + (clock_->TimeInMilliseconds() - last_frame_capture_time_ms_) * (ctx.feedback_state_.frequency_hz / 1000); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h index 58f19b0918..54f93f1ab7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h @@ -93,7 +93,7 @@ class RTCPSender { int32_t SetNackStatus(bool enable); - void SetStartTimestamp(uint32_t start_timestamp); + void SetTimestampOffset(uint32_t timestamp_offset); void SetLastRtpTime(uint32_t rtp_timestamp, int64_t capture_time_ms); @@ -214,7 +214,7 @@ class RTCPSender { int64_t next_time_to_send_rtcp_ GUARDED_BY(critical_section_rtcp_sender_); - uint32_t start_timestamp_ GUARDED_BY(critical_section_rtcp_sender_); + uint32_t timestamp_offset_ GUARDED_BY(critical_section_rtcp_sender_); uint32_t last_rtp_timestamp_ GUARDED_BY(critical_section_rtcp_sender_); int64_t last_frame_capture_time_ms_ GUARDED_BY(critical_section_rtcp_sender_); uint32_t ssrc_ 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 270088ebac..5d230dc6f1 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -243,7 +243,7 @@ class RtcpSenderTest : public ::testing::Test { nullptr, nullptr, &test_transport_)); rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); - rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp); + rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); } @@ -813,7 +813,7 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { nullptr, nullptr, &mock_transport)); rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); - rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp); + rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); // Set up XR VoIP metric to be included with BYE diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index b6384421c0..9b9a0bf5b0 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -105,6 +105,11 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) uint32_t SSRC = rtp_sender_.SSRC(); rtcp_sender_.SetSSRC(SSRC); SetRtcpReceiverSsrcs(SSRC); + + // Make sure rtcp sender use same timestamp offset as rtp sender. + rtcp_sender_.SetTimestampOffset(rtp_sender_.TimestampOffset()); + + // Set default packet size limit. SetMaxTransferUnit(IP_PACKET_SIZE); } @@ -262,13 +267,13 @@ int8_t ModuleRtpRtcpImpl::SendPayloadType() const { } uint32_t ModuleRtpRtcpImpl::StartTimestamp() const { - return rtp_sender_.StartTimestamp(); + return rtp_sender_.TimestampOffset(); } // Configure start timestamp, default is a random number. void ModuleRtpRtcpImpl::SetStartTimestamp(const uint32_t timestamp) { - rtcp_sender_.SetStartTimestamp(timestamp); - rtp_sender_.SetStartTimestamp(timestamp, true); + rtcp_sender_.SetTimestampOffset(timestamp); + rtp_sender_.SetTimestampOffset(timestamp); } uint16_t ModuleRtpRtcpImpl::SequenceNumber() const { @@ -281,8 +286,8 @@ void ModuleRtpRtcpImpl::SetSequenceNumber(const uint16_t seq_num) { } void ModuleRtpRtcpImpl::SetRtpState(const RtpState& rtp_state) { - SetStartTimestamp(rtp_state.start_timestamp); rtp_sender_.SetRtpState(rtp_state); + rtcp_sender_.SetTimestampOffset(rtp_state.start_timestamp); } void ModuleRtpRtcpImpl::SetRtxState(const RtpState& rtp_state) { @@ -353,13 +358,8 @@ int32_t ModuleRtpRtcpImpl::SetSendingStatus(const bool sending) { collision_detected_ = false; - // Generate a new time_stamp if true and not configured via API // Generate a new SSRC for the next "call" if false rtp_sender_.SetSendingStatus(sending); - if (sending) { - // Make sure the RTCP sender has the same timestamp offset. - rtcp_sender_.SetStartTimestamp(rtp_sender_.StartTimestamp()); - } // Make sure that RTCP objects are aware of our SSRC (it could have changed // Due to collision) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index d4749f9f5e..e2717d3432 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -109,8 +109,6 @@ RTPSender::RTPSender( send_packet_observer_(send_packet_observer), bitrate_callback_(bitrate_callback), // RTP variables - start_timestamp_forced_(false), - start_timestamp_(0), ssrc_db_(SSRCDatabase::GetSSRCDatabase()), remote_ssrc_(0), sequence_number_forced_(false), @@ -128,6 +126,8 @@ RTPSender::RTPSender( ssrc_rtx_ = ssrc_db_->CreateSSRC(); RTC_DCHECK(ssrc_rtx_ != 0); + // This random initialization is not intended to be cryptographic strong. + timestamp_offset_ = random_.Rand(); // Random start, 16 bits. Can't be 0. sequence_number_rtx_ = random_.Rand(1, kMaxInitRtpSeqNumber); sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); @@ -1089,7 +1089,7 @@ int32_t RTPSender::BuildRtpHeader(uint8_t* data_buffer, if (!sending_media_) return -1; - timestamp_ = start_timestamp_ + capture_timestamp; + timestamp_ = timestamp_offset_ + capture_timestamp; last_timestamp_time_ms_ = clock_->TimeInMilliseconds(); uint32_t sequence_number = sequence_number_++; capture_time_ms_ = capture_time_ms; @@ -1489,13 +1489,7 @@ bool RTPSender::UpdateTransportSequenceNumber(RtpPacketToSend* packet, } void RTPSender::SetSendingStatus(bool enabled) { - if (enabled) { - uint32_t frequency_hz = SendPayloadFrequency(); - uint32_t RTPtime = CurrentRtp(*clock_, frequency_hz); - - // Will be ignored if it's already configured via API. - SetStartTimestamp(RTPtime, false); - } else { + if (!enabled) { rtc::CritScope lock(&send_critsect_); if (!ssrc_forced_) { // Generate a new SSRC. @@ -1526,21 +1520,14 @@ uint32_t RTPSender::Timestamp() const { return timestamp_; } -void RTPSender::SetStartTimestamp(uint32_t timestamp, bool force) { +void RTPSender::SetTimestampOffset(uint32_t timestamp) { rtc::CritScope lock(&send_critsect_); - if (force) { - start_timestamp_forced_ = true; - start_timestamp_ = timestamp; - } else { - if (!start_timestamp_forced_) { - start_timestamp_ = timestamp; - } - } + timestamp_offset_ = timestamp; } -uint32_t RTPSender::StartTimestamp() const { +uint32_t RTPSender::TimestampOffset() const { rtc::CritScope lock(&send_critsect_); - return start_timestamp_; + return timestamp_offset_; } uint32_t RTPSender::GenerateNewSSRC() { @@ -1705,6 +1692,7 @@ void RTPSender::SetRtpState(const RtpState& rtp_state) { rtc::CritScope lock(&send_critsect_); sequence_number_ = rtp_state.sequence_number; sequence_number_forced_ = true; + timestamp_offset_ = rtp_state.start_timestamp; timestamp_ = rtp_state.timestamp; capture_time_ms_ = rtp_state.capture_time_ms; last_timestamp_time_ms_ = rtp_state.last_timestamp_time_ms; @@ -1716,7 +1704,7 @@ RtpState RTPSender::GetRtpState() const { RtpState state; state.sequence_number = sequence_number_; - state.start_timestamp = start_timestamp_; + state.start_timestamp = timestamp_offset_; state.timestamp = timestamp_; state.capture_time_ms = capture_time_ms_; state.last_timestamp_time_ms = last_timestamp_time_ms_; @@ -1735,7 +1723,7 @@ RtpState RTPSender::GetRtxRtpState() const { RtpState state; state.sequence_number = sequence_number_rtx_; - state.start_timestamp = start_timestamp_; + state.start_timestamp = timestamp_offset_; return state; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 4519b4ca30..540c6f18ec 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -90,8 +90,8 @@ class RTPSender { void GetDataCounters(StreamDataCounters* rtp_stats, StreamDataCounters* rtx_stats) const; - uint32_t StartTimestamp() const; - void SetStartTimestamp(uint32_t timestamp, bool force); + uint32_t TimestampOffset() const; + void SetTimestampOffset(uint32_t timestamp); uint32_t GenerateNewSSRC(); void SetSSRC(uint32_t ssrc); @@ -402,8 +402,7 @@ class RTPSender { BitrateStatisticsObserver* const bitrate_callback_; // RTP variables - bool start_timestamp_forced_ GUARDED_BY(send_critsect_); - uint32_t start_timestamp_ GUARDED_BY(send_critsect_); + uint32_t timestamp_offset_ GUARDED_BY(send_critsect_); SSRCDatabase* const ssrc_db_; uint32_t remote_ssrc_ GUARDED_BY(send_critsect_); bool sequence_number_forced_ GUARDED_BY(send_critsect_); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index cabd411abf..059c4c0b3a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -150,6 +150,7 @@ class RtpSenderTest : public ::testing::Test { &retransmission_rate_limiter_)); rtp_sender_->SetSequenceNumber(kSeqNum); rtp_sender_->SetSendPayloadType(kPayload); + rtp_sender_->SetTimestampOffset(0); } SimulatedClock fake_clock_;