[WebRtcVideoReceiveStream] Add ability to config flexfec post init.

Bug: webrtc:11993
Change-Id: I35d7e645e18b7cb4a86645ea52c8958063f13d3c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269243
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37661}

call/video_receive_stream.h

@@ -287,6 +287,13 @@ class VideoReceiveStreamInterface : public MediaReceiveStreamInterface {
 
   virtual void SetRtcpMode(RtcpMode mode) = 0;
 
+  // Sets or clears a flexfec RTP sink. This affects `rtp.packet_sink_` and
+  // `rtp.protected_by_flexfec` parts of the configuration. Must be called on
+  // the packet delivery thread.
+  // TODO(bugs.webrtc.org/11993): Packet delivery thread today means `worker
+  // thread` but will be `network thread`.
+  virtual void SetFlexFecProtection(RtpPacketSinkInterface* flexfec_sink) = 0;
+
  protected:
   virtual ~VideoReceiveStreamInterface() {}
 };

media/engine/fake_webrtc_call.h

@@ -284,6 +284,11 @@ class FakeVideoReceiveStream final
     config_.rtp.rtcp_mode = mode;
   }
 
+  void SetFlexFecProtection(webrtc::RtpPacketSinkInterface* sink) override {
+    config_.rtp.packet_sink_ = sink;
+    config_.rtp.protected_by_flexfec = (sink != nullptr);
+  }
+
   void Start() override;
   void Stop() override;
 

media/engine/webrtc_video_engine.cc

@@ -3060,9 +3060,9 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFlexFecPayload(
     flexfec_stream_->SetPayloadType(payload_type);
 
     if (payload_type == -1) {
-      // TODO(tommi): Delete `flexfec_stream_` and clear references to it from
-      // `stream_` without having to recreate `stream_`.
-      flexfec_needs_recreation = true;
+      stream_->SetFlexFecProtection(nullptr);
+      call_->DestroyFlexfecReceiveStream(flexfec_stream_);
+      flexfec_stream_ = nullptr;
     }
   } else if (payload_type != -1) {
     flexfec_config_.payload_type = payload_type;

media/engine/webrtc_video_engine_unittest.cc

@@ -4231,11 +4231,11 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest,
 }
 
 // Test changing the configuration after a video stream has been created with
-// flexfec enabled and then turn off flexfec. This will result in the video
-// stream being recreated because the flexfec stream pointer is injected to the
-// video stream at construction and that config needs to be torn down.
+// flexfec enabled and then turn off flexfec. This will not result in the video
+// stream being recreated. The flexfec stream pointer that's held by the video
+// stream will be set/cleared as dictated by the configuration change.
 TEST_F(WebRtcVideoChannelFlexfecRecvTest,
-       DisablingFlexfecRecreatesVideoReceiveStream) {
+       DisablingFlexfecDoesNotRecreateVideoReceiveStream) {
   cricket::VideoRecvParameters recv_parameters;
   recv_parameters.codecs.push_back(GetEngineCodec("VP8"));
   recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03"));
@@ -4258,9 +4258,10 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest,
   recv_parameters.codecs.clear();
   recv_parameters.codecs.push_back(GetEngineCodec("VP8"));
   ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters));
-  // Now the count of created streams will be 3 since the video stream had to
-  // be recreated on account of the flexfec stream being deleted.
-  EXPECT_EQ(3, fake_call_->GetNumCreatedReceiveStreams())
+  // The count of created streams should remain 2 since the video stream will
+  // have been reconfigured to not reference flexfec and not recreated on
+  // account of the flexfec stream being deleted.
+  EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams())
       << "Disabling FlexFEC should not recreate VideoReceiveStreamInterface.";
   EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size())
       << "Disabling FlexFEC should not destroy VideoReceiveStreamInterface.";

video/rtp_video_stream_receiver2.cc

@@ -229,6 +229,7 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2(
       ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc,
                                               this,
                                               config->rtp.extensions)),
