From 5e58bcbf295bf6908cf6d5da1b011aae405b88be Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Wed, 24 Oct 2018 13:34:32 +0200 Subject: [PATCH] Forward audio rtp frequency to Rtcp sender and use it for SR packets Process video rtp frequency in the same way. Bug: webrtc:6458 Change-Id: Ia22768e1242d686c2b3e2b911f3e5e492cf8b895 Reviewed-on: https://webrtc-review.googlesource.com/c/107651 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#25334} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 6 ++--- modules/rtp_rtcp/source/rtcp_sender.cc | 24 +++++++++++++++---- modules/rtp_rtcp/source/rtcp_sender.h | 13 +++++++++- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 6 +++-- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 9 ++++--- 5 files changed, 45 insertions(+), 13 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index e5873135a1..5b9a051525 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -32,9 +32,9 @@ class TransportFeedback; } const int kVideoPayloadTypeFrequency = 90000; -// TODO(solenberg): RTP time stamp rate for RTCP is fixed at 8k, this is legacy -// and should be fixed. -// See: https://bugs.chromium.org/p/webrtc/issues/detail?id=6458 + +// TODO(bugs.webrtc.org/6458): Remove this when all the depending projects are +// updated to correctly set rtp rate for RtcpSender. const int kBogusRtpRateForAudioRtcp = 8000; // Minimum RTP header size in bytes. diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index bfddc422f0..80a22edaf8 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -151,7 +151,8 @@ RTCPSender::RTCPSender( xr_send_receiver_reference_time_enabled_(false), packet_type_counter_observer_(packet_type_counter_observer), - send_video_bitrate_allocation_(false) { + send_video_bitrate_allocation_(false), + last_payload_type_(-1) { RTC_DCHECK(transport_ != nullptr); builders_[kRtcpSr] = &RTCPSender::BuildSR; @@ -254,8 +255,14 @@ void RTCPSender::SetTimestampOffset(uint32_t timestamp_offset) { } void RTCPSender::SetLastRtpTime(uint32_t rtp_timestamp, - int64_t capture_time_ms) { + int64_t capture_time_ms, + int8_t payload_type) { rtc::CritScope lock(&critical_section_rtcp_sender_); + // For compatibility with clients who don't set payload type correctly on all + // calls. + if (payload_type != -1) { + last_payload_type_ = payload_type; + } last_rtp_timestamp_ = rtp_timestamp; if (capture_time_ms < 0) { // We don't currently get a capture time from VoiceEngine. @@ -265,6 +272,11 @@ void RTCPSender::SetLastRtpTime(uint32_t rtp_timestamp, } } +void RTCPSender::SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz) { + rtc::CritScope lock(&critical_section_rtcp_sender_); + rtp_clock_rates_khz_[payload_type] = rtp_clock_rate_hz / 1000; +} + uint32_t RTCPSender::SSRC() const { rtc::CritScope lock(&critical_section_rtcp_sender_); return ssrc_; @@ -411,8 +423,12 @@ std::unique_ptr RTCPSender::BuildSR(const RtcpContext& ctx) { // 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. - uint32_t rtp_rate = - (audio_ ? kBogusRtpRateForAudioRtcp : kVideoPayloadTypeFrequency) / 1000; + int rtp_rate = rtp_clock_rates_khz_[last_payload_type_]; + if (rtp_rate <= 0) { + rtp_rate = + (audio_ ? kBogusRtpRateForAudioRtcp : kVideoPayloadTypeFrequency) / + 1000; + } uint32_t rtp_timestamp = timestamp_offset_ + last_rtp_timestamp_ + (clock_->TimeInMilliseconds() - last_frame_capture_time_ms_) * rtp_rate; diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index c9220dd35c..8570591a89 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -82,7 +82,13 @@ class RTCPSender { void SetTimestampOffset(uint32_t timestamp_offset); - void SetLastRtpTime(uint32_t rtp_timestamp, int64_t capture_time_ms); + // TODO(bugs.webrtc.org/6458): Remove default parameter value when all the + // depending projects are updated to correctly set payload type. + void SetLastRtpTime(uint32_t rtp_timestamp, + int64_t capture_time_ms, + int8_t payload_type = -1); + + void SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz); uint32_t SSRC() const; @@ -244,6 +250,11 @@ class RTCPSender { RTC_GUARDED_BY(critical_section_rtcp_sender_); bool send_video_bitrate_allocation_ RTC_GUARDED_BY(critical_section_rtcp_sender_); + + std::map rtp_clock_rates_khz_ + RTC_GUARDED_BY(critical_section_rtcp_sender_); + int8_t last_payload_type_ RTC_GUARDED_BY(critical_section_rtcp_sender_); + absl::optional CheckAndUpdateLayerStructure( const VideoBitrateAllocation& bitrate) const RTC_EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index bcffc81213..a2f1d2140b 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -90,7 +90,8 @@ class RtcpSenderTest : public ::testing::Test { rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); - rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), + /*paylpad_type=*/0); } void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { @@ -603,7 +604,8 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); - rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds()); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), + /*paylpad_type=*/0); // Set up REMB info to be included with BYE. rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index d54f942333..6e40d0271a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -256,6 +256,7 @@ void ModuleRtpRtcpImpl::IncomingRtcpPacket(const uint8_t* rtcp_packet, } int32_t ModuleRtpRtcpImpl::RegisterSendPayload(const CodecInst& voice_codec) { + rtcp_sender_.SetRtpClockRate(voice_codec.pltype, voice_codec.plfreq); return rtp_sender_->RegisterPayload( voice_codec.plname, voice_codec.pltype, voice_codec.plfreq, voice_codec.channels, (voice_codec.rate < 0) ? 0 : voice_codec.rate); @@ -263,8 +264,10 @@ int32_t ModuleRtpRtcpImpl::RegisterSendPayload(const CodecInst& voice_codec) { void ModuleRtpRtcpImpl::RegisterVideoSendPayload(int payload_type, const char* payload_name) { - RTC_CHECK_EQ( - 0, rtp_sender_->RegisterPayload(payload_name, payload_type, 90000, 0, 0)); + rtcp_sender_.SetRtpClockRate(payload_type, kVideoPayloadTypeFrequency); + RTC_CHECK_EQ(0, + rtp_sender_->RegisterPayload(payload_name, payload_type, + kVideoPayloadTypeFrequency, 0, 0)); } int32_t ModuleRtpRtcpImpl::DeRegisterSendPayload(const int8_t payload_type) { @@ -410,7 +413,7 @@ bool ModuleRtpRtcpImpl::SendOutgoingData( const RTPFragmentationHeader* fragmentation, const RTPVideoHeader* rtp_video_header, uint32_t* transport_frame_id_out) { - rtcp_sender_.SetLastRtpTime(time_stamp, capture_time_ms); + rtcp_sender_.SetLastRtpTime(time_stamp, capture_time_ms, payload_type); // Make sure an RTCP report isn't queued behind a key frame. if (rtcp_sender_.TimeToSendRTCPReport(kVideoFrameKey == frame_type)) { rtcp_sender_.SendRTCP(GetFeedbackState(), kRtcpReport);