Revert of Use RtxReceiveStream. (patchset #5 id:80001 of https://codereview.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
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# 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-Original-Commit-Position: refs/heads/master@{#19653}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 3c39c0137afa274d1d524b150b50304b38a2847b
diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc
index 2b85452..ed44dad 100644
--- a/call/rampup_tests.cc
+++ b/call/rampup_tests.cc
@@ -207,9 +207,8 @@
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/call/video_receive_stream.h b/call/video_receive_stream.h
index 542027e..ef1c715 100644
--- a/call/video_receive_stream.h
+++ b/call/video_receive_stream.h
@@ -165,11 +165,6 @@
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/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc
index cd34e6b..a9dc11e 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -2188,11 +2188,6 @@
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/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc
index 1306082..c6adfd3 100644
--- a/media/engine/webrtcvideoengine_unittest.cc
+++ b/media/engine/webrtcvideoengine_unittest.cc
@@ -83,32 +83,6 @@
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 @@
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 @@
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 @@
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 @@
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/video/BUILD.gn b/video/BUILD.gn
index 29f5097..f3bba0e 100644
--- a/video/BUILD.gn
+++ b/video/BUILD.gn
@@ -60,9 +60,6 @@
"../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/video/end_to_end_tests.cc b/video/end_to_end_tests.cc
index 107aed8..a30b08a 100644
--- a/video/end_to_end_tests.cc
+++ b/video/end_to_end_tests.cc
@@ -1184,9 +1184,7 @@
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/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 51171d1..0dbb2f7 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -105,6 +105,7 @@
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 @@
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 @@
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 @@
}
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/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h
index 86e2de7..15af2e6 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -185,6 +185,8 @@
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/video/video_receive_stream.cc b/video/video_receive_stream.cc
index 9f5528b..6750c9e 100644
--- a/video/video_receive_stream.cc
+++ b/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 @@
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 @@
// 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/video/video_receive_stream.h b/video/video_receive_stream.h
index 2f29c90..833e189 100644
--- a/video/video_receive_stream.h
+++ b/video/video_receive_stream.h
@@ -38,7 +38,6 @@
class RTPFragmentationHeader;
class RtpStreamReceiverInterface;
class RtpStreamReceiverControllerInterface;
-class RtxReceiveStream;
class VCMTiming;
class VCMJitterEstimator;
@@ -142,7 +141,6 @@
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
