From f147639fcf3f45fdf764480834a483eb88b1cf2a Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Thu, 17 Jul 2014 16:10:14 +0000 Subject: Fix issue where padding is sent before media with undefined timestamps if not abs-send-time is enabled. This broke bandwidth estimation for calls without abs-send-time is enabled, but where RTX was. BUG= R=pbos@webrtc.org Review URL: https://webrtc-codereview.appspot.com/16929004 git-svn-id: http://webrtc.googlecode.com/svn/trunk/webrtc@6719 4adac7df-926f-26a2-2b94-8c16560cd09d --- modules/rtp_rtcp/interface/rtp_rtcp_defines.h | 4 +++- modules/rtp_rtcp/source/rtp_header_extension.cc | 10 +++++++++ modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc | 4 ++++ modules/rtp_rtcp/source/rtp_sender.cc | 26 ++++++++++++++++++++--- modules/rtp_rtcp/source/rtp_sender.h | 1 + 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/modules/rtp_rtcp/interface/rtp_rtcp_defines.h b/modules/rtp_rtcp/interface/rtp_rtcp_defines.h index e1bec5fc..99808439 100644 --- a/modules/rtp_rtcp/interface/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/interface/rtp_rtcp_defines.h @@ -198,12 +198,14 @@ struct RtpState { start_timestamp(0), timestamp(0), capture_time_ms(-1), - last_timestamp_time_ms(-1) {} + last_timestamp_time_ms(-1), + media_has_been_sent(false) {} uint16_t sequence_number; uint32_t start_timestamp; uint32_t timestamp; int64_t capture_time_ms; int64_t last_timestamp_time_ms; + bool media_has_been_sent; }; class RtpData diff --git a/modules/rtp_rtcp/source/rtp_header_extension.cc b/modules/rtp_rtcp/source/rtp_header_extension.cc index 2e72d750..9a1836e1 100644 --- a/modules/rtp_rtcp/source/rtp_header_extension.cc +++ b/modules/rtp_rtcp/source/rtp_header_extension.cc @@ -65,6 +65,16 @@ int32_t RtpHeaderExtensionMap::Deregister(const RTPExtensionType type) { return 0; } +bool RtpHeaderExtensionMap::IsRegistered(RTPExtensionType type) const { + std::map::const_iterator it = + extensionMap_.begin(); + for (; it != extensionMap_.end(); ++it) { + if (it->second->type == type) + return true; + } + return false; +} + int32_t RtpHeaderExtensionMap::GetType(const uint8_t id, RTPExtensionType* type) const { assert(type); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 930778c3..e4507c34 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -413,6 +413,10 @@ TEST_F(RtpSendingTest, RoundRobinPadding) { TEST_F(RtpSendingTest, RoundRobinPaddingRtx) { // Enable RTX to allow padding to be sent prior to media. for (int i = 1; i < codec_.numberOfSimulcastStreams + 1; ++i) { + // Abs-send-time is needed to be allowed to send padding prior to media, + // as otherwise the timestmap used for BWE will be broken. + senders_[i]->RegisterSendRtpHeaderExtension(kRtpExtensionAbsoluteSendTime, + 1); senders_[i]->SetRtxSendPayloadType(96); senders_[i]->SetRtxSsrc(kSenderRtxSsrc + i); senders_[i]->SetRTXSendStatus(kRtxRetransmitted); diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 0d465d93..eb88be7c 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -87,6 +87,7 @@ RTPSender::RTPSender(const int32_t id, timestamp_(0), capture_time_ms_(0), last_timestamp_time_ms_(0), + media_has_been_sent_(false), last_packet_marker_bit_(false), num_csrcs_(0), csrcs_(), @@ -520,6 +521,11 @@ int RTPSender::SendPadData(int payload_type, ++sequence_number_; over_rtx = false; } else { + // Without abs-send-time a media packet must be sent before padding so + // that the timestamps used for estimation are correct. + if (!media_has_been_sent_ && !rtp_header_extension_map_.IsRegistered( + kRtpExtensionAbsoluteSendTime)) + return bytes_sent; ssrc = ssrc_rtx_; sequence_number = sequence_number_rtx_; ++sequence_number_rtx_; @@ -603,10 +609,13 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id, uint32_t min_resend_time) { return length; } } - - CriticalSectionScoped lock(send_critsect_); + int rtx = kRtxOff; + { + CriticalSectionScoped lock(send_critsect_); + rtx = rtx_; + } return PrepareAndSendPacket(data_buffer, length, capture_time_ms, - (rtx_ & kRtxRetransmitted) > 0, true) ? + (rtx & kRtxRetransmitted) > 0, true) ? length : -1; } @@ -799,6 +808,10 @@ bool RTPSender::PrepareAndSendPacket(uint8_t* buffer, diff_ms); UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms); bool ret = SendPacketToNetwork(buffer_to_send_ptr, length); + if (ret) { + CriticalSectionScoped lock(send_critsect_); + media_has_been_sent_ = true; + } UpdateRtpStats(buffer_to_send_ptr, length, rtp_header, send_over_rtx, is_retransmit); return ret; @@ -936,6 +949,11 @@ int32_t RTPSender::SendToNetwork( uint32_t length = payload_length + rtp_header_length; if (!SendPacketToNetwork(buffer, length)) return -1; + assert(payload_length - rtp_header.paddingLength > 0); + { + CriticalSectionScoped lock(send_critsect_); + media_has_been_sent_ = true; + } UpdateRtpStats(buffer, length, rtp_header, false, false); return 0; } @@ -1670,6 +1688,7 @@ void RTPSender::SetRtpState(const RtpState& rtp_state) { timestamp_ = rtp_state.timestamp; capture_time_ms_ = rtp_state.capture_time_ms; last_timestamp_time_ms_ = rtp_state.last_timestamp_time_ms; + media_has_been_sent_ = rtp_state.media_has_been_sent; } RtpState RTPSender::GetRtpState() const { @@ -1681,6 +1700,7 @@ RtpState RTPSender::GetRtpState() const { state.timestamp = timestamp_; state.capture_time_ms = capture_time_ms_; state.last_timestamp_time_ms = last_timestamp_time_ms_; + state.media_has_been_sent = media_has_been_sent_; return state; } diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 198c6103..f3a2bdc1 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -387,6 +387,7 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer { uint32_t timestamp_ GUARDED_BY(send_critsect_); int64_t capture_time_ms_ GUARDED_BY(send_critsect_); int64_t last_timestamp_time_ms_ GUARDED_BY(send_critsect_); + bool media_has_been_sent_ GUARDED_BY(send_critsect_); bool last_packet_marker_bit_ GUARDED_BY(send_critsect_); uint8_t num_csrcs_ GUARDED_BY(send_critsect_); uint32_t csrcs_[kRtpCsrcSize] GUARDED_BY(send_critsect_); -- cgit v1.2.3