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-Original-Commit-Position: refs/heads/master@{#19649}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 5c0f6c62ea3b1d2c43f8fc152961af27033475f7
diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc
index ed44dad..2b85452 100644
--- a/call/rampup_tests.cc
+++ b/call/rampup_tests.cc
@@ -207,8 +207,9 @@
recv_config.rtp.ulpfec.ulpfec_payload_type =
send_config->rtp.ulpfec.ulpfec_payload_type;
if (rtx_) {
- recv_config.rtp.ulpfec.red_rtx_payload_type =
- send_config->rtp.ulpfec.red_rtx_payload_type;
+ recv_config.rtp.rtx_associated_payload_types
+ [send_config->rtp.ulpfec.red_rtx_payload_type] =
+ send_config->rtp.ulpfec.red_payload_type;
}
}
diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h
index 8ed93a1..e78a369 100644
--- a/call/video_receive_stream.h
+++ b/call/video_receive_stream.h
@@ -161,6 +161,11 @@
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 00d138a..4fabe52 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -2188,6 +2188,11 @@
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 c6adfd3..1306082 100644
--- a/media/engine/webrtcvideoengine_unittest.cc
+++ b/media/engine/webrtcvideoengine_unittest.cc
@@ -83,6 +83,32 @@
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) {
@@ -112,15 +138,6 @@
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 {
@@ -1316,9 +1333,12 @@
cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
EXPECT_FALSE(
recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
- EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
- recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
+ 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";
+
EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
}
@@ -1329,6 +1349,12 @@
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) {
@@ -3796,9 +3822,11 @@
recv_stream = fake_call_->GetVideoReceiveStreams()[0];
EXPECT_FALSE(
recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
- EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
- recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
+ 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 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 f3bba0e..29f5097 100644
--- a/video/BUILD.gn
+++ b/video/BUILD.gn
@@ -60,6 +60,9 @@
"../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 4744049..8ace93e 100644
--- a/video/end_to_end_tests.cc
+++ b/video/end_to_end_tests.cc
@@ -1184,7 +1184,9 @@
send_config->rtp.rtx.payload_type = kSendRtxPayloadType;
(*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0];
(*receive_configs)[0]
- .rtp.rtx_associated_payload_types[kSendRtxPayloadType] =
+ .rtp.rtx_associated_payload_types[(payload_type_ == kRedPayloadType)
+ ? kRtxRedPayloadType
+ : 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 0dbb2f7..51171d1 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -105,7 +105,6 @@
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,
@@ -146,12 +145,8 @@
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()) {
@@ -168,11 +163,6 @@
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)
@@ -495,31 +485,7 @@
}
ulpfec_receiver_->ProcessReceivedFec();
} else if (rtp_payload_registry_.IsRtx(header)) {
- 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;
+ LOG(LS_WARNING) << "Unexpected RTX packet on media ssrc";
}
}
diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h
index 15af2e6..86e2de7 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -185,8 +185,6 @@
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 ae14634..5af855b 100644
--- a/video/video_receive_stream.cc
+++ b/video/video_receive_stream.cc
@@ -17,6 +17,7 @@
#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"
@@ -32,6 +33,7 @@
#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"
@@ -120,7 +122,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(
@@ -131,9 +133,12 @@
// Register with RtpStreamReceiverController.
media_receiver_ = receiver_controller->CreateReceiver(
config_.rtp.remote_ssrc, &rtp_video_stream_receiver_);
- if (config.rtp.rtx_ssrc) {
+ 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);
rtx_receiver_ = receiver_controller->CreateReceiver(
- config_.rtp.rtx_ssrc, &rtp_video_stream_receiver_);
+ config_.rtp.rtx_ssrc, rtx_receive_stream_.get());
}
}
diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h
index ba340ea..967efcc 100644
--- a/video/video_receive_stream.h
+++ b/video/video_receive_stream.h
@@ -38,6 +38,7 @@
class RTPFragmentationHeader;
class RtpStreamReceiverInterface;
class RtpStreamReceiverControllerInterface;
+class RtxReceiveStream;
class VCMTiming;
class VCMJitterEstimator;
@@ -143,6 +144,7 @@
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
