Revert of Use RtxReceiveStream. (patchset #5 id:80001 of https://code…

…review.webrtc.org/3008773002/ ) Reason for revert: A few perf tests broken, including RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx RampUpTest.UpDownUpTransportSequenceNumberRtx RampUpTest.UpDownUpTransportSequenceNumberPacketLoss Original issue's description: > Use RtxReceiveStream. > > This also has the beneficial side-effect that when a media stream > which is protected by FlexFEC receives an RTX retransmission, the > retransmitted media packet is passed into the FlexFEC machinery, > which should improve its ability to recover packets via FEC. > > BUG=webrtc:7135 > > Review-Url: https://codereview.webrtc.org/3008773002 > Cr-Commit-Position: refs/heads/master@{#19649} > Committed: https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152961af27033475f7 [email protected],[email protected],[email protected],[email protected] # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/3010983002 Cr-Commit-Position: refs/heads/master@{#19653}
pristineio · Sep 4, 2017 · 3c39c01 · 3c39c01
1 parent 9af8f94
commit 3c39c01
Show file tree

Hide file tree

Showing 10 changed files with 57 additions and 72 deletions.
diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc
@@ -207,9 +207,8 @@ void RampUpTester::ModifyVideoConfigs(
       recv_config.rtp.ulpfec.ulpfec_payload_type =
           send_config->rtp.ulpfec.ulpfec_payload_type;
       if (rtx_) {
-        recv_config.rtp.rtx_associated_payload_types
-            [send_config->rtp.ulpfec.red_rtx_payload_type] =
-            send_config->rtp.ulpfec.red_payload_type;
+        recv_config.rtp.ulpfec.red_rtx_payload_type =
+            send_config->rtp.ulpfec.red_rtx_payload_type;
       }
     }
 

diff --git a/webrtc/call/video_receive_stream.h b/webrtc/call/video_receive_stream.h
@@ -165,11 +165,6 @@ class VideoReceiveStream {
       NackConfig nack;
 
       // See UlpfecConfig for description.
-      // TODO(nisse): UlpfecConfig includes the field red_rtx_payload_type,
-      // which duplicates info in the rtx_associated_payload_types mapping. So
-      // delete the use of UlpfecConfig here, and replace by the values which
-      // make sense in this context, likely those are ulpfec_payload_type_ and
-      // red_payload_type_.
       UlpfecConfig ulpfec;
 
       // SSRC for retransmissions.

diff --git a/webrtc/media/engine/webrtcvideoengine.cc b/webrtc/media/engine/webrtcvideoengine.cc
@@ -2188,11 +2188,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureCodecs(
 
   config_.rtp.nack.rtp_history_ms =
       HasNack(recv_codecs.begin()->codec) ? kNackHistoryMs : 0;
-  if (config_.rtp.ulpfec.red_rtx_payload_type != -1) {
-    config_.rtp
-        .rtx_associated_payload_types[config_.rtp.ulpfec.red_rtx_payload_type] =
-        config_.rtp.ulpfec.red_payload_type;
-  }
 }
 
 void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureFlexfecCodec(

diff --git a/webrtc/media/engine/webrtcvideoengine_unittest.cc b/webrtc/media/engine/webrtcvideoengine_unittest.cc
@@ -83,32 +83,6 @@ bool HasRtxCodec(const std::vector<cricket::VideoCodec>& codecs,
   return false;
 }
 
-// TODO(nisse): Duplicated in call.cc.
-const int* FindKeyByValue(const std::map<int, int>& m, int v) {
-  for (const auto& kv : m) {
-    if (kv.second == v)
-      return &kv.first;
-  }
-  return nullptr;
-}
-
-bool HasRtxReceiveAssociation(
-    const webrtc::VideoReceiveStream::Config& config,
-    int payload_type) {
-  return FindKeyByValue(config.rtp.rtx_associated_payload_types,
-                        payload_type) != nullptr;
-}
-
-// Check that there's an Rtx payload type for each decoder.
-bool VerifyRtxReceiveAssociations(
-    const webrtc::VideoReceiveStream::Config& config) {
-  for (const auto& decoder : config.decoders) {
-    if (!HasRtxReceiveAssociation(config, decoder.payload_type))
-      return false;
-  }
-  return true;
-}
-
 rtc::scoped_refptr<webrtc::VideoFrameBuffer> CreateBlackFrameBuffer(
     int width,
     int height) {
@@ -138,6 +112,15 @@ cricket::MediaConfig GetMediaConfig() {
   return media_config;
 }
 
+// TODO(nisse): Duplicated in call.cc.
+const int* FindKeyByValue(const std::map<int, int>& m, int v) {
+  for (const auto& kv : m) {
+    if (kv.second == v)
+      return &kv.first;
+  }
+  return nullptr;
+}
+
 }  // namespace
 
 namespace cricket {
@@ -1333,12 +1316,9 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithSimAndRtx) {
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
   EXPECT_FALSE(
       recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
-  EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig()))
+  EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
+            recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
       << "RTX should be mapped for all decoders/payload types.";
-  EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(),
-                                          GetEngineCodec("red").id))
-      << "RTX should be mapped for the RED payload type";
-
   EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 
