diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc index 2b85452ac3..ed44dad5e5 100644 --- a/webrtc/call/rampup_tests.cc +++ b/webrtc/call/rampup_tests.cc @@ -207,9 +207,8 @@ void RampUpTester::ModifyVideoConfigs( 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/webrtc/call/rtx_receive_stream.cc b/webrtc/call/rtx_receive_stream.cc index 6a5432fea6..16463525c7 100644 --- a/webrtc/call/rtx_receive_stream.cc +++ b/webrtc/call/rtx_receive_stream.cc @@ -11,21 +11,17 @@ #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, - ReceiveStatistics* rtp_receive_statistics /* = nullptr */) +RtxReceiveStream::RtxReceiveStream(RtpPacketSinkInterface* media_sink, + std::map 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( 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 c288a27eda..418775c778 100644 --- a/webrtc/call/rtx_receive_stream.h +++ b/webrtc/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 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 @@ 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 f199ca1f85..e51184e2dd 100644 --- a/webrtc/call/video_receive_stream.h +++ b/webrtc/call/video_receive_stream.h @@ -167,11 +167,6 @@ 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 a613dc6de6..9c0031ab48 100644 --- a/webrtc/media/engine/webrtcvideoengine.cc +++ b/webrtc/media/engine/webrtcvideoengine.cc @@ -2187,11 +2187,6 @@ 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 1306082abf..c6adfd3976 100644 --- a/webrtc/media/engine/webrtcvideoengine_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine_unittest.cc @@ -83,32 +83,6 @@ 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) { @@ -138,6 +112,15 @@ 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 { @@ -1333,12 +1316,9 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithSimAndRtx) { 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 @@ 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) { @@ -3822,11 +3796,9 @@ TEST_F(WebRtcVideoChannelTest, DefaultReceiveStreamReconfiguresToUseRtx) { 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/webrtc/video/BUILD.gn b/webrtc/video/BUILD.gn index 16f6bd0050..7756ff416d 100644 --- a/webrtc/video/BUILD.gn +++ b/webrtc/video/BUILD.gn @@ -61,9 +61,6 @@ 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 97b9ee7e35..644319cf79 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1184,9 +1184,7 @@ 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[(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/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc index 1dbd86988d..a47055abf0 100644 --- a/webrtc/video/rtp_video_stream_receiver.cc +++ b/webrtc/video/rtp_video_stream_receiver.cc @@ -86,7 +86,6 @@ 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, @@ -103,11 +102,12 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( 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 @@ 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()) { @@ -164,6 +168,11 @@ 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) @@ -486,7 +495,31 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( } 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(header.payloadType); + return; + } + restored_packet_in_use_ = true; + OnRecoveredPacket(restored_packet_, packet_length); + restored_packet_in_use_ = false; } } diff --git a/webrtc/video/rtp_video_stream_receiver.h b/webrtc/video/rtp_video_stream_receiver.h index 15afc18919..f65280e627 100644 --- a/webrtc/video/rtp_video_stream_receiver.h +++ b/webrtc/video/rtp_video_stream_receiver.h @@ -72,7 +72,6 @@ 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, @@ -181,11 +180,13 @@ class RtpVideoStreamReceiver : public RtpData, const std::unique_ptr rtp_header_parser_; const std::unique_ptr rtp_receiver_; - ReceiveStatistics* const rtp_receive_statistics_; + const std::unique_ptr rtp_receive_statistics_; std::unique_ptr 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 rtp_rtcp_; diff --git a/webrtc/video/rtp_video_stream_receiver_unittest.cc b/webrtc/video/rtp_video_stream_receiver_unittest.cc index 4f2dca7338..d309397dd4 100644 --- a/webrtc/video/rtp_video_stream_receiver_unittest.cc +++ b/webrtc/video/rtp_video_stream_receiver_unittest.cc @@ -126,14 +126,11 @@ class RtpVideoStreamReceiverTest : public testing::Test { process_thread_(ProcessThread::Create("TestThread")) {} void SetUp() { - rtp_receive_statistics_ = - rtc::WrapUnique(ReceiveStatistics::Create(Clock::GetRealTimeClock())); - rtp_video_stream_receiver_ = rtc::MakeUnique( + 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 @@ 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_receive_stream.cc b/webrtc/video/video_receive_stream.cc index a89e3ce366..74c90134b9 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/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 @@ 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_), @@ -98,7 +95,6 @@ VideoReceiveStream::VideoReceiveStream( call_stats_->rtcp_rtt_stats(), packet_router, &config_, - rtp_receive_statistics_.get(), &stats_proxy_, process_thread_, this, // NackSender @@ -124,7 +120,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( @@ -135,12 +131,9 @@ VideoReceiveStream::VideoReceiveStream( // 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( - &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/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 216ef12eda..833e189fb1 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -38,7 +38,6 @@ class ProcessThread; class RTPFragmentationHeader; class RtpStreamReceiverInterface; class RtpStreamReceiverControllerInterface; -class RtxReceiveStream; class VCMTiming; class VCMJitterEstimator; @@ -126,10 +125,6 @@ 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_; @@ -146,7 +141,6 @@ 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