Revert of Use RtxReceiveStream. (patchset #5 id:320001 of https://codereview.webrtc.org/3006063002/ )
Reason for revert:
This change appears to break ulpfec, with severe regressions, e.g., for webrtc_perf_test FullStackTest.ForemanCifPlr5Ulpfec
Original issue's description:
> Reland of Use RtxReceiveStream. (patchset #1 id:1 of https://codereview.webrtc.org/3010983002/ )
>
> Reason for revert:
> Intend to fix perf failures and reland.
>
> Original issue's description:
> > 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-Commit-Position: refs/heads/master@{#19653}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150b50304b38a2847b
>
> 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
> BUG=webrtc:7135
>
> Review-Url: https://codereview.webrtc.org/3006063002
> Cr-Commit-Position: refs/heads/master@{#19715}
> Committed: https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc158d7b8fdf7ec3d78
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3007303002
Cr-Original-Commit-Position: refs/heads/master@{#19744}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 8e7eee035178a7f10e19883681b5eaa4a7523107
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/rtx_receive_stream.cc b/call/rtx_receive_stream.cc
index 6a5432f..1646352 100644
--- a/call/rtx_receive_stream.cc
+++ b/call/rtx_receive_stream.cc
@@ -11,21 +11,17 @@
#include <utility>
#include "webrtc/call/rtx_receive_stream.h"
-#include "webrtc/modules/rtp_rtcp/include/receive_statistics.h"
#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
#include "webrtc/rtc_base/logging.h"
namespace webrtc {
-RtxReceiveStream::RtxReceiveStream(
- RtpPacketSinkInterface* media_sink,
- std::map<int, int> associated_payload_types,
- uint32_t media_ssrc,
- ReceiveStatistics* rtp_receive_statistics /* = nullptr */)
+RtxReceiveStream::RtxReceiveStream(RtpPacketSinkInterface* media_sink,
+ std::map<int, int> associated_payload_types,
+ uint32_t media_ssrc)
: media_sink_(media_sink),
associated_payload_types_(std::move(associated_payload_types)),
- media_ssrc_(media_ssrc),
- rtp_receive_statistics_(rtp_receive_statistics) {
+ media_ssrc_(media_ssrc) {
if (associated_payload_types_.empty()) {
LOG(LS_WARNING)
<< "RtxReceiveStream created with empty payload type mapping.";
@@ -35,12 +31,6 @@
RtxReceiveStream::~RtxReceiveStream() = default;
void RtxReceiveStream::OnRtpPacket(const RtpPacketReceived& rtx_packet) {
- if (rtp_receive_statistics_) {
- RTPHeader header;
- rtx_packet.GetHeader(&header);
- rtp_receive_statistics_->IncomingPacket(header, rtx_packet.size(),
- false /* retransmitted */);
- }
rtc::ArrayView<const uint8_t> payload = rtx_packet.payload();
if (payload.size() < kRtxHeaderSize) {
diff --git a/call/rtx_receive_stream.h b/call/rtx_receive_stream.h
index c288a27..418775c 100644
--- a/call/rtx_receive_stream.h
+++ b/call/rtx_receive_stream.h
@@ -17,20 +17,13 @@
namespace webrtc {
-class ReceiveStatistics;
-
// This class is responsible for RTX decapsulation. The resulting media packets
// are passed on to a sink representing the associated media stream.
class RtxReceiveStream : public RtpPacketSinkInterface {
public:
RtxReceiveStream(RtpPacketSinkInterface* media_sink,
std::map<int, int> associated_payload_types,
- uint32_t media_ssrc,
- // TODO(nisse): Delete this argument, and
- // corresponding member variable, by moving the
- // responsibility for rtcp feedback to
- // RtpStreamReceiverController.
- ReceiveStatistics* rtp_receive_statistics = nullptr);
+ uint32_t media_ssrc);
~RtxReceiveStream() override;
// RtpPacketSinkInterface.
void OnRtpPacket(const RtpPacketReceived& packet) override;
@@ -42,7 +35,6 @@
// TODO(nisse): Ultimately, the media receive stream shouldn't care about the
// ssrc, and we should delete this.
const uint32_t media_ssrc_;
- ReceiveStatistics* const rtp_receive_statistics_;
};
} // namespace webrtc
diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h
index f199ca1..e51184e 100644
--- a/call/video_receive_stream.h
+++ b/call/video_receive_stream.h
@@ -167,11 +167,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 a613dc6..9c0031a 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -2187,11 +2187,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 16f6bd0..7756ff4 100644
--- a/video/BUILD.gn
+++ b/video/BUILD.gn
@@ -61,9 +61,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 97b9ee7..644319c 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 1dbd869..a47055a 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -86,7 +86,6 @@
RtcpRttStats* rtt_stats,
PacketRouter* packet_router,
const VideoReceiveStream::Config* config,
- ReceiveStatistics* rtp_receive_statistics,
ReceiveStatisticsProxy* receive_stats_proxy,
ProcessThread* process_thread,
NackSender* nack_sender,
@@ -103,11 +102,12 @@
this,
this,
&rtp_payload_registry_)),
- rtp_receive_statistics_(rtp_receive_statistics),
+ 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_,
+ rtp_rtcp_(CreateRtpRtcpModule(rtp_receive_statistics_.get(),
transport,
rtt_stats,
receive_stats_proxy,
@@ -146,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()) {
@@ -164,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)
@@ -486,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 15afc18..f65280e 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -72,7 +72,6 @@
RtcpRttStats* rtt_stats,
PacketRouter* packet_router,
const VideoReceiveStream::Config* config,
- ReceiveStatistics* rtp_receive_statistics,
ReceiveStatisticsProxy* receive_stats_proxy,
ProcessThread* process_thread,
NackSender* nack_sender,
@@ -181,11 +180,13 @@
const std::unique_ptr<RtpHeaderParser> rtp_header_parser_;
const std::unique_ptr<RtpReceiver> rtp_receiver_;
- ReceiveStatistics* const rtp_receive_statistics_;
+ const std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
std::unique_ptr<UlpfecReceiver> ulpfec_receiver_;
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/rtp_video_stream_receiver_unittest.cc b/video/rtp_video_stream_receiver_unittest.cc
index 4f2dca7..d309397 100644
--- a/video/rtp_video_stream_receiver_unittest.cc
+++ b/video/rtp_video_stream_receiver_unittest.cc
@@ -126,14 +126,11 @@
process_thread_(ProcessThread::Create("TestThread")) {}
void SetUp() {
- rtp_receive_statistics_ =
- rtc::WrapUnique(ReceiveStatistics::Create(Clock::GetRealTimeClock()));
- rtp_video_stream_receiver_ = rtc::MakeUnique<RtpVideoStreamReceiver>(
+ rtp_video_stream_receiver_.reset(new RtpVideoStreamReceiver(
&mock_transport_, nullptr, &packet_router_, &config_,
- rtp_receive_statistics_.get(), nullptr, process_thread_.get(),
- &mock_nack_sender_,
+ nullptr, process_thread_.get(), &mock_nack_sender_,
&mock_key_frame_request_sender_, &mock_on_complete_frame_callback_,
- &timing_);
+ &timing_));
}
WebRtcRTPHeader GetDefaultPacket() {
@@ -199,7 +196,6 @@
PacketRouter packet_router_;
VCMTiming timing_;
std::unique_ptr<ProcessThread> process_thread_;
- std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
std::unique_ptr<RtpVideoStreamReceiver> rtp_video_stream_receiver_;
};
diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc
index a89e3ce..74c9013 100644
--- a/video/video_receive_stream.cc
+++ b/video/video_receive_stream.cc
@@ -18,7 +18,6 @@
#include "webrtc/api/optional.h"
#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/checks.h"
#include "webrtc/rtc_base/location.h"
#include "webrtc/rtc_base/logging.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"
@@ -90,7 +88,6 @@
"DecodingThread",
rtc::kHighestPriority),
call_stats_(call_stats),
- rtp_receive_statistics_(ReceiveStatistics::Create(clock_)),
timing_(new VCMTiming(clock_)),
video_receiver_(clock_, nullptr, this, timing_.get(), this, this),
stats_proxy_(&config_, clock_),
@@ -98,7 +95,6 @@
call_stats_->rtcp_rtt_stats(),
packet_router,
&config_,
- rtp_receive_statistics_.get(),
&stats_proxy_,
process_thread_,
this, // NackSender
@@ -124,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(
@@ -135,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, rtp_receive_statistics_.get());
+ 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 216ef12..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;
@@ -126,10 +125,6 @@
CallStats* const call_stats_;
- // Shared by media and rtx stream receivers, since the latter has no RtpRtcp
- // module of its own.
- const std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
-
std::unique_ptr<VCMTiming> timing_; // Jitter buffer experiment.
vcm::VideoReceiver video_receiver_;
std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
@@ -146,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