+      packet_sink_(config->rtp.packet_sink_),
       receiving_(false),
       last_packet_log_ms_(-1),
       rtp_rtcp_(CreateRtpRtcpModule(
@@ -661,8 +662,8 @@ void RtpVideoStreamReceiver2::OnRtpPacket(const RtpPacketReceived& packet) {
     rtp_receive_statistics_->OnRtpPacket(packet);
   }
 
-  if (config_.rtp.packet_sink_) {
-    config_.rtp.packet_sink_->OnRtpPacket(packet);
+  if (packet_sink_) {
+    packet_sink_->OnRtpPacket(packet);
   }
 }
 
@@ -928,6 +929,12 @@ void RtpVideoStreamReceiver2::SetRtcpMode(RtcpMode mode) {
   rtp_rtcp_->SetRTCPStatus(mode);
 }
 
+void RtpVideoStreamReceiver2::SetPacketSink(
+    RtpPacketSinkInterface* packet_sink) {
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+  packet_sink_ = packet_sink;
+}
+
 absl::optional<int64_t> RtpVideoStreamReceiver2::LastReceivedPacketMs() const {
   RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
   if (last_received_rtp_system_time_) {

video/rtp_video_stream_receiver2.h

@@ -187,6 +187,13 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender,
   // Forwards the call to set rtcp_sender_ to the RTCP mode of the rtcp sender.
   void SetRtcpMode(RtcpMode mode);
 
+  // Sets or clears the callback sink that gets called for RTP packets. Used for
+  // packet handlers such as FlexFec. Must be called on the packet delivery
+  // thread (same context as `OnRtpPacket` is called on).
+  // TODO(bugs.webrtc.org/11993): Packet delivery thread today means `worker
+  // thread` but will be `network thread`.
+  void SetPacketSink(RtpPacketSinkInterface* packet_sink);
+
   absl::optional<int64_t> LastReceivedPacketMs() const;
   absl::optional<int64_t> LastReceivedKeyframePacketMs() const;
 
@@ -318,6 +325,7 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender,
   // that belong to the network thread. Once the packets are fully delivered
   // on the network thread, this comment will be deleted.
   RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_sequence_checker_;
+  RtpPacketSinkInterface* packet_sink_ RTC_GUARDED_BY(packet_sequence_checker_);
   bool receiving_ RTC_GUARDED_BY(packet_sequence_checker_);
   int64_t last_packet_log_ms_ RTC_GUARDED_BY(packet_sequence_checker_);
 

video/video_receive_stream2.cc

@@ -507,6 +507,16 @@ void VideoReceiveStream2::SetRtcpMode(RtcpMode mode) {
   rtp_video_stream_receiver_.SetRtcpMode(mode);
 }
 
+void VideoReceiveStream2::SetFlexFecProtection(
+    RtpPacketSinkInterface* flexfec_sink) {
+  RTC_DCHECK_RUN_ON(&packet_sequence_checker_);
+  rtp_video_stream_receiver_.SetPacketSink(flexfec_sink);
+  // TODO(tommi): Stop using the config struct for the internal state.
+  const_cast<RtpPacketSinkInterface*&>(config_.rtp.packet_sink_) = flexfec_sink;
+  const_cast<bool&>(config_.rtp.protected_by_flexfec) =
+      (flexfec_sink != nullptr);
+}
+
 void VideoReceiveStream2::CreateAndRegisterExternalDecoder(
     const Decoder& decoder) {
   TRACE_EVENT0("webrtc",

video/video_receive_stream2.h

@@ -146,6 +146,7 @@ class VideoReceiveStream2
   bool transport_cc() const override;
   void SetTransportCc(bool transport_cc) override;
   void SetRtcpMode(RtcpMode mode) override;
+  void SetFlexFecProtection(RtpPacketSinkInterface* flexfec_sink) override;
 
   webrtc::VideoReceiveStreamInterface::Stats GetStats() const override;