From 845c6aa140662bf5c9c8d8d5c23917cef874e78e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 29 May 2019 13:02:24 +0200 Subject: [PATCH] Add support for early loss detection using transport feedback. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10676 Change-Id: Ifdef133e123a0c54204397fb323f4c671c40a464 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/135881 Reviewed-by: Sebastian Jansson Reviewed-by: Danil Chapovalov Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#28106} --- call/BUILD.gn | 1 + call/rtp_video_sender.cc | 61 ++++++++-- call/rtp_video_sender.h | 9 +- call/rtp_video_sender_unittest.cc | 120 +++++++++++++++++++- modules/rtp_rtcp/include/rtp_rtcp.h | 5 - modules/rtp_rtcp/include/rtp_rtcp_defines.h | 11 -- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 2 - modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 - modules/rtp_rtcp/source/rtp_rtcp_impl.h | 1 - modules/rtp_rtcp/source/rtp_sender.h | 5 +- 10 files changed, 177 insertions(+), 43 deletions(-) diff --git a/call/BUILD.gn b/call/BUILD.gn index 1696478ded..52f13da16a 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -156,6 +156,7 @@ rtc_source_set("rtp_sender") { "../rtc_base:rtc_task_queue", "../rtc_base/task_utils:repeating_task", "../system_wrappers:field_trial", + "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/container:inlined_vector", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/types:optional", diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index d67bf0873e..698e158ef7 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -15,6 +15,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/memory/memory.h" #include "api/array_view.h" #include "api/transport/field_trial_based_config.h" @@ -222,6 +223,8 @@ RtpVideoSender::RtpVideoSender( webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")), account_for_packetization_overhead_(!webrtc::field_trial::IsDisabled( "WebRTC-SubtractPacketizationOverhead")), + use_early_loss_detection_( + !webrtc::field_trial::IsDisabled("WebRTC-UseEarlyLossDetection")), active_(false), module_process_thread_(nullptr), suspended_ssrcs_(std::move(suspended_ssrcs)), @@ -563,7 +566,7 @@ void RtpVideoSender::DeliverRtcp(const uint8_t* packet, size_t length) { void RtpVideoSender::ConfigureSsrcs() { // Configure regular SSRCs. - RTC_CHECK(ssrc_to_acknowledged_packets_observers_.empty()); + RTC_CHECK(ssrc_to_rtp_sender_.empty()); for (size_t i = 0; i < rtp_config_.ssrcs.size(); ++i) { uint32_t ssrc = rtp_config_.ssrcs[i]; RtpRtcp* const rtp_rtcp = rtp_streams_[i].rtp_rtcp.get(); @@ -574,10 +577,9 @@ void RtpVideoSender::ConfigureSsrcs() { if (it != suspended_ssrcs_.end()) rtp_rtcp->SetRtpState(it->second); - AcknowledgedPacketsObserver* receive_observer = - rtp_rtcp->GetAcknowledgedPacketsObserver(); - RTC_DCHECK(receive_observer != nullptr); - ssrc_to_acknowledged_packets_observers_[ssrc] = receive_observer; + RTPSender* rtp_sender = rtp_rtcp->RtpSender(); + RTC_DCHECK(rtp_sender != nullptr); + ssrc_to_rtp_sender_[ssrc] = rtp_sender; } // Set up RTX if available. @@ -785,8 +787,8 @@ void RtpVideoSender::OnPacketFeedbackVector( rtc::CritScope cs(&crit_); for (const PacketFeedback& packet : packet_feedback_vector) { if (packet.send_time_ms == PacketFeedback::kNoSendTime || !packet.ssrc || - std::find(rtp_config_.ssrcs.begin(), rtp_config_.ssrcs.end(), - *packet.ssrc) == rtp_config_.ssrcs.end()) { + absl::c_find(rtp_config_.ssrcs, *packet.ssrc) == + rtp_config_.ssrcs.end()) { // If packet send time is missing, the feedback for this packet has // probably already been processed, so ignore it. // If packet does not belong to a registered media ssrc, we are also @@ -807,17 +809,54 @@ void RtpVideoSender::OnPacketFeedbackVector( } } + if (use_early_loss_detection_) { + // Map from SSRC to vector of RTP sequence numbers that are indicated as + // lost by feedback, without being trailed by any received packets. + std::map> early_loss_detected_per_ssrc; + + for (const PacketFeedback& packet : packet_feedback_vector) { + if (packet.send_time_ms == PacketFeedback::kNoSendTime || !packet.ssrc || + absl::c_find(rtp_config_.ssrcs, *packet.ssrc) == + rtp_config_.ssrcs.end()) { + // If packet send time is missing, the feedback for this packet has + // probably already been processed, so ignore it. + // If packet does not belong to a registered media ssrc, we are also + // not interested in it. + continue; + } + + if (packet.arrival_time_ms == PacketFeedback::kNotReceived) { + // Last known lost packet, might not be detectable as lost by remote + // jitter buffer. + early_loss_detected_per_ssrc[*packet.ssrc].push_back( + packet.rtp_sequence_number); + } else { + // Packet received, so any loss prior to this is already detectable. + early_loss_detected_per_ssrc.erase(*packet.ssrc); + } + } + + for (const auto& kv : early_loss_detected_per_ssrc) { + const uint32_t ssrc = kv.first; + auto it = ssrc_to_rtp_sender_.find(ssrc); + RTC_DCHECK(it != ssrc_to_rtp_sender_.end()); + RTPSender* rtp_sender = it->second; + for (uint16_t sequence_number : kv.second) { + rtp_sender->ReSendPacket(sequence_number); + } + } + } + for (const auto& kv : acked_packets_per_ssrc) { const uint32_t ssrc = kv.first; - if (ssrc_to_acknowledged_packets_observers_.find(ssrc) == - ssrc_to_acknowledged_packets_observers_.end()) { + auto it = ssrc_to_rtp_sender_.find(ssrc); + if (it == ssrc_to_rtp_sender_.end()) { // Packets not for a media SSRC, so likely RTX or FEC. If so, ignore // since there's no RTP history to clean up anyway. continue; } rtc::ArrayView rtp_sequence_numbers(kv.second); - ssrc_to_acknowledged_packets_observers_[ssrc]->OnPacketsAcknowledged( - rtp_sequence_numbers); + it->second->OnPacketsAcknowledged(rtp_sequence_numbers); } } diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index b437769c82..48bc864b9b 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -27,6 +27,7 @@ #include "call/rtp_video_sender_interface.h" #include "logging/rtc_event_log/rtc_event_log.h" #include "modules/rtp_rtcp/include/flexfec_sender.h" +#include "modules/rtp_rtcp/source/rtp_sender.h" #include "modules/rtp_rtcp/source/rtp_sender_video.h" #include "modules/rtp_rtcp/source/rtp_sequence_number_map.h" #include "modules/rtp_rtcp/source/rtp_video_header.h" @@ -161,6 +162,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, const bool send_side_bwe_with_overhead_; const bool account_for_packetization_overhead_; + const bool use_early_loss_detection_; // TODO(holmer): Remove crit_ once RtpVideoSender runs on the // transport task queue. @@ -196,11 +198,10 @@ class RtpVideoSender : public RtpVideoSenderInterface, std::vector frame_counts_ RTC_GUARDED_BY(crit_); FrameCountObserver* const frame_count_observer_; - // Effectively const map from ssrc to AcknowledgedPacketsObserver. This - // map is set at construction time and never changed, but it's + // Effectively const map from ssrc to RTPSender, for all media ssrcs. + // This map is set at construction time and never changed, but it's // non-trivial to make it properly const. - std::map - ssrc_to_acknowledged_packets_observers_; + std::map ssrc_to_rtp_sender_; RTC_DISALLOW_COPY_AND_ASSIGN(RtpVideoSender); }; diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 9622fbc10e..16eb8a35c9 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -49,6 +49,7 @@ const int16_t kInitialPictureId2 = 44; const int16_t kInitialTl0PicIdx1 = 99; const int16_t kInitialTl0PicIdx2 = 199; const int64_t kRetransmitWindowSizeMs = 500; +const int kTransportsSequenceExtensionId = 7; class MockRtcpIntraFrameObserver : public RtcpIntraFrameObserver { public: @@ -100,6 +101,8 @@ VideoSendStream::Config CreateVideoSendStreamConfig( config.rtp.payload_type = payload_type; config.rtp.rtx.payload_type = payload_type + 1; config.rtp.nack.rtp_history_ms = 1000; + config.rtp.extensions.emplace_back(RtpExtension::kTransportSequenceNumberUri, + kTransportsSequenceExtensionId); return config; } @@ -391,7 +394,7 @@ TEST(RtpVideoSenderTest, FrameCountCallbacks) { } // Integration test verifying that ack of packet via TransportFeedback means -// that the packet is removed from RtpPacketHistory and won't be retranmistted +// that the packet is removed from RtpPacketHistory and won't be retransmitted // again. TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { const int64_t kTimeoutMs = 500; @@ -513,4 +516,119 @@ TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { test.router()->DeliverRtcp(nack_buffer.data(), nack_buffer.size()); ASSERT_TRUE(event.Wait(kTimeoutMs)); } + +// Integration test verifying that retransmissions are sent for packets which +// can be detected as lost early, using transport wide feedback. +TEST(RtpVideoSenderTest, EarlyRetransmits) { + const int64_t kTimeoutMs = 500; + + RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, + kPayloadType, {}); + test.router()->SetActive(true); + + constexpr uint8_t kPayload = 'a'; + EncodedImage encoded_image; + encoded_image.SetTimestamp(1); + encoded_image.capture_time_ms_ = 2; + encoded_image._frameType = VideoFrameType::kVideoFrameKey; + encoded_image.Allocate(1); + encoded_image.data()[0] = kPayload; + encoded_image.set_size(1); + encoded_image.SetSpatialIndex(0); + + CodecSpecificInfo codec_specific; + codec_specific.codecType = VideoCodecType::kVideoCodecGeneric; + + // Send two tiny images, mapping to single RTP packets. Capture sequence + // numbers. + rtc::Event event; + uint16_t frame1_rtp_sequence_number = 0; + uint16_t frame1_transport_sequence_number = 0; + EXPECT_CALL(test.transport(), SendRtp) + .WillOnce([&event, &frame1_rtp_sequence_number, + &frame1_transport_sequence_number]( + const uint8_t* packet, size_t length, + const PacketOptions& options) { + RtpPacket rtp_packet; + EXPECT_TRUE(rtp_packet.Parse(packet, length)); + frame1_rtp_sequence_number = rtp_packet.SequenceNumber(); + frame1_transport_sequence_number = options.packet_id; + EXPECT_EQ(rtp_packet.Ssrc(), kSsrc1); + event.Set(); + return true; + }); + EXPECT_EQ(test.router() + ->OnEncodedImage(encoded_image, &codec_specific, nullptr) + .error, + EncodedImageCallback::Result::OK); + const int64_t send_time_ms = test.clock().TimeInMilliseconds(); + + test.clock().AdvanceTimeMilliseconds(33); + ASSERT_TRUE(event.Wait(kTimeoutMs)); + + uint16_t frame2_rtp_sequence_number = 0; + uint16_t frame2_transport_sequence_number = 0; + encoded_image.SetSpatialIndex(1); + EXPECT_CALL(test.transport(), SendRtp) + .WillOnce([&event, &frame2_rtp_sequence_number, + &frame2_transport_sequence_number]( + const uint8_t* packet, size_t length, + const PacketOptions& options) { + RtpPacket rtp_packet; + EXPECT_TRUE(rtp_packet.Parse(packet, length)); + frame2_rtp_sequence_number = rtp_packet.SequenceNumber(); + frame2_transport_sequence_number = options.packet_id; + EXPECT_EQ(rtp_packet.Ssrc(), kSsrc2); + event.Set(); + return true; + }); + EXPECT_EQ(test.router() + ->OnEncodedImage(encoded_image, &codec_specific, nullptr) + .error, + EncodedImageCallback::Result::OK); + test.clock().AdvanceTimeMilliseconds(33); + ASSERT_TRUE(event.Wait(kTimeoutMs)); + + EXPECT_NE(frame1_transport_sequence_number, frame2_transport_sequence_number); + + // Inject a transport feedback where the packet for the first frame is lost, + // expect a retransmission for it. + EXPECT_CALL(test.transport(), SendRtp) + .WillOnce([&event, &frame1_rtp_sequence_number]( + const uint8_t* packet, size_t length, + const PacketOptions& options) { + RtpPacket rtp_packet; + EXPECT_TRUE(rtp_packet.Parse(packet, length)); + EXPECT_EQ(rtp_packet.Ssrc(), kRtxSsrc2); + + // Retransmitted sequence number from the RTX header should match + // the lost packet. + rtc::ArrayView payload = rtp_packet.payload(); + EXPECT_EQ(ByteReader::ReadBigEndian(payload.data()), + frame1_rtp_sequence_number); + event.Set(); + return true; + }); + + PacketFeedback first_packet_feedback(PacketFeedback::kNotReceived, + frame1_transport_sequence_number); + first_packet_feedback.rtp_sequence_number = frame1_rtp_sequence_number; + first_packet_feedback.ssrc = kSsrc1; + first_packet_feedback.send_time_ms = send_time_ms; + + PacketFeedback second_packet_feedback(test.clock().TimeInMilliseconds(), + frame2_transport_sequence_number); + first_packet_feedback.rtp_sequence_number = frame2_rtp_sequence_number; + first_packet_feedback.ssrc = kSsrc2; + first_packet_feedback.send_time_ms = send_time_ms + 33; + + std::vector feedback_vector = {first_packet_feedback, + second_packet_feedback}; + + test.router()->OnPacketFeedbackVector(feedback_vector); + + // Wait for pacer to run and send the RTX packet. + test.clock().AdvanceTimeMilliseconds(33); + ASSERT_TRUE(event.Wait(kTimeoutMs)); +} } // namespace webrtc diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 22de74ef57..1c74335c3f 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -277,11 +277,6 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { virtual StreamDataCountersCallback* GetSendChannelRtpStatisticsCallback() const = 0; - // Returns a pointer to an observer that handles information about packets - // that have been received by the remote end, or nullptr if not applicable. - virtual AcknowledgedPacketsObserver* GetAcknowledgedPacketsObserver() - const = 0; - // ************************************************************************** // RTCP // ************************************************************************** diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 1ca7b9a3aa..b5ba5a6a46 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -315,17 +315,6 @@ class TransportFeedbackObserver { virtual void OnTransportFeedback(const rtcp::TransportFeedback& feedback) = 0; }; -class AcknowledgedPacketsObserver { - public: - AcknowledgedPacketsObserver() = default; - virtual ~AcknowledgedPacketsObserver() = default; - - // Indicates RTP sequence numbers for packets that have been acknowledged as - // received by the remote end. - virtual void OnPacketsAcknowledged( - rtc::ArrayView sequence_numbers) = 0; -}; - // Interface for PacketRouter to send rtcp feedback on behalf of // congestion controller. // TODO(bugs.webrtc.org/8239): Remove and use RtcpTransceiver directly diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index b1ec2ca728..92601537bd 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -160,8 +160,6 @@ class MockRtpRtcp : public RtpRtcp { void(StreamDataCountersCallback*)); MOCK_CONST_METHOD0(GetSendChannelRtpStatisticsCallback, StreamDataCountersCallback*()); - MOCK_CONST_METHOD0(GetAcknowledgedPacketsObserver, - AcknowledgedPacketsObserver*()); MOCK_METHOD1(SetVideoBitrateAllocation, void(const VideoBitrateAllocation&)); MOCK_METHOD0(RtpSender, RTPSender*()); MOCK_CONST_METHOD0(RtpSender, const RTPSender*()); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 4855775beb..843f616b89 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -871,11 +871,6 @@ ModuleRtpRtcpImpl::GetSendChannelRtpStatisticsCallback() const { return rtp_sender_->GetRtpStatisticsCallback(); } -AcknowledgedPacketsObserver* ModuleRtpRtcpImpl::GetAcknowledgedPacketsObserver() - const { - return rtp_sender_.get(); -} - void ModuleRtpRtcpImpl::SetVideoBitrateAllocation( const VideoBitrateAllocation& bitrate) { rtcp_sender_.SetVideoBitrateAllocation(bitrate); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 286a8af5fd..4cb274b9dd 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -280,7 +280,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { StreamDataCountersCallback* callback) override; StreamDataCountersCallback* GetSendChannelRtpStatisticsCallback() const override; - AcknowledgedPacketsObserver* GetAcknowledgedPacketsObserver() const override; void OnReceivedNack( const std::vector& nack_sequence_numbers) override; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index ac5cf467ad..3933ba3cf7 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -42,7 +42,7 @@ class RateLimiter; class RtcEventLog; class RtpPacketToSend; -class RTPSender : public AcknowledgedPacketsObserver { +class RTPSender { public: RTPSender(bool audio, Clock* clock, @@ -175,8 +175,7 @@ class RTPSender : public AcknowledgedPacketsObserver { void SetRtt(int64_t rtt_ms); - void OnPacketsAcknowledged( - rtc::ArrayView sequence_numbers) override; + void OnPacketsAcknowledged(rtc::ArrayView sequence_numbers); private: // Maps capture time in milliseconds to send-side delay in milliseconds.