@@ -1349,12 +1329,6 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithRtx) {
   params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]);
   FakeVideoReceiveStream* recv_stream = AddRecvStream(params);
   EXPECT_EQ(kRtxSsrcs1[0], recv_stream->GetConfig().rtp.rtx_ssrc);
-
-  EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig()))
-      << "RTX should be mapped for all decoders/payload types.";
-  EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(),
-                                          GetEngineCodec("red").id))
-      << "RTX should be mapped for the RED payload type";
 }
 
 TEST_F(WebRtcVideoChannelTest, RecvStreamNoRtx) {
@@ -3822,11 +3796,9 @@ TEST_F(WebRtcVideoChannelTest, DefaultReceiveStreamReconfiguresToUseRtx) {
   recv_stream = fake_call_->GetVideoReceiveStreams()[0];
   EXPECT_FALSE(
       recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
-  EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig()))
+  EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
+            recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
       << "RTX should be mapped for all decoders/payload types.";
-  EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(),
-                                          GetEngineCodec("red").id))
-      << "RTX should be mapped also for the RED payload type";
   EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 

diff --git a/webrtc/video/BUILD.gn b/webrtc/video/BUILD.gn
@@ -60,9 +60,6 @@ rtc_static_library("video") {
     "../call:call_interfaces",
     "../call:rtp_interfaces",
     "../call:video_stream_api",
-
-    # For RtxReceiveStream.
-    "../call:rtp_receiver",
     "../common_video",
     "../logging:rtc_event_log_api",
     "../media:rtc_media_base",

diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
@@ -1184,9 +1184,7 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) {
         send_config->rtp.rtx.payload_type = kSendRtxPayloadType;
         (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0];
         (*receive_configs)[0]
-            .rtp.rtx_associated_payload_types[(payload_type_ == kRedPayloadType)
-                                                  ? kRtxRedPayloadType
-                                                  : kSendRtxPayloadType] =
+            .rtp.rtx_associated_payload_types[kSendRtxPayloadType] =
             payload_type_;
       }
       // Configure encoding and decoding with VP8, since generic packetization

diff --git a/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc
@@ -105,6 +105,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
       rtp_receive_statistics_(ReceiveStatistics::Create(clock_)),
       ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)),
       receiving_(false),
+      restored_packet_in_use_(false),
       last_packet_log_ms_(-1),
       rtp_rtcp_(CreateRtpRtcpModule(rtp_receive_statistics_.get(),
                                     transport,
@@ -145,8 +146,12 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
   rtp_receive_statistics_->SetMaxReorderingThreshold(max_reordering_threshold);
 
   if (config_.rtp.rtx_ssrc) {
-    // Needed for rtp_payload_registry_.RtxEnabled().
     rtp_payload_registry_.SetRtxSsrc(config_.rtp.rtx_ssrc);
+
+    for (const auto& kv : config_.rtp.rtx_associated_payload_types) {
+      RTC_DCHECK_NE(kv.first, 0);
+      rtp_payload_registry_.SetRtxPayloadType(kv.first, kv.second);
+    }
   }
 
   if (IsUlpfecEnabled()) {
@@ -163,6 +168,11 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
     strncpy(red_codec.plName, "red", sizeof(red_codec.plName));
     red_codec.plType = config_.rtp.ulpfec.red_payload_type;
     RTC_CHECK(AddReceiveCodec(red_codec));
+    if (config_.rtp.ulpfec.red_rtx_payload_type != -1) {
+      rtp_payload_registry_.SetRtxPayloadType(
+          config_.rtp.ulpfec.red_rtx_payload_type,
+          config_.rtp.ulpfec.red_payload_type);
+    }
   }
 
   if (config_.rtp.rtcp_xr.receiver_reference_time_report)
@@ -485,7 +495,31 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader(
     }
     ulpfec_receiver_->ProcessReceivedFec();
   } else if (rtp_payload_registry_.IsRtx(header)) {
-    LOG(LS_WARNING) << "Unexpected RTX packet on media ssrc";
+    if (header.headerLength + header.paddingLength == packet_length) {
+      // This is an empty packet and should be silently dropped before trying to
+      // parse the RTX header.
+      return;
+    }
+    // Remove the RTX header and parse the original RTP header.
+    if (packet_length < header.headerLength)
+      return;
+    if (packet_length > sizeof(restored_packet_))
+      return;
+    if (restored_packet_in_use_) {
+      LOG(LS_WARNING) << "Multiple RTX headers detected, dropping packet.";
+      return;
+    }
+    if (!rtp_payload_registry_.RestoreOriginalPacket(
+            restored_packet_, packet, &packet_length, config_.rtp.remote_ssrc,
+            header)) {
+      LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header ssrc: "
+                      << header.ssrc << " payload type: "
+                      << static_cast<int>(header.payloadType);
+      return;
+    }
+    restored_packet_in_use_ = true;
+    OnRecoveredPacket(restored_packet_, packet_length);
+    restored_packet_in_use_ = false;
   }
 }
 

