rtx-time implementation
provides an implementation of the rtx-time parameter from
https://tools.ietf.org/html/rfc4588#section-8
that determines the maximum time a receiver waits for a frame
before sending a PLI.
BUG=webrtc:12420
Change-Id: Iff20d92c806989cd4d56fe330d105b3dd127ed24
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/201400
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33627}
diff --git a/media/base/media_constants.cc b/media/base/media_constants.cc
index d16196a..2ac3825 100644
--- a/media/base/media_constants.cc
+++ b/media/base/media_constants.cc
@@ -24,7 +24,6 @@
const float kLowSystemCpuThreshold = 0.65f;
const float kProcessCpuThreshold = 0.10f;
-const char kRtxCodecName[] = "rtx";
const char kRedCodecName[] = "red";
const char kUlpfecCodecName[] = "ulpfec";
const char kMultiplexCodecName[] = "multiplex";
@@ -36,7 +35,11 @@
// draft-ietf-payload-flexible-fec-scheme-02.txt
const char kFlexfecFmtpRepairWindow[] = "repair-window";
+// RFC 4588 RTP Retransmission Payload Format
+const char kRtxCodecName[] = "rtx";
+const char kCodecParamRtxTime[] = "rtx-time";
const char kCodecParamAssociatedPayloadType[] = "apt";
+
const char kCodecParamAssociatedCodecName[] = "acn";
const char kOpusCodecName[] = "opus";
diff --git a/media/base/media_constants.h b/media/base/media_constants.h
index 90fb424..16b97ca 100644
--- a/media/base/media_constants.h
+++ b/media/base/media_constants.h
@@ -32,7 +32,6 @@
extern const float kLowSystemCpuThreshold;
extern const float kProcessCpuThreshold;
-extern const char kRtxCodecName[];
extern const char kRedCodecName[];
extern const char kUlpfecCodecName[];
extern const char kFlexfecCodecName[];
@@ -40,8 +39,10 @@
extern const char kFlexfecFmtpRepairWindow[];
-// Codec parameters
+extern const char kRtxCodecName[];
+extern const char kCodecParamRtxTime[];
extern const char kCodecParamAssociatedPayloadType[];
+
extern const char kCodecParamAssociatedCodecName[];
extern const char kOpusCodecName[];
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index c260de4..057fdf6 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -1038,7 +1038,7 @@
if (changed_params.send_codec || changed_params.rtcp_mode) {
// Update receive feedback parameters from new codec or RTCP mode.
RTC_LOG(LS_INFO)
- << "SetFeedbackOptions on all the receive streams because the send "
+ << "SetFeedbackParameters on all the receive streams because the send "
"codec or RTCP mode has changed.";
for (auto& kv : receive_streams_) {
RTC_DCHECK(kv.second != nullptr);
@@ -1046,7 +1046,8 @@
HasLntf(send_codec_->codec), HasNack(send_codec_->codec),
HasTransportCc(send_codec_->codec),
send_params_.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize
- : webrtc::RtcpMode::kCompound);
+ : webrtc::RtcpMode::kCompound,
+ send_codec_->rtx_time);
}
}
return true;
@@ -1523,6 +1524,12 @@
? webrtc::RtcpMode::kReducedSize
: webrtc::RtcpMode::kCompound;
+ // rtx-time (RFC 4588) is a declarative attribute similar to rtcp-rsize and
+ // determined by the sender / send codec.
+ if (send_codec_ && send_codec_->rtx_time != -1) {
+ config->rtp.nack.rtp_history_ms = send_codec_->rtx_time;
+ }
+
config->rtp.transport_cc =
send_codec_ ? HasTransportCc(send_codec_->codec) : false;
@@ -2864,6 +2871,11 @@
config_.rtp.lntf.enabled = HasLntf(codec.codec);
config_.rtp.nack.rtp_history_ms = HasNack(codec.codec) ? kNackHistoryMs : 0;
+ // The rtx-time parameter can be used to override the hardcoded default for
+ // the NACK buffer length.
+ if (codec.rtx_time != -1 && config_.rtp.nack.rtp_history_ms != 0) {
+ config_.rtp.nack.rtp_history_ms = codec.rtx_time;
+ }
config_.rtp.rtcp_xr.receiver_reference_time_report = HasRrtr(codec.codec);
if (codec.ulpfec.red_rtx_payload_type != -1) {
config_.rtp
@@ -2897,8 +2909,10 @@
bool lntf_enabled,
bool nack_enabled,
bool transport_cc_enabled,
- webrtc::RtcpMode rtcp_mode) {
- int nack_history_ms = nack_enabled ? kNackHistoryMs : 0;
+ webrtc::RtcpMode rtcp_mode,
+ int rtx_time) {
+ int nack_history_ms =
+ nack_enabled ? rtx_time != -1 ? rtx_time : kNackHistoryMs : 0;
if (config_.rtp.lntf.enabled == lntf_enabled &&
config_.rtp.nack.rtp_history_ms == nack_history_ms &&
config_.rtp.transport_cc == transport_cc_enabled &&
@@ -2907,7 +2921,8 @@
<< "Ignoring call to SetFeedbackParameters because parameters are "
"unchanged; lntf="
<< lntf_enabled << ", nack=" << nack_enabled
- << ", transport_cc=" << transport_cc_enabled;
+ << ", transport_cc=" << transport_cc_enabled
+ << ", rtx_time=" << rtx_time;
return;
}
config_.rtp.lntf.enabled = lntf_enabled;
@@ -3177,20 +3192,21 @@
}
WebRtcVideoChannel::VideoCodecSettings::VideoCodecSettings()
- : flexfec_payload_type(-1), rtx_payload_type(-1) {}
+ : flexfec_payload_type(-1), rtx_payload_type(-1), rtx_time(-1) {}
bool WebRtcVideoChannel::VideoCodecSettings::operator==(
const WebRtcVideoChannel::VideoCodecSettings& other) const {
return codec == other.codec && ulpfec == other.ulpfec &&
flexfec_payload_type == other.flexfec_payload_type &&
- rtx_payload_type == other.rtx_payload_type;
+ rtx_payload_type == other.rtx_payload_type &&
+ rtx_time == other.rtx_time;
}
bool WebRtcVideoChannel::VideoCodecSettings::EqualsDisregardingFlexfec(
const WebRtcVideoChannel::VideoCodecSettings& a,
const WebRtcVideoChannel::VideoCodecSettings& b) {
return a.codec == b.codec && a.ulpfec == b.ulpfec &&
- a.rtx_payload_type == b.rtx_payload_type;
+ a.rtx_payload_type == b.rtx_payload_type && a.rtx_time == b.rtx_time;
}
bool WebRtcVideoChannel::VideoCodecSettings::operator!=(
@@ -3208,6 +3224,7 @@
std::map<int, VideoCodec::CodecType> payload_codec_type;
// |rtx_mapping| maps video payload type to rtx payload type.
std::map<int, int> rtx_mapping;
+ std::map<int, int> rtx_time_mapping;
webrtc::UlpfecConfig ulpfec_config;
absl::optional<int> flexfec_payload_type;
@@ -3269,6 +3286,10 @@
<< in_codec.ToString();
return {};
}
+ int rtx_time;
+ if (in_codec.GetParam(kCodecParamRtxTime, &rtx_time) && rtx_time > 0) {
+ rtx_time_mapping[associated_payload_type] = rtx_time;
+ }
rtx_mapping[associated_payload_type] = payload_type;
break;
}
@@ -3318,6 +3339,16 @@
if (it != rtx_mapping.end()) {
const int rtx_payload_type = it->second;
codec_settings.rtx_payload_type = rtx_payload_type;
+
+ auto rtx_time_it = rtx_time_mapping.find(payload_type);
+ if (rtx_time_it != rtx_time_mapping.end()) {
+ const int rtx_time = rtx_time_it->second;
+ if (rtx_time < kNackHistoryMs) {
+ codec_settings.rtx_time = rtx_time;
+ } else {
+ codec_settings.rtx_time = kNackHistoryMs;
+ }
+ }
}
}
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h
index f278897..fe3ad6d 100644
--- a/media/engine/webrtc_video_engine.h
+++ b/media/engine/webrtc_video_engine.h
@@ -273,6 +273,7 @@
webrtc::UlpfecConfig ulpfec;
int flexfec_payload_type; // -1 if absent.
int rtx_payload_type; // -1 if absent.
+ int rtx_time; // -1 if absent.
};
struct ChangedSendParameters {
@@ -455,7 +456,8 @@
void SetFeedbackParameters(bool lntf_enabled,
bool nack_enabled,
bool transport_cc_enabled,
- webrtc::RtcpMode rtcp_mode);
+ webrtc::RtcpMode rtcp_mode,
+ int rtx_time);
void SetRecvParameters(const ChangedRecvParameters& recv_params);
void OnFrame(const webrtc::VideoFrame& frame) override;
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index a4176ba..fc945dd 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -4934,6 +4934,76 @@
EXPECT_EQ(kRtxSsrcs1[0], config_after.rtp.rtx_ssrc);
}
+TEST_F(WebRtcVideoChannelTest, SetRecvCodecsRtxWithRtxTime) {
+ const int kUnusedPayloadType1 = 126;
+ const int kUnusedPayloadType2 = 127;
+ EXPECT_FALSE(FindCodecById(engine_.recv_codecs(), kUnusedPayloadType1));
+ EXPECT_FALSE(FindCodecById(engine_.recv_codecs(), kUnusedPayloadType2));
+
+ // SSRCs for RTX.
+ cricket::StreamParams params =
+ cricket::StreamParams::CreateLegacy(kSsrcs1[0]);
+ params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]);
+ AddRecvStream(params);
+
+ // Payload type for RTX.
+ cricket::VideoRecvParameters parameters;
+ parameters.codecs.push_back(GetEngineCodec("VP8"));
+ cricket::VideoCodec rtx_codec(kUnusedPayloadType1, "rtx");
+ rtx_codec.SetParam("apt", GetEngineCodec("VP8").id);
+ parameters.codecs.push_back(rtx_codec);
+ EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+ ASSERT_EQ(1U, fake_call_->GetVideoReceiveStreams().size());
+ const webrtc::VideoReceiveStream::Config& config =
+ fake_call_->GetVideoReceiveStreams()[0]->GetConfig();
+
+ const int kRtxTime = 343;
+ // Assert that the default value is different from the ones we test
+ // and store the default value.
+ EXPECT_NE(config.rtp.nack.rtp_history_ms, kRtxTime);
+ int default_history_ms = config.rtp.nack.rtp_history_ms;
+
+ // Set rtx-time.
+ parameters.codecs[1].SetParam(kCodecParamRtxTime, kRtxTime);
+ EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+ EXPECT_EQ(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ kRtxTime);
+
+ // Negative values are ignored so the default value applies.
+ parameters.codecs[1].SetParam(kCodecParamRtxTime, -1);
+ EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+ EXPECT_NE(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ -1);
+ EXPECT_EQ(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ default_history_ms);
+
+ // 0 is ignored so the default applies.
+ parameters.codecs[1].SetParam(kCodecParamRtxTime, 0);
+ EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+ EXPECT_NE(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ 0);
+ EXPECT_EQ(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ default_history_ms);
+
+ // Values larger than the default are clamped to the default.
+ parameters.codecs[1].SetParam(kCodecParamRtxTime, default_history_ms + 100);
+ EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+ EXPECT_EQ(fake_call_->GetVideoReceiveStreams()[0]
+ ->GetConfig()
+ .rtp.nack.rtp_history_ms,
+ default_history_ms);
+}
+
TEST_F(WebRtcVideoChannelTest, SetRecvCodecsDifferentPayloadType) {
cricket::VideoRecvParameters parameters;
parameters.codecs.push_back(GetEngineCodec("VP8"));
diff --git a/pc/media_session.cc b/pc/media_session.cc
index caf2458..6374f17 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -793,6 +793,12 @@
// FindMatchingCodec shouldn't return something with no apt value.
RTC_DCHECK(apt_it != theirs.params.end());
negotiated.SetParam(kCodecParamAssociatedPayloadType, apt_it->second);
+
+ // We support parsing the declarative rtx-time parameter.
+ const auto rtx_time_it = theirs.params.find(kCodecParamRtxTime);
+ if (rtx_time_it != theirs.params.end()) {
+ negotiated.SetParam(kCodecParamRtxTime, rtx_time_it->second);
+ }
}
if (absl::EqualsIgnoreCase(ours.name, kH264CodecName)) {
webrtc::H264::GenerateProfileLevelIdForAnswer(
diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc
index 8df76d5..6664820 100644
--- a/video/rtp_video_stream_receiver2.cc
+++ b/video/rtp_video_stream_receiver2.cc
@@ -114,6 +114,7 @@
if (config.rtp.nack.rtp_history_ms == 0)
return nullptr;
+ // TODO(bugs.webrtc.org/12420): pass rtp_history_ms to the nack module.
return std::make_unique<NackModule2>(current_queue, clock, nack_sender,
keyframe_request_sender);
}
diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc
index 073fd93..2e35302 100644
--- a/video/video_receive_stream2.cc
+++ b/video/video_receive_stream2.cc
@@ -186,6 +186,23 @@
} // namespace
+int DetermineMaxWaitForFrame(const VideoReceiveStream::Config& config,
+ bool is_keyframe) {
+ // A (arbitrary) conversion factor between the remotely signalled NACK buffer
+ // time (if not present defaults to 1000ms) and the maximum time we wait for a
+ // remote frame. Chosen to not change existing defaults when using not
+ // rtx-time.
+ const int conversion_factor = 3;
+
+ if (config.rtp.nack.rtp_history_ms > 0 &&
+ conversion_factor * config.rtp.nack.rtp_history_ms < kMaxWaitForFrameMs) {
+ return is_keyframe ? config.rtp.nack.rtp_history_ms
+ : conversion_factor * config.rtp.nack.rtp_history_ms;
+ }
+ return is_keyframe ? VideoReceiveStream2::kMaxWaitForKeyFrameMs
+ : kMaxWaitForFrameMs;
+}
+
VideoReceiveStream2::VideoReceiveStream2(
TaskQueueFactory* task_queue_factory,
TaskQueueBase* current_queue,
@@ -225,8 +242,8 @@
config_.frame_decryptor,
config_.frame_transformer),
rtp_stream_sync_(current_queue, this),
- max_wait_for_keyframe_ms_(kMaxWaitForKeyFrameMs),
- max_wait_for_frame_ms_(kMaxWaitForFrameMs),
+ max_wait_for_keyframe_ms_(DetermineMaxWaitForFrame(config, true)),
+ max_wait_for_frame_ms_(DetermineMaxWaitForFrame(config, false)),
low_latency_renderer_enabled_("enabled", true),
low_latency_renderer_include_predecode_buffer_("include_predecode_buffer",
true),
diff --git a/video/video_receive_stream2_unittest.cc b/video/video_receive_stream2_unittest.cc
index aa9cee3..f0916c1 100644
--- a/video/video_receive_stream2_unittest.cc
+++ b/video/video_receive_stream2_unittest.cc
@@ -482,7 +482,8 @@
video_receive_stream_->Stop();
}
-class VideoReceiveStream2TestWithSimulatedClock : public ::testing::Test {
+class VideoReceiveStream2TestWithSimulatedClock
+ : public ::testing::TestWithParam<int> {
public:
class FakeDecoder2 : public test::FakeDecoder {
public:
@@ -509,6 +510,7 @@
VideoReceiveStream::Config config(transport);
config.rtp.remote_ssrc = 1111;
config.rtp.local_ssrc = 2222;
+ config.rtp.nack.rtp_history_ms = GetParam(); // rtx-time.
config.renderer = renderer;
config.decoder_factory = decoder_factory;
VideoReceiveStream::Decoder fake_decoder;
@@ -566,10 +568,9 @@
std::unique_ptr<rtc::Event> event_;
};
-TEST_F(VideoReceiveStream2TestWithSimulatedClock,
+TEST_P(VideoReceiveStream2TestWithSimulatedClock,
RequestsKeyFramesUntilKeyFrameReceived) {
- auto tick = TimeDelta::Millis(
- internal::VideoReceiveStream2::kMaxWaitForKeyFrameMs / 2);
+ auto tick = TimeDelta::Millis(GetParam() / 2);
EXPECT_CALL(mock_transport_, SendRtcp).Times(1).WillOnce(Invoke([this]() {
loop_.Quit();
return 0;
@@ -581,7 +582,8 @@
loop_.Run();
testing::Mock::VerifyAndClearExpectations(&mock_transport_);
- // T+200ms: still no key frame received, expect key frame request sent again.
+ // T+keyframetimeout: still no key frame received, expect key frame request
+ // sent again.
EXPECT_CALL(mock_transport_, SendRtcp).Times(1).WillOnce(Invoke([this]() {
loop_.Quit();
return 0;
@@ -591,8 +593,8 @@
loop_.Run();
testing::Mock::VerifyAndClearExpectations(&mock_transport_);
- // T+200ms: now send a key frame - we should not observe new key frame
- // requests after this.
+ // T+keyframetimeout: now send a key frame - we should not observe new key
+ // frame requests after this.
EXPECT_CALL(mock_transport_, SendRtcp).Times(0);
PassEncodedFrameAndWait(MakeFrame(VideoFrameType::kVideoFrameKey, 3));
time_controller_.AdvanceTime(2 * tick);
@@ -601,6 +603,12 @@
loop_.Run();
}
+INSTANTIATE_TEST_SUITE_P(
+ RtxTime,
+ VideoReceiveStream2TestWithSimulatedClock,
+ ::testing::Values(internal::VideoReceiveStream2::kMaxWaitForKeyFrameMs,
+ 50 /*ms*/));
+
class VideoReceiveStream2TestWithLazyDecoderCreation : public ::testing::Test {
public:
VideoReceiveStream2TestWithLazyDecoderCreation()
