diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc index ed44dad5e5..2b85452ac3 100644 --- a/webrtc/call/rampup_tests.cc +++ b/webrtc/call/rampup_tests.cc @@ -207,8 +207,9 @@ void RampUpTester::ModifyVideoConfigs( 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/webrtc/call/rtx_receive_stream.cc b/webrtc/call/rtx_receive_stream.cc index 16463525c7..6a5432fea6 100644 --- a/webrtc/call/rtx_receive_stream.cc +++ b/webrtc/call/rtx_receive_stream.cc @@ -11,17 +11,21 @@ #include #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 associated_payload_types, - uint32_t media_ssrc) +RtxReceiveStream::RtxReceiveStream( + RtpPacketSinkInterface* media_sink, + std::map associated_payload_types, + uint32_t media_ssrc, + ReceiveStatistics* rtp_receive_statistics /* = nullptr */) : media_sink_(media_sink), associated_payload_types_(std::move(associated_payload_types)), - media_ssrc_(media_ssrc) { + media_ssrc_(media_ssrc), + rtp_receive_statistics_(rtp_receive_statistics) { if (associated_payload_types_.empty()) { LOG(LS_WARNING) << "RtxReceiveStream created with empty payload type mapping."; @@ -31,6 +35,12 @@ RtxReceiveStream::RtxReceiveStream(RtpPacketSinkInterface* media_sink, 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 payload = rtx_packet.payload(); if (payload.size() < kRtxHeaderSize) { diff --git a/webrtc/call/rtx_receive_stream.h b/webrtc/call/rtx_receive_stream.h index 418775c778..c288a27eda 100644 --- a/webrtc/call/rtx_receive_stream.h +++ b/webrtc/call/rtx_receive_stream.h @@ -17,13 +17,20 @@ 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 associated_payload_types, - uint32_t media_ssrc); + 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); ~RtxReceiveStream() override; // RtpPacketSinkInterface. void OnRtpPacket(const RtpPacketReceived& packet) override; @@ -35,6 +42,7 @@ class RtxReceiveStream : public RtpPacketSinkInterface { // 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/webrtc/call/video_receive_stream.h b/webrtc/call/video_receive_stream.h index e51184e2dd..f199ca1f85 100644 --- a/webrtc/call/video_receive_stream.h +++ b/webrtc/call/video_receive_stream.h @@ -167,6 +167,11 @@ class VideoReceiveStream { 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/webrtc/media/engine/webrtcvideoengine.cc b/webrtc/media/engine/webrtcvideoengine.cc index 9c0031ab48..a613dc6de6 100644 --- a/webrtc/media/engine/webrtcvideoengine.cc +++ b/webrtc/media/engine/webrtcvideoengine.cc @@ -2187,6 +2187,11 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureCodecs( 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/webrtc/media/engine/webrtcvideoengine_unittest.cc b/webrtc/media/engine/webrtcvideoengine_unittest.cc index c6adfd3976..1306082abf 100644 --- a/webrtc/media/engine/webrtcvideoengine_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine_unittest.cc @@ -83,6 +83,32 @@ bool HasRtxCodec(const std::vector& codecs, return false; } +// TODO(nisse): Duplicated in call.cc. +const int* FindKeyByValue(const std::map& 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 CreateBlackFrameBuffer( int width, int height) { @@ -112,15 +138,6 @@ cricket::MediaConfig GetMediaConfig() { return media_config; } -// TODO(nisse): Duplicated in call.cc. -const int* FindKeyByValue(const std::map& m, int v) { - for (const auto& kv : m) { - if (kv.second == v) - return &kv.first; - } - return nullptr; -} - } // namespace namespace cricket { @@ -1316,9 +1333,12 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithSimAndRtx) { 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 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithRtx) { 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 @@ TEST_F(WebRtcVideoChannelTest, DefaultReceiveStreamReconfiguresToUseRtx) { 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/webrtc/video/BUILD.gn b/webrtc/video/BUILD.gn index 7756ff416d..16f6bd0050 100644 --- a/webrtc/video/BUILD.gn +++ b/webrtc/video/BUILD.gn @@ -61,6 +61,9 @@ rtc_static_library("video") { "../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/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index ffd52fd531..e92d8fe2d4 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1184,7 +1184,9 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) { 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/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc index a47055abf0..1dbd86988d 100644 --- a/webrtc/video/rtp_video_stream_receiver.cc +++ b/webrtc/video/rtp_video_stream_receiver.cc @@ -86,6 +86,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( RtcpRttStats* rtt_stats, PacketRouter* packet_router, const VideoReceiveStream::Config* config, + ReceiveStatistics* rtp_receive_statistics, ReceiveStatisticsProxy* receive_stats_proxy, ProcessThread* process_thread, NackSender* nack_sender, @@ -102,12 +103,11 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( this, this, &rtp_payload_registry_)), - rtp_receive_statistics_(ReceiveStatistics::Create(clock_)), + rtp_receive_statistics_(rtp_receive_statistics), 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(), + rtp_rtcp_(CreateRtpRtcpModule(rtp_receive_statistics_, transport, rtt_stats, receive_stats_proxy, @@ -146,12 +146,8 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( 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 +164,6 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( 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 +486,7 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( } 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(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/webrtc/video/rtp_video_stream_receiver.h b/webrtc/video/rtp_video_stream_receiver.h index 3c8b2aa167..994ef55e89 100644 --- a/webrtc/video/rtp_video_stream_receiver.h +++ b/webrtc/video/rtp_video_stream_receiver.h @@ -72,6 +72,7 @@ class RtpVideoStreamReceiver : public RtpData, RtcpRttStats* rtt_stats, PacketRouter* packet_router, const VideoReceiveStream::Config* config, + ReceiveStatistics* rtp_receive_statistics, ReceiveStatisticsProxy* receive_stats_proxy, ProcessThread* process_thread, NackSender* nack_sender, @@ -180,13 +181,11 @@ class RtpVideoStreamReceiver : public RtpData, const std::unique_ptr rtp_header_parser_; const std::unique_ptr rtp_receiver_; - const std::unique_ptr rtp_receive_statistics_; + ReceiveStatistics* const rtp_receive_statistics_; std::unique_ptr ulpfec_receiver_; rtc::SequencedTaskChecker worker_task_checker_; bool receiving_ RTC_GUARDED_BY(worker_task_checker_); - uint8_t restored_packet_[IP_PACKET_SIZE] RTC_GUARDED_BY(worker_task_checker_); - bool restored_packet_in_use_ RTC_GUARDED_BY(worker_task_checker_); int64_t last_packet_log_ms_ RTC_GUARDED_BY(worker_task_checker_); const std::unique_ptr rtp_rtcp_; diff --git a/webrtc/video/rtp_video_stream_receiver_unittest.cc b/webrtc/video/rtp_video_stream_receiver_unittest.cc index d309397dd4..4f2dca7338 100644 --- a/webrtc/video/rtp_video_stream_receiver_unittest.cc +++ b/webrtc/video/rtp_video_stream_receiver_unittest.cc @@ -126,11 +126,14 @@ class RtpVideoStreamReceiverTest : public testing::Test { process_thread_(ProcessThread::Create("TestThread")) {} void SetUp() { - rtp_video_stream_receiver_.reset(new RtpVideoStreamReceiver( + rtp_receive_statistics_ = + rtc::WrapUnique(ReceiveStatistics::Create(Clock::GetRealTimeClock())); + rtp_video_stream_receiver_ = rtc::MakeUnique( &mock_transport_, nullptr, &packet_router_, &config_, - nullptr, process_thread_.get(), &mock_nack_sender_, + rtp_receive_statistics_.get(), nullptr, process_thread_.get(), + &mock_nack_sender_, &mock_key_frame_request_sender_, &mock_on_complete_frame_callback_, - &timing_)); + &timing_); } WebRtcRTPHeader GetDefaultPacket() { @@ -196,6 +199,7 @@ class RtpVideoStreamReceiverTest : public testing::Test { PacketRouter packet_router_; VCMTiming timing_; std::unique_ptr process_thread_; + std::unique_ptr rtp_receive_statistics_; std::unique_ptr rtp_video_stream_receiver_; }; diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index 11e060566b..6294184f2a 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -1555,6 +1555,9 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, video_send_config_.rtp.ulpfec.ulpfec_payload_type; it->rtp.ulpfec.red_rtx_payload_type = video_send_config_.rtp.ulpfec.red_rtx_payload_type; + it->rtp.rtx_associated_payload_types[video_send_config_.rtp.ulpfec + .red_rtx_payload_type] = + video_send_config_.rtp.ulpfec.red_payload_type; } } else { video_receive_configs_[params_.ss.selected_stream] @@ -1566,6 +1569,10 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, video_receive_configs_[params_.ss.selected_stream] .rtp.ulpfec.red_rtx_payload_type = video_send_config_.rtp.ulpfec.red_rtx_payload_type; + video_receive_configs_[params_.ss.selected_stream] + .rtp.rtx_associated_payload_types[video_send_config_.rtp.ulpfec + .red_rtx_payload_type] = + video_send_config_.rtp.ulpfec.red_payload_type; } } } diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 74c90134b9..a89e3ce366 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -18,6 +18,7 @@ #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" @@ -32,6 +33,7 @@ #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" @@ -88,6 +90,7 @@ VideoReceiveStream::VideoReceiveStream( "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_), @@ -95,6 +98,7 @@ VideoReceiveStream::VideoReceiveStream( call_stats_->rtcp_rtt_stats(), packet_router, &config_, + rtp_receive_statistics_.get(), &stats_proxy_, process_thread_, this, // NackSender @@ -120,7 +124,7 @@ VideoReceiveStream::VideoReceiveStream( 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 +135,12 @@ VideoReceiveStream::VideoReceiveStream( // 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( + &rtp_video_stream_receiver_, config.rtp.rtx_associated_payload_types, + config_.rtp.remote_ssrc, rtp_receive_statistics_.get()); rtx_receiver_ = receiver_controller->CreateReceiver( - config_.rtp.rtx_ssrc, &rtp_video_stream_receiver_); + config_.rtp.rtx_ssrc, rtx_receive_stream_.get()); } } diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 32ce812326..8b680aeb1e 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -38,6 +38,7 @@ class ProcessThread; class RTPFragmentationHeader; class RtpStreamReceiverInterface; class RtpStreamReceiverControllerInterface; +class RtxReceiveStream; class VCMTiming; class VCMJitterEstimator; @@ -125,6 +126,10 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, 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 rtp_receive_statistics_; + std::unique_ptr timing_; // Jitter buffer experiment. vcm::VideoReceiver video_receiver_; std::unique_ptr> incoming_video_stream_; @@ -141,6 +146,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, std::unique_ptr frame_buffer_; std::unique_ptr media_receiver_; + std::unique_ptr rtx_receive_stream_; std::unique_ptr rtx_receiver_; // Whenever we are in an undecodable state (stream has just started or due to