From 65ab5fd728a467350a937cf5eec9a9e75e7a6f09 Mon Sep 17 00:00:00 2001
From: Danil Chapovalov <danilchap@webrtc.org>
Date: Wed, 1 Feb 2023 18:42:07 +0100
Subject: [PATCH] Cleanup RemoteEstimatorProxy::IncomingPacket

relax DCHECK and explain when it previous version could be hit.
Use concise versions of the GetExtension functions.
Reduce scope of the `lock_`

Bug: None
Change-Id: Iafc570ffe7e5b2dcbdfe166b26b140f7959c28c7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/291711
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39246}
---
 .../remote_estimator_proxy.cc                 | 33 ++++++++++---------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
index 367260491f..01a23ba9b0 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -88,28 +88,31 @@ void RemoteEstimatorProxy::IncomingPacket(const RtpPacketReceived& packet) {
     RTC_LOG(LS_WARNING) << "Arrival time not set.";
     return;
   }
-  MutexLock lock(&lock_);
-  send_periodic_feedback_ = packet.HasExtension<TransportSequenceNumber>();
 
   Packet internal_packet = {.arrival_time = packet.arrival_time(),
                             .size = DataSize::Bytes(packet.size()),
                             .ssrc = packet.Ssrc()};
   uint16_t seqnum;
-  absl::optional<FeedbackRequest> feedback_request;
-  if (!packet.GetExtension<TransportSequenceNumber>(&seqnum)) {
-    if (!packet.GetExtension<TransportSequenceNumberV2>(&seqnum,
-                                                        &feedback_request)) {
-      RTC_DCHECK_NOTREACHED() << " Expected transport sequence number.";
-      return;
-    }
+  if (packet.GetExtension<TransportSequenceNumber>(&seqnum) ||
+      packet.GetExtension<TransportSequenceNumberV2>(
+          &seqnum, &internal_packet.feedback_request)) {
+    internal_packet.transport_sequence_number = seqnum;
+  } else {
+    // This function expected to be called only for packets that have
+    // TransportSequenceNumber rtp header extension, however malformed RTP
+    // packet may contain unparsable TransportSequenceNumber.
+    RTC_DCHECK(packet.HasExtension<TransportSequenceNumber>() ||
+               packet.HasExtension<TransportSequenceNumberV2>())
+        << " Expected transport sequence number.";
+    return;
   }
-  internal_packet.transport_sequence_number = seqnum;
-  internal_packet.feedback_request = feedback_request;
 
-  uint32_t send_time_24_bits;
-  if (packet.GetExtension<AbsoluteSendTime>(&send_time_24_bits)) {
-    internal_packet.absolute_send_time_24bits = send_time_24_bits;
-  }
+  internal_packet.absolute_send_time_24bits =
+      packet.GetExtension<AbsoluteSendTime>();
+
+  MutexLock lock(&lock_);
+  send_periodic_feedback_ = packet.HasExtension<TransportSequenceNumber>();
+
   IncomingPacket(internal_packet);
 }