diff --git a/api/ortc/rtp_transport_interface.h b/api/ortc/rtp_transport_interface.h index ec712163da..ac43831d2d 100644 --- a/api/ortc/rtp_transport_interface.h +++ b/api/ortc/rtp_transport_interface.h @@ -26,12 +26,8 @@ class RtpTransportAdapter; struct RtpTransportParameters final { RtcpParameters rtcp; - // Enabled periodic sending of keep-alive packets, that help prevent timeouts - // on the network level, such as NAT bindings. See RFC6263 section 4.6. - RtpKeepAliveConfig keepalive; - bool operator==(const RtpTransportParameters& o) const { - return rtcp == o.rtcp && keepalive == o.keepalive; + return rtcp == o.rtcp; } bool operator!=(const RtpTransportParameters& o) const { return !(*this == o); diff --git a/api/rtp_headers.h b/api/rtp_headers.h index 8ab560c95c..04c38536b7 100644 --- a/api/rtp_headers.h +++ b/api/rtp_headers.h @@ -171,23 +171,6 @@ enum NetworkState { kNetworkDown, }; -struct RtpKeepAliveConfig final { - // If no packet has been sent for |timeout_interval_ms|, send a keep-alive - // packet. The keep-alive packet is an empty (no payload) RTP packet with a - // payload type of 20 as long as the other end has not negotiated the use of - // this value. If this value has already been negotiated, then some other - // unused static payload type from table 5 of RFC 3551 shall be used and set - // in |payload_type|. - int64_t timeout_interval_ms = -1; - uint8_t payload_type = 20; - - bool operator==(const RtpKeepAliveConfig& o) const { - return timeout_interval_ms == o.timeout_interval_ms && - payload_type == o.payload_type; - } - bool operator!=(const RtpKeepAliveConfig& o) const { return !(*this == o); } -}; - } // namespace webrtc #endif // API_RTP_HEADERS_H_ diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index 5262cc8412..18a6326d4c 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -48,7 +48,6 @@ class RtpVideoSenderInterface; class RateLimiter; class RtcpBandwidthObserver; class RtpPacketSender; -struct RtpKeepAliveConfig; class SendDelayStats; class SendStatisticsProxy; class TransportFeedbackObserver; diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 9dc3003f18..4446e64c0f 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -74,7 +74,6 @@ std::vector CreateRtpStreamSenders( RtcEventLog* event_log, RateLimiter* retransmission_rate_limiter, OverheadObserver* overhead_observer, - RtpKeepAliveConfig keepalive_config, FrameEncryptorInterface* frame_encryptor, const CryptoOptions& crypto_options) { RTC_DCHECK_GT(rtp_config.ssrcs.size(), 0); @@ -100,7 +99,6 @@ std::vector CreateRtpStreamSenders( configuration.event_log = event_log; configuration.retransmission_rate_limiter = retransmission_rate_limiter; configuration.overhead_observer = overhead_observer; - configuration.keepalive_config = keepalive_config; configuration.frame_encryptor = frame_encryptor; configuration.require_frame_encryption = crypto_options.sframe.require_frame_encryption; @@ -247,8 +245,6 @@ RtpVideoSender::RtpVideoSender( event_log, retransmission_limiter, this, - // TODO(srte): Remove this argument. - RtpKeepAliveConfig(), frame_encryptor, crypto_options)), rtp_config_(rtp_config), diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 34ccfeb02a..9ae6ddb8bc 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -97,8 +97,6 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { OverheadObserver* overhead_observer = nullptr; RtcpAckObserver* ack_observer = nullptr; - RtpKeepAliveConfig keepalive_config; - int rtcp_report_interval_ms = 0; // Update network2 instead of pacer_exit field of video timing extension. diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index b6489426d4..a4e75ba687 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -97,7 +97,7 @@ enum RTCPPacketType : uint32_t { enum KeyFrameRequestMethod { kKeyFrameReqPliRtcp, kKeyFrameReqFirRtcp }; -enum RtpRtcpPacketType { kPacketRtp = 0, kPacketKeepAlive = 1 }; +enum RtpRtcpPacketType { kPacketRtp = 0 }; enum RtxMode { kRtxOff = 0x0, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 6d1c9e2ba5..9a0fc7c81e 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -82,12 +82,10 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) : kDefaultVideoReportInterval), this), clock_(configuration.clock), - keepalive_config_(configuration.keepalive_config), last_bitrate_process_time_(clock_->TimeInMilliseconds()), last_rtt_process_time_(clock_->TimeInMilliseconds()), next_process_time_(clock_->TimeInMilliseconds() + kRtpRtcpMaxIdleTimeProcessMs), - next_keepalive_time_(-1), packet_overhead_(28), // IPV4 UDP. nack_last_time_sent_full_ms_(0), nack_last_seq_number_sent_(0), @@ -119,11 +117,6 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) // Make sure rtcp sender use same timestamp offset as rtp sender. rtcp_sender_.SetTimestampOffset(rtp_sender_->TimestampOffset()); - - if (keepalive_config_.timeout_interval_ms != -1) { - next_keepalive_time_ = - clock_->TimeInMilliseconds() + keepalive_config_.timeout_interval_ms; - } } // Set default packet size limit. @@ -154,20 +147,6 @@ void ModuleRtpRtcpImpl::Process() { next_process_time_ = std::min(next_process_time_, now + kRtpRtcpBitrateProcessTimeMs); } - if (keepalive_config_.timeout_interval_ms > 0 && - now >= next_keepalive_time_) { - int64_t last_send_time_ms = rtp_sender_->LastTimestampTimeMs(); - // If no packet has been sent, |last_send_time_ms| will be 0, and so the - // keep-alive will be triggered as expected. - if (now >= last_send_time_ms + keepalive_config_.timeout_interval_ms) { - rtp_sender_->SendKeepAlive(keepalive_config_.payload_type); - next_keepalive_time_ = now + keepalive_config_.timeout_interval_ms; - } else { - next_keepalive_time_ = - last_send_time_ms + keepalive_config_.timeout_interval_ms; - } - next_process_time_ = std::min(next_process_time_, next_keepalive_time_); - } } bool process_rtt = now >= last_rtt_process_time_ + kRtpRtcpRttProcessTimeMs; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 4b1c9279fe..3c7e4ef616 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -320,11 +320,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { Clock* const clock_; - const RtpKeepAliveConfig keepalive_config_; int64_t last_bitrate_process_time_; int64_t last_rtt_process_time_; int64_t next_process_time_; - int64_t next_keepalive_time_; uint16_t packet_overhead_; // Send side diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 98067b2b93..84db79a901 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -60,9 +60,7 @@ class SendTransport : public Transport { clock_(nullptr), delay_ms_(0), rtp_packets_sent_(0), - rtcp_packets_sent_(0), - keepalive_payload_type_(0), - num_keepalive_sent_(0) {} + rtcp_packets_sent_(0) {} void SetRtpRtcpModule(ModuleRtpRtcpImpl* receiver) { receiver_ = receiver; } void SimulateNetworkDelay(int64_t delay_ms, SimulatedClock* clock) { @@ -76,8 +74,6 @@ class SendTransport : public Transport { std::unique_ptr parser(RtpHeaderParser::Create()); EXPECT_TRUE(parser->Parse(static_cast(data), len, &header)); ++rtp_packets_sent_; - if (header.payloadType == keepalive_payload_type_) - ++num_keepalive_sent_; last_rtp_header_ = header; return true; } @@ -94,10 +90,6 @@ class SendTransport : public Transport { ++rtcp_packets_sent_; return true; } - void SetKeepalivePayloadType(uint8_t payload_type) { - keepalive_payload_type_ = payload_type; - } - size_t NumKeepaliveSent() { return num_keepalive_sent_; } size_t NumRtcpSent() { return rtcp_packets_sent_; } ModuleRtpRtcpImpl* receiver_; SimulatedClock* clock_; @@ -106,8 +98,6 @@ class SendTransport : public Transport { size_t rtcp_packets_sent_; RTPHeader last_rtp_header_; std::vector last_nack_list_; - uint8_t keepalive_payload_type_; - size_t num_keepalive_sent_; }; class RtpRtcpModule : public RtcpPacketTypeCounterObserver { @@ -127,7 +117,6 @@ class RtpRtcpModule : public RtcpPacketTypeCounterObserver { RtcpRttStatsTestImpl rtt_stats_; std::unique_ptr impl_; uint32_t remote_ssrc_; - RtpKeepAliveConfig keepalive_config_; int rtcp_report_interval_ms_ = 0; void SetRemoteSsrc(uint32_t ssrc) { @@ -157,12 +146,6 @@ class RtpRtcpModule : public RtcpPacketTypeCounterObserver { std::vector LastNackListSent() { return transport_.last_nack_list_; } - void SetKeepaliveConfigAndReset(const RtpKeepAliveConfig& config) { - keepalive_config_ = config; - // Need to create a new module impl, since it's configured at creation. - CreateModuleImpl(); - transport_.SetKeepalivePayloadType(config.payload_type); - } void SetRtcpReportIntervalAndReset(int rtcp_report_interval_ms) { rtcp_report_interval_ms_ = rtcp_report_interval_ms; CreateModuleImpl(); @@ -177,7 +160,6 @@ class RtpRtcpModule : public RtcpPacketTypeCounterObserver { config.receive_statistics = receive_statistics_.get(); config.rtcp_packet_type_counter_observer = this; config.rtt_stats = &rtt_stats_; - config.keepalive_config = keepalive_config_; config.rtcp_report_interval_ms = rtcp_report_interval_ms_; impl_.reset(new ModuleRtpRtcpImpl(config)); @@ -569,60 +551,6 @@ TEST_F(RtpRtcpImplTest, UniqueNackRequests) { EXPECT_EQ(75, sender_.RtcpReceived().UniqueNackRequestsInPercent()); } -TEST_F(RtpRtcpImplTest, SendsKeepaliveAfterTimout) { - const int kTimeoutMs = 1500; - - RtpKeepAliveConfig config; - config.timeout_interval_ms = kTimeoutMs; - - // Recreate sender impl with new configuration, and redo setup. - sender_.SetKeepaliveConfigAndReset(config); - SetUp(); - - // Initial process call. - sender_.impl_->Process(); - EXPECT_EQ(0U, sender_.transport_.NumKeepaliveSent()); - - // After one time, a single keep-alive packet should be sent. - clock_.AdvanceTimeMilliseconds(kTimeoutMs); - sender_.impl_->Process(); - EXPECT_EQ(1U, sender_.transport_.NumKeepaliveSent()); - - // Process for the same timestamp again, no new packet should be sent. - sender_.impl_->Process(); - EXPECT_EQ(1U, sender_.transport_.NumKeepaliveSent()); - - // Move ahead to the last ms before a keep-alive is expected, no action. - clock_.AdvanceTimeMilliseconds(kTimeoutMs - 1); - sender_.impl_->Process(); - EXPECT_EQ(1U, sender_.transport_.NumKeepaliveSent()); - - // Move the final ms, timeout relative last KA. Should create new keep-alive. - clock_.AdvanceTimeMilliseconds(1); - sender_.impl_->Process(); - EXPECT_EQ(2U, sender_.transport_.NumKeepaliveSent()); - - // Move ahead to the last ms before Christmas. - clock_.AdvanceTimeMilliseconds(kTimeoutMs - 1); - sender_.impl_->Process(); - EXPECT_EQ(2U, sender_.transport_.NumKeepaliveSent()); - - // Send actual payload data, no keep-alive expected. - SendFrame(&sender_, sender_video_.get(), 0); - sender_.impl_->Process(); - EXPECT_EQ(2U, sender_.transport_.NumKeepaliveSent()); - - // Move ahead as far as possible again, timeout now relative payload. No KA. - clock_.AdvanceTimeMilliseconds(kTimeoutMs - 1); - sender_.impl_->Process(); - EXPECT_EQ(2U, sender_.transport_.NumKeepaliveSent()); - - // Timeout relative payload, send new keep-alive. - clock_.AdvanceTimeMilliseconds(1); - sender_.impl_->Process(); - EXPECT_EQ(3U, sender_.transport_.NumKeepaliveSent()); -} - TEST_F(RtpRtcpImplTest, ConfigurableRtcpReportInterval) { const int kVideoReportInterval = 3000; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 197f52ba11..9f3cefce8e 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1229,21 +1229,6 @@ int64_t RTPSender::LastTimestampTimeMs() const { return last_timestamp_time_ms_; } -void RTPSender::SendKeepAlive(uint8_t payload_type) { - std::unique_ptr packet = AllocatePacket(); - packet->SetPayloadType(payload_type); - // Set marker bit and timestamps in the same manner as plain padding packets. - packet->SetMarker(false); - { - rtc::CritScope lock(&send_critsect_); - packet->SetTimestamp(last_rtp_timestamp_); - packet->set_capture_time_ms(capture_time_ms_); - } - AssignSequenceNumber(packet.get()); - SendToNetwork(std::move(packet), StorageType::kDontRetransmit, - RtpPacketSender::Priority::kLowPriority); -} - void RTPSender::SetRtt(int64_t rtt_ms) { packet_history_.SetRtt(rtt_ms); flexfec_packet_history_.SetRtt(rtt_ms); diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index f08eb896b2..eca86eefae 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -171,7 +171,6 @@ class RTPSender { RtpState GetRtxRtpState() const; int64_t LastTimestampTimeMs() const; - void SendKeepAlive(uint8_t payload_type); void SetRtt(int64_t rtt_ms); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 1c74e7e4c1..3f3f2214cd 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1759,40 +1759,6 @@ TEST_P(RtpSenderTest, DoesNotUpdateOverheadOnEqualSize) { SendGenericPacket(); } -TEST_P(RtpSenderTest, SendsKeepAlive) { - MockTransport transport; - rtp_sender_.reset( - new RTPSender(false, &fake_clock_, &transport, nullptr, absl::nullopt, - nullptr, nullptr, nullptr, nullptr, &mock_rtc_event_log_, - nullptr, &retransmission_rate_limiter_, nullptr, false, - nullptr, false, false, FieldTrialBasedConfig())); - rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetTimestampOffset(0); - rtp_sender_->SetSSRC(kSsrc); - - const uint8_t kKeepalivePayloadType = 20; - RTC_CHECK_NE(kKeepalivePayloadType, kPayload); - - EXPECT_CALL(transport, SendRtp(_, _, _)) - .WillOnce( - Invoke([&kKeepalivePayloadType](const uint8_t* packet, size_t len, - const PacketOptions& options) { - webrtc::RTPHeader rtp_header; - RtpUtility::RtpHeaderParser parser(packet, len); - EXPECT_TRUE(parser.Parse(&rtp_header, nullptr)); - EXPECT_FALSE(rtp_header.markerBit); - EXPECT_EQ(0U, rtp_header.paddingLength); - EXPECT_EQ(kKeepalivePayloadType, rtp_header.payloadType); - EXPECT_EQ(kSeqNum, rtp_header.sequenceNumber); - EXPECT_EQ(kSsrc, rtp_header.ssrc); - EXPECT_EQ(0u, len - rtp_header.headerLength); - return true; - })); - - rtp_sender_->SendKeepAlive(kKeepalivePayloadType); - EXPECT_EQ(kSeqNum + 1, rtp_sender_->SequenceNumber()); -} - INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTest, ::testing::Bool()); diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc index 0b076f79ec..650015ef83 100644 --- a/pc/rtp_transport.cc +++ b/pc/rtp_transport.cc @@ -169,12 +169,6 @@ RTCError RtpTransport::SetParameters(const RtpTransportParameters& parameters) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, "Disabling RTCP muxing is not allowed."); } - if (parameters.keepalive != parameters_.keepalive) { - // TODO(sprang): Wire up support for keep-alive (only ORTC support for now). - LOG_AND_RETURN_ERROR( - RTCErrorType::INVALID_MODIFICATION, - "RTP keep-alive parameters not supported by this channel."); - } RtpTransportParameters new_parameters = parameters; diff --git a/pc/rtp_transport_unittest.cc b/pc/rtp_transport_unittest.cc index f617445a06..54f182d2a6 100644 --- a/pc/rtp_transport_unittest.cc +++ b/pc/rtp_transport_unittest.cc @@ -52,17 +52,6 @@ TEST(RtpTransportTest, SetRtcpParametersEmptyCnameUsesExisting) { EXPECT_EQ(transport.GetParameters().rtcp.cname, kName); } -TEST(RtpTransportTest, SetRtpTransportKeepAliveNotSupported) { - // Tests that we warn users that keep-alive isn't supported yet. - // TODO(sprang): Wire up keep-alive and remove this test. - RtpTransport transport(kMuxDisabled); - RtpTransportParameters params; - params.keepalive.timeout_interval_ms = 1; - auto result = transport.SetParameters(params); - EXPECT_FALSE(result.ok()); - EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type()); -} - class SignalObserver : public sigslot::has_slots<> { public: explicit SignalObserver(RtpTransport* transport) { diff --git a/test/call_test.cc b/test/call_test.cc index 44df9cbe34..8d8a9d3853 100644 --- a/test/call_test.cc +++ b/test/call_test.cc @@ -705,9 +705,6 @@ const uint32_t CallTest::kReceiverLocalVideoSsrc = 0x123456; const uint32_t CallTest::kReceiverLocalAudioSsrc = 0x1234567; const int CallTest::kNackRtpHistoryMs = 1000; -const uint8_t CallTest::kDefaultKeepalivePayloadType = - RtpKeepAliveConfig().payload_type; - const std::map CallTest::payload_type_map_ = { {CallTest::kVideoSendPayloadType, MediaType::VIDEO}, {CallTest::kFakeVideoSendPayloadType, MediaType::VIDEO}, @@ -716,8 +713,7 @@ const std::map CallTest::payload_type_map_ = { {CallTest::kRtxRedPayloadType, MediaType::VIDEO}, {CallTest::kUlpfecPayloadType, MediaType::VIDEO}, {CallTest::kFlexfecPayloadType, MediaType::VIDEO}, - {CallTest::kAudioSendPayloadType, MediaType::AUDIO}, - {CallTest::kDefaultKeepalivePayloadType, MediaType::ANY}}; + {CallTest::kAudioSendPayloadType, MediaType::AUDIO}}; BaseTest::BaseTest() {} diff --git a/test/call_test.h b/test/call_test.h index 052d4c6137..dcdb03b2a1 100644 --- a/test/call_test.h +++ b/test/call_test.h @@ -67,7 +67,6 @@ class CallTest : public ::testing::Test { static const uint32_t kReceiverLocalVideoSsrc; static const uint32_t kReceiverLocalAudioSsrc; static const int kNackRtpHistoryMs; - static const uint8_t kDefaultKeepalivePayloadType; static const std::map payload_type_map_; protected: diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 53846bfaea..a84b129e4b 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -144,7 +144,6 @@ class VideoSendStreamImplTest : public ::testing::Test { CallStats call_stats_; SendStatisticsProxy stats_proxy_; PacketRouter packet_router_; - RtpKeepAliveConfig keepalive_config_; }; TEST_F(VideoSendStreamImplTest, RegistersAsBitrateObserverOnStart) {