diff --git a/webrtc/video/rtp_video_stream_receiver.h b/webrtc/video/rtp_video_stream_receiver.h
@@ -185,6 +185,8 @@ class RtpVideoStreamReceiver : public RtpData,
 
   rtc::SequencedTaskChecker worker_task_checker_;
   bool receiving_ GUARDED_BY(worker_task_checker_);
+  uint8_t restored_packet_[IP_PACKET_SIZE] GUARDED_BY(worker_task_checker_);
+  bool restored_packet_in_use_ GUARDED_BY(worker_task_checker_);
   int64_t last_packet_log_ms_ GUARDED_BY(worker_task_checker_);
 
   const std::unique_ptr<RtpRtcp> rtp_rtcp_;

diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
@@ -17,7 +17,6 @@
 #include <utility>
 
 #include "webrtc/call/rtp_stream_receiver_controller_interface.h"
-#include "webrtc/call/rtx_receive_stream.h"
 #include "webrtc/common_types.h"
 #include "webrtc/common_video/h264/profile_level_id.h"
 #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
@@ -33,7 +32,6 @@
 #include "webrtc/rtc_base/location.h"
 #include "webrtc/rtc_base/logging.h"
 #include "webrtc/rtc_base/optional.h"
-#include "webrtc/rtc_base/ptr_util.h"
 #include "webrtc/rtc_base/trace_event.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/field_trial.h"
@@ -122,7 +120,7 @@ VideoReceiveStream::VideoReceiveStream(
     decoder_payload_types.insert(decoder.payload_type);
   }
 
-  video_receiver_.SetRenderDelay(config_.render_delay_ms);
+  video_receiver_.SetRenderDelay(config.render_delay_ms);
 
   jitter_estimator_.reset(new VCMJitterEstimator(clock_));
   frame_buffer_.reset(new video_coding::FrameBuffer(
@@ -133,12 +131,9 @@ VideoReceiveStream::VideoReceiveStream(
   // Register with RtpStreamReceiverController.
   media_receiver_ = receiver_controller->CreateReceiver(
       config_.rtp.remote_ssrc, &rtp_video_stream_receiver_);
-  if (config_.rtp.rtx_ssrc) {
-    rtx_receive_stream_ = rtc::MakeUnique<RtxReceiveStream>(
-        &rtp_video_stream_receiver_, config.rtp.rtx_associated_payload_types,
-        config_.rtp.remote_ssrc);
+  if (config.rtp.rtx_ssrc) {
     rtx_receiver_ = receiver_controller->CreateReceiver(
-        config_.rtp.rtx_ssrc, rtx_receive_stream_.get());
+        config_.rtp.rtx_ssrc, &rtp_video_stream_receiver_);
   }
 }
 

diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h
@@ -38,7 +38,6 @@ class ProcessThread;
 class RTPFragmentationHeader;
 class RtpStreamReceiverInterface;
 class RtpStreamReceiverControllerInterface;
-class RtxReceiveStream;
 class VCMTiming;
 class VCMJitterEstimator;
 
@@ -142,7 +141,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
   std::unique_ptr<video_coding::FrameBuffer> frame_buffer_;
 
   std::unique_ptr<RtpStreamReceiverInterface> media_receiver_;
-  std::unique_ptr<RtxReceiveStream> rtx_receive_stream_;
   std::unique_ptr<RtpStreamReceiverInterface> rtx_receiver_;
 
   // Whenever we are in an undecodable state (stream has just started or due to