From b69b81944c4abdb65294624435d48fbd2d83a64a Mon Sep 17 00:00:00 2001
From: Tommi <tommi@webrtc.org>
Date: Fri, 12 Aug 2022 11:03:34 +0200
Subject: [PATCH] Conditionally construct UlpfecReceiver

Bug: none
Change-Id: I986803dcba5d7b6bb6e58e6a51fb38a216c1d03d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271120
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37764}
---
 modules/rtp_rtcp/source/ulpfec_receiver.cc |  7 ++++-
 video/rtp_video_stream_receiver2.cc        | 31 +++++++++++++++-------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/modules/rtp_rtcp/source/ulpfec_receiver.cc b/modules/rtp_rtcp/source/ulpfec_receiver.cc
index f226a66786..4090d99e8d 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver.cc
+++ b/modules/rtp_rtcp/source/ulpfec_receiver.cc
@@ -31,7 +31,12 @@ UlpfecReceiver::UlpfecReceiver(uint32_t ssrc,
       clock_(clock),
       extensions_(extensions),
       recovered_packet_callback_(callback),
-      fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {}
+      fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {
+  // TODO(tommi, brandtr): Once considerations for red have been split
+  // away from this implementation, we can require the ulpfec payload type
+  // to always be valid and use uint8 for storage (as is done elsewhere).
+  RTC_DCHECK_GE(ulpfec_payload_type_, -1);
+}
 
 UlpfecReceiver::~UlpfecReceiver() {
   RTC_DCHECK_RUN_ON(&sequence_checker_);
diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc
index ac12224c75..536d16f89d 100644
--- a/video/rtp_video_stream_receiver2.cc
+++ b/video/rtp_video_stream_receiver2.cc
@@ -123,6 +123,25 @@ std::unique_ptr<NackRequester> MaybeConstructNackModule(
                                          keyframe_request_sender, field_trials);
 }
 
+std::unique_ptr<UlpfecReceiver> MaybeConstructUlpfecReceiver(
+    const VideoReceiveStreamInterface::Config::Rtp& rtp,
+    RecoveredPacketReceiver* callback,
+    Clock* clock) {
+  RTC_DCHECK_GE(rtp.ulpfec_payload_type, -1);
+  if (rtp.red_payload_type == -1)
+    return nullptr;
+
+  // TODO(tommi, brandtr): Consider including this check too once
+  // `UlpfecReceiver` has been updated to not consider both red and ulpfec
+  // payload ids.
+  //  if (rtp.ulpfec_payload_type == -1)
+  //    return nullptr;
+
+  return std::make_unique<UlpfecReceiver>(rtp.remote_ssrc,
+                                          rtp.ulpfec_payload_type, callback,
+                                          rtp.extensions, clock);
+}
+
 static const int kPacketLogIntervalMs = 10000;
 
 }  // namespace
@@ -237,12 +256,7 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2(
       forced_playout_delay_max_ms_("max_ms", absl::nullopt),
       forced_playout_delay_min_ms_("min_ms", absl::nullopt),
       rtp_receive_statistics_(rtp_receive_statistics),
-      ulpfec_receiver_(
-          std::make_unique<UlpfecReceiver>(config->rtp.remote_ssrc,
-                                           config_.rtp.ulpfec_payload_type,
-                                           this,
-                                           config->rtp.extensions,
-                                           clock)),
+      ulpfec_receiver_(MaybeConstructUlpfecReceiver(config->rtp, this, clock)),
       packet_sink_(config->rtp.packet_sink_),
       receiving_(false),
       last_packet_log_ms_(-1),
@@ -1033,10 +1047,9 @@ void RtpVideoStreamReceiver2::ParseAndHandleEncapsulatingHeader(
       // packets.
       NotifyReceiverOfEmptyPacket(packet.SequenceNumber());
     }
-    if (!ulpfec_receiver_->AddReceivedRedPacket(packet)) {
-      return;
+    if (ulpfec_receiver_ && ulpfec_receiver_->AddReceivedRedPacket(packet)) {
+      ulpfec_receiver_->ProcessReceivedFec();
     }
-    ulpfec_receiver_->ProcessReceivedFec();
   }
 }