From f349e53ca54eefbb252b198cfd5f1a06fef43a40 Mon Sep 17 00:00:00 2001 From: Jianhui Dai Date: Wed, 1 Dec 2021 19:23:31 +0800 Subject: [PATCH] Reland "Call: Deduplicate SentPacket notifications" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of Ib121d5af07abe208bd7d36715a234f48cdabb032 In order to be backward compatible with bandwidth estimation behavior, pass all packets without a |packet_id| to downstream. Original change's description: > Call: Deduplicate SentPacket notifications > > When bundling is in effect, multiple senders may be sharing the same > transport. It means every |sent_packet| will be multiply notified from > different channels, WebRtcVoiceMediaChannel or WebRtcVideoChannel. > Record |last_sent_packet_| to deduplicate redundant notifications to > downstream objects. > > This CL reduces 50% PostTask/Wakeup of Dynamic Mode Pacer. > > [1] https://datatracker.ietf.org/doc/html/rfc8829#section-4.1.1 > [2] https://datatracker.ietf.org/doc/html/rfc8843 > > Bug: webrtc:13417 > Change-Id: Ib121d5af07abe208bd7d36715a234f48cdabb032 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/238720 > Reviewed-by: Markus Handell > Reviewed-by: Stefan Holmer > Reviewed-by: Henrik Boström > Reviewed-by: Tommi > Commit-Queue: Markus Handell > Cr-Commit-Position: refs/heads/main@{#35417} Bug: webrtc:13417, webrtc:13437 Change-Id: Ia5e9fbe5e4f47fe851935ca2484125411114cb68 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/239437 Reviewed-by: Jakob Ivarsson Reviewed-by: Markus Handell Reviewed-by: Henrik Boström Reviewed-by: Stefan Holmer Commit-Queue: Stefan Holmer Cr-Commit-Position: refs/heads/main@{#35492} --- call/call.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/call/call.cc b/call/call.cc index 0d58ed8ed9..b30b92f861 100644 --- a/call/call.cc +++ b/call/call.cc @@ -466,6 +466,10 @@ class Call final : public webrtc::Call, bool is_started_ RTC_GUARDED_BY(worker_thread_) = false; + RTC_NO_UNIQUE_ADDRESS SequenceChecker sent_packet_sequence_checker_; + absl::optional last_sent_packet_ + RTC_GUARDED_BY(sent_packet_sequence_checker_); + RTC_DISALLOW_COPY_AND_ASSIGN(Call); }; } // namespace internal @@ -814,6 +818,7 @@ Call::Call(Clock* clock, RTC_DCHECK(worker_thread_->IsCurrent()); send_transport_sequence_checker_.Detach(); + sent_packet_sequence_checker_.Detach(); // Do not remove this call; it is here to convince the compiler that the // WebRTC source timestamp string needs to be in the final binary. @@ -1382,6 +1387,20 @@ void Call::OnUpdateSyncGroup(webrtc::AudioReceiveStream& stream, } void Call::OnSentPacket(const rtc::SentPacket& sent_packet) { + RTC_DCHECK_RUN_ON(&sent_packet_sequence_checker_); + // When bundling is in effect, multiple senders may be sharing the same + // transport. It means every |sent_packet| will be multiply notified from + // different channels, WebRtcVoiceMediaChannel or WebRtcVideoChannel. Record + // |last_sent_packet_| to deduplicate redundant notifications to downstream. + // (https://crbug.com/webrtc/13437): Pass all packets without a |packet_id| to + // downstream. + if (last_sent_packet_.has_value() && last_sent_packet_->packet_id != -1 && + last_sent_packet_->packet_id == sent_packet.packet_id && + last_sent_packet_->send_time_ms == sent_packet.send_time_ms) { + return; + } + last_sent_packet_ = sent_packet; + // In production and with most tests, this method will be called on the // network thread. However some test classes such as DirectTransport don't // incorporate a network thread. This means that tests for RtpSenderEgress