From fe2bef39cd2a5c891a49f7320514fb04324dc66c Mon Sep 17 00:00:00 2001 From: brandtr Date: Thu, 26 Jan 2017 08:03:58 -0800 Subject: [PATCH] Make RTX pt/apt reconfigurable by calling WebRtcVideoChannel2::SetRecvParameters. Prior to this CL, received RTX (associated) payload types were only configured when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX SSRC was set up. After this CL, the RTX (associated) payload types are set in WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since that is the code path that sets other SSRCs. As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed. We remove the possibility for each video payload type to have an associated specific RTX SSRC. Although the config previously allowed for this, all payload types always had the same RTX SSRC set, and the underlying RtpPayloadRegistry did not support multiple SSRCs. This change to the config struct should thus not have any functional impact. The change does however affect the RtcEventLog, since that is used for storing the VideoReceiveStream::Configs. For simplicity, this CL does not change the event log proto definitions, instead duplicating the serialized RTX SSRCs such that they fit in the existing proto definition. BUG=webrtc:7011 Review-Url: https://codereview.webrtc.org/2646073004 Cr-Commit-Position: refs/heads/master@{#16302} --- webrtc/call/call.cc | 7 +- webrtc/call/rampup_tests.cc | 8 +- webrtc/logging/rtc_event_log/rtc_event_log.cc | 6 +- .../rtc_event_log/rtc_event_log_parser.cc | 25 ++++- .../rtc_event_log/rtc_event_log_unittest.cc | 9 +- .../rtc_event_log_unittest_helper.cc | 22 ++-- webrtc/media/engine/webrtcvideoengine2.cc | 18 ++- .../engine/webrtcvideoengine2_unittest.cc | 105 +++++++++++++++--- webrtc/tools/event_log_visualizer/analyzer.cc | 12 +- webrtc/video/end_to_end_tests.cc | 18 +-- webrtc/video/receive_statistics_proxy.cc | 7 +- webrtc/video/rtp_stream_receiver.cc | 13 +-- webrtc/video/video_quality_test.cc | 4 +- webrtc/video/video_receive_stream.cc | 10 +- webrtc/video_receive_stream.h | 18 +-- 15 files changed, 178 insertions(+), 104 deletions(-) diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index d37d08ef8e..ac9866b314 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -649,11 +649,8 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream( RTC_DCHECK(video_receive_ssrcs_.find(config.rtp.remote_ssrc) == video_receive_ssrcs_.end()); video_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream; - // TODO(pbos): Configure different RTX payloads per receive payload. - VideoReceiveStream::Config::Rtp::RtxMap::const_iterator it = - config.rtp.rtx.begin(); - if (it != config.rtp.rtx.end()) - video_receive_ssrcs_[it->second.ssrc] = receive_stream; + if (config.rtp.rtx_ssrc) + video_receive_ssrcs_[config.rtp.rtx_ssrc] = receive_stream; video_receive_streams_.insert(receive_stream); ConfigureSync(config.sync_group); } diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc index f637f77577..d8f0592ec4 100644 --- a/webrtc/call/rampup_tests.cc +++ b/webrtc/call/rampup_tests.cc @@ -199,10 +199,10 @@ void RampUpTester::ModifyVideoConfigs( } if (rtx_) { - recv_config.rtp.rtx[send_config->encoder_settings.payload_type].ssrc = - video_rtx_ssrcs_[i]; - recv_config.rtp.rtx[send_config->encoder_settings.payload_type] - .payload_type = send_config->rtp.rtx.payload_type; + recv_config.rtp.rtx_ssrc = video_rtx_ssrcs_[i]; + recv_config.rtp + .rtx_payload_types[send_config->encoder_settings.payload_type] = + send_config->rtp.rtx.payload_type; } ++i; } diff --git a/webrtc/logging/rtc_event_log/rtc_event_log.cc b/webrtc/logging/rtc_event_log/rtc_event_log.cc index 65ee7d8881..b545d6453d 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log.cc +++ b/webrtc/logging/rtc_event_log/rtc_event_log.cc @@ -242,11 +242,11 @@ void RtcEventLogImpl::LogVideoReceiveStreamConfig( receiver_config->set_rtcp_mode(ConvertRtcpMode(config.rtp.rtcp_mode)); receiver_config->set_remb(config.rtp.remb); - for (const auto& kv : config.rtp.rtx) { + for (const auto& kv : config.rtp.rtx_payload_types) { rtclog::RtxMap* rtx = receiver_config->add_rtx_map(); rtx->set_payload_type(kv.first); - rtx->mutable_config()->set_rtx_ssrc(kv.second.ssrc); - rtx->mutable_config()->set_rtx_payload_type(kv.second.payload_type); + rtx->mutable_config()->set_rtx_ssrc(config.rtp.rtx_ssrc); + rtx->mutable_config()->set_rtx_payload_type(kv.second); } for (const auto& e : config.rtp.extensions) { diff --git a/webrtc/logging/rtc_event_log/rtc_event_log_parser.cc b/webrtc/logging/rtc_event_log/rtc_event_log_parser.cc index 3b808b2e43..c1bf94458d 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/webrtc/logging/rtc_event_log/rtc_event_log_parser.cc @@ -10,8 +10,10 @@ #include "webrtc/logging/rtc_event_log/rtc_event_log_parser.h" +#include #include +#include #include #include #include @@ -314,17 +316,30 @@ void ParsedRtcEventLog::GetVideoReceiveConfig( RTC_CHECK(receiver_config.has_remb()); config->rtp.remb = receiver_config.remb(); // Get RTX map. - config->rtp.rtx.clear(); + std::vector rtx_ssrcs(receiver_config.rtx_map_size()); + config->rtp.rtx_payload_types.clear(); for (int i = 0; i < receiver_config.rtx_map_size(); i++) { const rtclog::RtxMap& map = receiver_config.rtx_map(i); RTC_CHECK(map.has_payload_type()); RTC_CHECK(map.has_config()); RTC_CHECK(map.config().has_rtx_ssrc()); + rtx_ssrcs[i] = map.config().rtx_ssrc(); RTC_CHECK(map.config().has_rtx_payload_type()); - webrtc::VideoReceiveStream::Config::Rtp::Rtx rtx_pair; - rtx_pair.ssrc = map.config().rtx_ssrc(); - rtx_pair.payload_type = map.config().rtx_payload_type(); - config->rtp.rtx.insert(std::make_pair(map.payload_type(), rtx_pair)); + config->rtp.rtx_payload_types.insert( + std::make_pair(map.payload_type(), map.config().rtx_payload_type())); + } + if (!rtx_ssrcs.empty()) { + config->rtp.rtx_ssrc = rtx_ssrcs[0]; + + auto pred = [&config](uint32_t ssrc) { + return ssrc == config->rtp.rtx_ssrc; + }; + if (!std::all_of(rtx_ssrcs.cbegin(), rtx_ssrcs.cend(), pred)) { + LOG(LS_WARNING) << "RtcEventLog protobuf contained different SSRCs for " + "different received RTX payload types. Will only use " + "rtx_ssrc = " + << config->rtp.rtx_ssrc << "."; + } } // Get header extensions. GetHeaderExtensions(&config->rtp.extensions, diff --git a/webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc b/webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc index f0ee973a13..97c82b3609 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc +++ b/webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc @@ -165,11 +165,10 @@ void GenerateVideoReceiveConfig(uint32_t extensions_bitvector, config->rtp.rtcp_mode = prng->Rand() ? RtcpMode::kCompound : RtcpMode::kReducedSize; config->rtp.remb = prng->Rand(); - // Add a map from a payload type to a new ssrc and a new payload type for RTX. - VideoReceiveStream::Config::Rtp::Rtx rtx_pair; - rtx_pair.ssrc = prng->Rand(); - rtx_pair.payload_type = prng->Rand(0, 127); - config->rtp.rtx.insert(std::make_pair(prng->Rand(0, 127), rtx_pair)); + config->rtp.rtx_ssrc = prng->Rand(); + // Add a map from a payload type to a new payload type for RTX. + config->rtp.rtx_payload_types.insert( + std::make_pair(prng->Rand(0, 127), prng->Rand(0, 127))); // Add header extensions. for (unsigned i = 0; i < kNumExtensions; i++) { if (extensions_bitvector & (1u << i)) { diff --git a/webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc index 19ca8aa15c..5bd8e4d060 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc +++ b/webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc @@ -127,19 +127,18 @@ void RtcEventLogTestHelper::VerifyVideoReceiveStreamConfig( ASSERT_TRUE(receiver_config.has_remb()); EXPECT_EQ(config.rtp.remb, receiver_config.remb()); // Check RTX map. - ASSERT_EQ(static_cast(config.rtp.rtx.size()), + ASSERT_EQ(static_cast(config.rtp.rtx_payload_types.size()), receiver_config.rtx_map_size()); for (const rtclog::RtxMap& rtx_map : receiver_config.rtx_map()) { ASSERT_TRUE(rtx_map.has_payload_type()); ASSERT_TRUE(rtx_map.has_config()); - EXPECT_EQ(1u, config.rtp.rtx.count(rtx_map.payload_type())); + EXPECT_EQ(1u, config.rtp.rtx_payload_types.count(rtx_map.payload_type())); const rtclog::RtxConfig& rtx_config = rtx_map.config(); - const VideoReceiveStream::Config::Rtp::Rtx& rtx = - config.rtp.rtx.at(rtx_map.payload_type()); ASSERT_TRUE(rtx_config.has_rtx_ssrc()); ASSERT_TRUE(rtx_config.has_rtx_payload_type()); - EXPECT_EQ(rtx.ssrc, rtx_config.rtx_ssrc()); - EXPECT_EQ(rtx.payload_type, rtx_config.rtx_payload_type()); + EXPECT_EQ(config.rtp.rtx_ssrc, rtx_config.rtx_ssrc()); + EXPECT_EQ(config.rtp.rtx_payload_types.at(rtx_map.payload_type()), + rtx_config.rtx_payload_type()); } // Check header extensions. ASSERT_EQ(static_cast(config.rtp.extensions.size()), @@ -173,12 +172,13 @@ void RtcEventLogTestHelper::VerifyVideoReceiveStreamConfig( EXPECT_EQ(config.rtp.rtcp_mode, parsed_config.rtp.rtcp_mode); EXPECT_EQ(config.rtp.remb, parsed_config.rtp.remb); // Check RTX map. - EXPECT_EQ(config.rtp.rtx.size(), parsed_config.rtp.rtx.size()); - for (const auto& kv : config.rtp.rtx) { - auto parsed_kv = parsed_config.rtp.rtx.find(kv.first); + EXPECT_EQ(config.rtp.rtx_ssrc, parsed_config.rtp.rtx_ssrc); + EXPECT_EQ(config.rtp.rtx_payload_types.size(), + parsed_config.rtp.rtx_payload_types.size()); + for (const auto& kv : config.rtp.rtx_payload_types) { + auto parsed_kv = parsed_config.rtp.rtx_payload_types.find(kv.first); EXPECT_EQ(kv.first, parsed_kv->first); - EXPECT_EQ(kv.second.ssrc, parsed_kv->second.ssrc); - EXPECT_EQ(kv.second.payload_type, parsed_kv->second.payload_type); + EXPECT_EQ(kv.second, parsed_kv->second); } // Check header extensions. EXPECT_EQ(config.rtp.extensions.size(), parsed_config.rtp.extensions.size()); diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 0cd31937cc..40f486dc0f 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1233,16 +1233,7 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( flexfec_config->rtp_header_extensions = config->rtp.extensions; } - for (size_t i = 0; i < recv_codecs_.size(); ++i) { - uint32_t rtx_ssrc; - if (recv_codecs_[i].rtx_payload_type != -1 && - sp.GetFidSsrc(ssrc, &rtx_ssrc)) { - webrtc::VideoReceiveStream::Config::Rtp::Rtx& rtx = - config->rtp.rtx[recv_codecs_[i].codec.id]; - rtx.ssrc = rtx_ssrc; - rtx.payload_type = recv_codecs_[i].rtx_payload_type; - } - } + sp.GetFidSsrc(ssrc, &config->rtp.rtx_ssrc); config->rtp.extensions = recv_rtp_extensions_; } @@ -2205,7 +2196,12 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::ConfigureCodecs( config_.decoders.push_back(decoder); } - // TODO(pbos): Reconfigure RTX based on incoming recv_codecs. + config_.rtp.rtx_payload_types.clear(); + for (const VideoCodecSettings& recv_codec : recv_codecs) { + config_.rtp.rtx_payload_types[recv_codec.codec.id] = + recv_codec.rtx_payload_type; + } + config_.rtp.ulpfec = recv_codecs.front().ulpfec; flexfec_config_.payload_type = recv_codecs.front().flexfec_payload_type; diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index a93d2d98c2..ef77e9e927 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -1228,13 +1228,11 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithSimAndRtx) { // Receiver side. FakeVideoReceiveStream* recv_stream = AddRecvStream( cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs)); - EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx.empty()); + EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx_payload_types.empty()); EXPECT_EQ(recv_stream->GetConfig().decoders.size(), - recv_stream->GetConfig().rtp.rtx.size()) + recv_stream->GetConfig().rtp.rtx_payload_types.size()) << "RTX should be mapped for all decoders/payload types."; - for (const auto& kv : recv_stream->GetConfig().rtp.rtx) { - EXPECT_EQ(rtx_ssrcs[0], kv.second.ssrc); - } + EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc); } TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) { @@ -1243,8 +1241,7 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) { cricket::StreamParams::CreateLegacy(kSsrcs1[0]); params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]); FakeVideoReceiveStream* recv_stream = AddRecvStream(params); - EXPECT_EQ(kRtxSsrcs1[0], - recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc); + EXPECT_EQ(kRtxSsrcs1[0], recv_stream->GetConfig().rtp.rtx_ssrc); } TEST_F(WebRtcVideoChannel2Test, RecvStreamNoRtx) { @@ -1252,7 +1249,7 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamNoRtx) { cricket::StreamParams params = cricket::StreamParams::CreateLegacy(kSsrcs1[0]); FakeVideoReceiveStream* recv_stream = AddRecvStream(params); - ASSERT_TRUE(recv_stream->GetConfig().rtp.rtx.empty()); + ASSERT_EQ(0U, recv_stream->GetConfig().rtp.rtx_ssrc); } TEST_F(WebRtcVideoChannel2Test, NoHeaderExtesionsByDefault) { @@ -2482,6 +2479,43 @@ TEST_F(WebRtcVideoChannel2Test, } } +TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithChangedRtxPayloadType) { + const int kUnusedPayloadType1 = 126; + const int kUnusedPayloadType2 = 127; + EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType1)); + EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType2)); + + // SSRCs for RTX. + cricket::StreamParams params = + cricket::StreamParams::CreateLegacy(kSsrcs1[0]); + params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]); + AddSendStream(params); + + // Original payload type for RTX. + cricket::VideoSendParameters 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_->SetSendParameters(parameters)); + ASSERT_EQ(1U, fake_call_->GetVideoSendStreams().size()); + const webrtc::VideoSendStream::Config& config_before = + fake_call_->GetVideoSendStreams()[0]->GetConfig(); + EXPECT_EQ(kUnusedPayloadType1, config_before.rtp.rtx.payload_type); + ASSERT_EQ(1U, config_before.rtp.rtx.ssrcs.size()); + EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx.ssrcs[0]); + + // Change payload type for RTX. + parameters.codecs[1].id = kUnusedPayloadType2; + EXPECT_TRUE(channel_->SetSendParameters(parameters)); + ASSERT_EQ(1U, fake_call_->GetVideoSendStreams().size()); + const webrtc::VideoSendStream::Config& config_after = + fake_call_->GetVideoSendStreams()[0]->GetConfig(); + EXPECT_EQ(kUnusedPayloadType2, config_after.rtp.rtx.payload_type); + ASSERT_EQ(1U, config_after.rtp.rtx.ssrcs.size()); + EXPECT_EQ(kRtxSsrcs1[0], config_after.rtp.rtx.ssrcs[0]); +} + TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFecDisablesFec) { cricket::VideoSendParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); @@ -2812,6 +2846,49 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithRtx) { "rejected."; } +TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithChangedRtxPayloadType) { + const int kUnusedPayloadType1 = 126; + const int kUnusedPayloadType2 = 127; + EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType1)); + EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType2)); + + // SSRCs for RTX. + cricket::StreamParams params = + cricket::StreamParams::CreateLegacy(kSsrcs1[0]); + params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]); + AddRecvStream(params); + + // Original 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_before = + fake_call_->GetVideoReceiveStreams()[0]->GetConfig(); + EXPECT_EQ(1U, config_before.rtp.rtx_payload_types.size()); + auto it_before = + config_before.rtp.rtx_payload_types.find(GetEngineCodec("VP8").id); + ASSERT_NE(it_before, config_before.rtp.rtx_payload_types.end()); + EXPECT_EQ(kUnusedPayloadType1, it_before->second); + EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx_ssrc); + + // Change payload type for RTX. + parameters.codecs[1].id = kUnusedPayloadType2; + EXPECT_TRUE(channel_->SetRecvParameters(parameters)); + ASSERT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()); + const webrtc::VideoReceiveStream::Config& config_after = + fake_call_->GetVideoReceiveStreams()[0]->GetConfig(); + EXPECT_EQ(1U, config_after.rtp.rtx_payload_types.size()); + auto it_after = + config_after.rtp.rtx_payload_types.find(GetEngineCodec("VP8").id); + ASSERT_NE(it_after, config_after.rtp.rtx_payload_types.end()); + EXPECT_EQ(kUnusedPayloadType2, it_after->second); + EXPECT_EQ(kRtxSsrcs1[0], config_after.rtp.rtx_ssrc); +} + TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsDifferentPayloadType) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); @@ -3441,21 +3518,19 @@ TEST_F(WebRtcVideoChannel2Test, DefaultReceiveStreamReconfiguresToUseRtx) { ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()) << "No default receive stream created."; FakeVideoReceiveStream* recv_stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_TRUE(recv_stream->GetConfig().rtp.rtx.empty()) + EXPECT_EQ(0u, recv_stream->GetConfig().rtp.rtx_ssrc) << "Default receive stream should not have configured RTX"; EXPECT_TRUE(channel_->AddRecvStream( cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs))); ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()) - << "AddRecvStream should've reconfigured, not added a new receiver."; + << "AddRecvStream should have reconfigured, not added a new receiver."; recv_stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx.empty()); + EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx_payload_types.empty()); EXPECT_EQ(recv_stream->GetConfig().decoders.size(), - recv_stream->GetConfig().rtp.rtx.size()) + recv_stream->GetConfig().rtp.rtx_payload_types.size()) << "RTX should be mapped for all decoders/payload types."; - for (const auto& kv : recv_stream->GetConfig().rtp.rtx) { - EXPECT_EQ(rtx_ssrcs[0], kv.second.ssrc); - } + EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc); } TEST_F(WebRtcVideoChannel2Test, RejectsAddingStreamsWithMissingSsrcsForRtx) { diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc index 3fd2c25fe8..9794ab811a 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc @@ -311,13 +311,11 @@ EventLogAnalyzer::EventLogAnalyzer(const ParsedRtcEventLog& log) StreamId stream(config.rtp.remote_ssrc, kIncomingPacket); extension_maps[stream] = RtpHeaderExtensionMap(config.rtp.extensions); video_ssrcs_.insert(stream); - for (auto kv : config.rtp.rtx) { - StreamId rtx_stream(kv.second.ssrc, kIncomingPacket); - extension_maps[rtx_stream] = - RtpHeaderExtensionMap(config.rtp.extensions); - video_ssrcs_.insert(rtx_stream); - rtx_ssrcs_.insert(rtx_stream); - } + StreamId rtx_stream(config.rtp.rtx_ssrc, kIncomingPacket); + extension_maps[rtx_stream] = + RtpHeaderExtensionMap(config.rtp.extensions); + video_ssrcs_.insert(rtx_stream); + rtx_ssrcs_.insert(rtx_stream); break; } case ParsedRtcEventLog::VIDEO_SENDER_CONFIG_EVENT: { diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index fab6f8e186..6812467cb7 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1091,8 +1091,8 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) { if (retransmission_ssrc_ == kSendRtxSsrcs[0]) { send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]); send_config->rtp.rtx.payload_type = kSendRtxPayloadType; - (*receive_configs)[0].rtp.rtx[payload_type_].ssrc = kSendRtxSsrcs[0]; - (*receive_configs)[0].rtp.rtx[payload_type_].payload_type = + (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0]; + (*receive_configs)[0].rtp.rtx_payload_types[payload_type_] = kSendRtxPayloadType; } // Configure encoding and decoding with VP8, since generic packetization @@ -2331,9 +2331,8 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, if (use_rtx_) { send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]); send_config->rtp.rtx.payload_type = kSendRtxPayloadType; - (*receive_configs)[0].rtp.rtx[kFakeVideoSendPayloadType].ssrc = - kSendRtxSsrcs[0]; - (*receive_configs)[0].rtp.rtx[kFakeVideoSendPayloadType].payload_type = + (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0]; + (*receive_configs)[0].rtp.rtx_payload_types[kFakeVideoSendPayloadType] = kSendRtxPayloadType; } // RTT needed for RemoteNtpTimeEstimator for the receive stream. @@ -2953,9 +2952,8 @@ TEST_P(EndToEndTest, GetStats) { (*receive_configs)[i].renderer = &receive_stream_renderer_; (*receive_configs)[i].rtp.nack.rtp_history_ms = kNackRtpHistoryMs; - (*receive_configs)[i].rtp.rtx[kFakeVideoSendPayloadType].ssrc = - kSendRtxSsrcs[i]; - (*receive_configs)[i].rtp.rtx[kFakeVideoSendPayloadType].payload_type = + (*receive_configs)[i].rtp.rtx_ssrc = kSendRtxSsrcs[i]; + (*receive_configs)[i].rtp.rtx_payload_types[kFakeVideoSendPayloadType] = kSendRtxPayloadType; } @@ -3939,7 +3937,9 @@ TEST_P(EndToEndTest, VerifyDefaultVideoReceiveConfigParameters) { EXPECT_FALSE( default_receive_config.rtp.rtcp_xr.receiver_reference_time_report) << "RTCP XR settings require rtcp-xr to be negotiated."; - EXPECT_TRUE(default_receive_config.rtp.rtx.empty()) + EXPECT_EQ(0U, default_receive_config.rtp.rtx_ssrc) + << "Enabling RTX requires ssrc-group: FID negotiation"; + EXPECT_TRUE(default_receive_config.rtp.rtx_payload_types.empty()) << "Enabling RTX requires rtpmap: rtx negotiation."; EXPECT_TRUE(default_receive_config.rtp.extensions.empty()) << "Enabling RTP extensions require negotiation."; diff --git a/webrtc/video/receive_statistics_proxy.cc b/webrtc/video/receive_statistics_proxy.cc index 8ac2c43e9a..6fe85745c3 100644 --- a/webrtc/video/receive_statistics_proxy.cc +++ b/webrtc/video/receive_statistics_proxy.cc @@ -78,8 +78,11 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( avg_rtt_ms_(0), frame_window_accumulated_bytes_(0) { stats_.ssrc = config_.rtp.remote_ssrc; - for (auto it : config_.rtp.rtx) - rtx_stats_[it.second.ssrc] = StreamDataCounters(); + // TODO(brandtr): Replace |rtx_stats_| with a single instance of + // StreamDataCounters. + if (config_.rtp.rtx_ssrc) { + rtx_stats_[config_.rtp.rtx_ssrc] = StreamDataCounters(); + } } ReceiveStatisticsProxy::~ReceiveStatisticsProxy() { diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index e8acf48b00..c4081f3a76 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -150,14 +150,13 @@ RtpStreamReceiver::RtpStreamReceiver( : kDefaultMaxReorderingThreshold; rtp_receive_statistics_->SetMaxReorderingThreshold(max_reordering_threshold); - // TODO(pbos): Support multiple RTX, per video payload. - for (const auto& kv : config_.rtp.rtx) { - RTC_DCHECK(kv.second.ssrc != 0); - RTC_DCHECK(kv.second.payload_type != 0); + if (config_.rtp.rtx_ssrc) { + rtp_payload_registry_.SetRtxSsrc(config_.rtp.rtx_ssrc); - rtp_payload_registry_.SetRtxSsrc(kv.second.ssrc); - rtp_payload_registry_.SetRtxPayloadType(kv.second.payload_type, - kv.first); + for (const auto& kv : config_.rtp.rtx_payload_types) { + RTC_DCHECK(kv.second != 0); + rtp_payload_registry_.SetRtxPayloadType(kv.second, kv.first); + } } if (IsUlpfecEnabled()) { diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index bd9c4f089c..c1e07864d2 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -1124,8 +1124,8 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, for (size_t i = 0; i < num_video_streams; ++i) { video_receive_configs_[i].rtp.nack.rtp_history_ms = kNackRtpHistoryMs; - video_receive_configs_[i].rtp.rtx[payload_type].ssrc = kSendRtxSsrcs[i]; - video_receive_configs_[i].rtp.rtx[payload_type].payload_type = + video_receive_configs_[i].rtp.rtx_ssrc = kSendRtxSsrcs[i]; + video_receive_configs_[i].rtp.rtx_payload_types[payload_type] = kSendRtxPayloadType; video_receive_configs_[i].rtp.transport_cc = params_.call.send_side_bwe; } diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 5e8754b6bf..0ab2707ea6 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -100,12 +100,10 @@ std::string VideoReceiveStream::Config::Rtp::ToString() const { ss << ", transport_cc: " << (transport_cc ? "on" : "off"); ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}'; ss << ", ulpfec: " << ulpfec.ToString(); - ss << ", rtx: {"; - for (auto& kv : rtx) { - ss << kv.first << " -> "; - ss << "{ssrc: " << kv.second.ssrc; - ss << ", payload_type: " << kv.second.payload_type; - ss << '}'; + ss << ", rtx_ssrc: " << rtx_ssrc; + ss << ", rtx_payload_types: {"; + for (auto& kv : rtx_payload_types) { + ss << kv.first << " (apt) -> " << kv.second << " (pt), "; } ss << '}'; ss << ", extensions: ["; diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h index 7d97a29c69..511db3462c 100644 --- a/webrtc/video_receive_stream.h +++ b/webrtc/video_receive_stream.h @@ -117,6 +117,7 @@ class VideoReceiveStream { // Synchronization source (stream identifier) to be received. uint32_t remote_ssrc = 0; + // Sender SSRC used for sending RTCP (such as receiver reports). uint32_t local_ssrc = 0; @@ -142,19 +143,12 @@ class VideoReceiveStream { // See UlpfecConfig for description. UlpfecConfig ulpfec; - // RTX settings for incoming video payloads that may be received. RTX is - // disabled if there's no config present. - struct Rtx { - // SSRCs to use for the RTX streams. - uint32_t ssrc = 0; + // SSRC for retransmissions. + uint32_t rtx_ssrc = 0; - // Payload type to use for the RTX stream. - int payload_type = 0; - }; - - // Map from video RTP payload type -> RTX config. - typedef std::map RtxMap; - RtxMap rtx; + // Map from video payload type (apt) -> RTX payload type (pt). + // For RTX to be enabled, both an SSRC and this mapping are needed. + std::map rtx_payload_types; // RTP header extensions used for the received stream. std::vector extensions;