From 7b4ee155f103bb0838d2d277a2e411a9722b45ba Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Thu, 6 May 2021 11:52:03 +0200 Subject: [PATCH] Refactor RemoteEstimatorProxy As it will be heavily modified in a follow-up change, it's refactored as a separate commit to make commits small: * Extracted code to separate AddPacket and CullOldPackets methods * BuildFeedbackPacket now returns a packet. In the next iteration, it might not, so it needs to be able to decide when to increment the packet sequence number. * Documented some existing fields. The follow-up is in change I3dd58105f9fbfb111f176833cd4aa6b040c0e01d. Bug: webrtc:12689 Change-Id: I5ad0aee5a7da008f9e209f7c13bf299c12f9d1f3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217581 Commit-Queue: Victor Boivie Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#33942} --- .../remote_estimator_proxy.cc | 70 +++++++++++-------- .../remote_estimator_proxy.h | 27 +++++-- 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index a3361092bd..ba047755bf 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -54,6 +54,25 @@ RemoteEstimatorProxy::RemoteEstimatorProxy( RemoteEstimatorProxy::~RemoteEstimatorProxy() {} +void RemoteEstimatorProxy::AddPacket(int64_t sequence_number, + int64_t arrival_time_ms) { + packet_arrival_times_[sequence_number] = arrival_time_ms; +} + +void RemoteEstimatorProxy::MaybeCullOldPackets(int64_t sequence_number, + int64_t arrival_time_ms) { + if (periodic_window_start_seq_ && + packet_arrival_times_.lower_bound(*periodic_window_start_seq_) == + packet_arrival_times_.end()) { + // Start new feedback packet, cull old packets. + for (auto it = packet_arrival_times_.begin(); + it != packet_arrival_times_.end() && it->first < sequence_number && + arrival_time_ms - it->second >= send_config_.back_window->ms();) { + it = packet_arrival_times_.erase(it); + } + } +} + void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms, size_t payload_size, const RTPHeader& header) { @@ -69,16 +88,8 @@ void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms, seq = unwrapper_.Unwrap(header.extension.transportSequenceNumber); if (send_periodic_feedback_) { - if (periodic_window_start_seq_ && - packet_arrival_times_.lower_bound(*periodic_window_start_seq_) == - packet_arrival_times_.end()) { - // Start new feedback packet, cull old packets. - for (auto it = packet_arrival_times_.begin(); - it != packet_arrival_times_.end() && it->first < seq && - arrival_time_ms - it->second >= send_config_.back_window->ms();) { - it = packet_arrival_times_.erase(it); - } - } + MaybeCullOldPackets(seq, arrival_time_ms); + if (!periodic_window_start_seq_ || seq < *periodic_window_start_seq_) { periodic_window_start_seq_ = seq; } @@ -88,7 +99,7 @@ void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms, if (packet_arrival_times_.find(seq) != packet_arrival_times_.end()) return; - packet_arrival_times_[seq] = arrival_time_ms; + AddPacket(seq, arrival_time_ms); // Limit the range of sequence numbers to send feedback for. auto first_arrival_time_to_keep = packet_arrival_times_.lower_bound( @@ -204,10 +215,10 @@ void RemoteEstimatorProxy::SendPeriodicFeedbacks() { begin_iterator != packet_arrival_times_.cend(); begin_iterator = packet_arrival_times_.lower_bound(*periodic_window_start_seq_)) { - auto feedback_packet = std::make_unique(); - periodic_window_start_seq_ = BuildFeedbackPacket( - feedback_packet_count_++, media_ssrc_, *periodic_window_start_seq_, - begin_iterator, packet_arrival_times_.cend(), feedback_packet.get()); + auto feedback_packet = BuildFeedbackPacket( + /*include_timestamps=*/true, *periodic_window_start_seq_, + begin_iterator, packet_arrival_times_.cend(), + /*is_periodic_update=*/true); RTC_DCHECK(feedback_sender_ != nullptr); @@ -231,18 +242,15 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest( return; } - auto feedback_packet = std::make_unique( - feedback_request.include_timestamps); - int64_t first_sequence_number = sequence_number - feedback_request.sequence_count + 1; auto begin_iterator = packet_arrival_times_.lower_bound(first_sequence_number); auto end_iterator = packet_arrival_times_.upper_bound(sequence_number); - BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_, - first_sequence_number, begin_iterator, end_iterator, - feedback_packet.get()); + auto feedback_packet = BuildFeedbackPacket( + feedback_request.include_timestamps, first_sequence_number, + begin_iterator, end_iterator, /*is_periodic_update=*/false); // Clear up to the first packet that is included in this feedback packet. packet_arrival_times_.erase(packet_arrival_times_.begin(), begin_iterator); @@ -253,24 +261,27 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest( feedback_sender_(std::move(packets)); } -int64_t RemoteEstimatorProxy::BuildFeedbackPacket( - uint8_t feedback_packet_count, - uint32_t media_ssrc, +std::unique_ptr +RemoteEstimatorProxy::BuildFeedbackPacket( + bool include_timestamps, int64_t base_sequence_number, std::map::const_iterator begin_iterator, std::map::const_iterator end_iterator, - rtcp::TransportFeedback* feedback_packet) { + bool is_periodic_update) { RTC_DCHECK(begin_iterator != end_iterator); + auto feedback_packet = + std::make_unique(include_timestamps); + // TODO(sprang): Measure receive times in microseconds and remove the // conversions below. - feedback_packet->SetMediaSsrc(media_ssrc); + feedback_packet->SetMediaSsrc(media_ssrc_); // Base sequence number is the expected first sequence number. This is known, // but we might not have actually received it, so the base time shall be the // time of the first received packet in the feedback. feedback_packet->SetBase(static_cast(base_sequence_number & 0xFFFF), begin_iterator->second * 1000); - feedback_packet->SetFeedbackSequenceNumber(feedback_packet_count); + feedback_packet->SetFeedbackSequenceNumber(feedback_packet_count_++); int64_t next_sequence_number = base_sequence_number; for (auto it = begin_iterator; it != end_iterator; ++it) { if (!feedback_packet->AddReceivedPacket( @@ -285,7 +296,10 @@ int64_t RemoteEstimatorProxy::BuildFeedbackPacket( } next_sequence_number = it->first + 1; } - return next_sequence_number; + if (is_periodic_update) { + periodic_window_start_seq_ = next_sequence_number; + } + return feedback_packet; } } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h index 5aabfe1f0c..37ef7c43a1 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -77,19 +77,35 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { static const int kMaxNumberOfPackets; + // Records the fact that a packet with `sequence_number` arrived at + // `arrival_time_ms`. + void AddPacket(int64_t sequence_number, int64_t arrival_time_ms) + RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); + void MaybeCullOldPackets(int64_t sequence_number, int64_t arrival_time_ms) + RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); void SendPeriodicFeedbacks() RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); void SendFeedbackOnRequest(int64_t sequence_number, const FeedbackRequest& feedback_request) RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); - static int64_t BuildFeedbackPacket( - uint8_t feedback_packet_count, - uint32_t media_ssrc, + + // Returns a Transport Feedback packet with information about as many packets + // that has been received between [`begin_iterator`, `end_iterator`) that can + // fit in it. If `is_periodic_update`, this represents sending a periodic + // feedback message, which will make it update the + // `periodic_window_start_seq_` variable with the first packet that was not + // included in the feedback packet, so that the next update can continue from + // that sequence number. + // + // `include_timestamps` decide if the returned TransportFeedback should + // include timestamps. + std::unique_ptr BuildFeedbackPacket( + bool include_timestamps, int64_t base_sequence_number, std::map::const_iterator begin_iterator, // |begin_iterator| is inclusive. std::map::const_iterator end_iterator, // |end_iterator| is exclusive. - rtcp::TransportFeedback* feedback_packet); + bool is_periodic_update) RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); Clock* const clock_; const TransportFeedbackSender feedback_sender_; @@ -103,6 +119,9 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { uint32_t media_ssrc_ RTC_GUARDED_BY(&lock_); uint8_t feedback_packet_count_ RTC_GUARDED_BY(&lock_); SeqNumUnwrapper unwrapper_ RTC_GUARDED_BY(&lock_); + + // The next sequence number that should be the start sequence number during + // periodic reporting. Will be absl::nullopt before the first seen packet. absl::optional periodic_window_start_seq_ RTC_GUARDED_BY(&lock_); // Map unwrapped seq -> time. std::map packet_arrival_times_ RTC_GUARDED_BY(&lock_);