From db2a9fc6ec119052139d42f8897412bba7fa848b Mon Sep 17 00:00:00 2001 From: sprang Date: Wed, 9 Aug 2017 06:42:32 -0700 Subject: [PATCH] Wire up RTP keep-alive in ortc api. [This CL is work in progress.] Wire up the rtp keep-alive in webrtc::Call::Config using new SetRtpTransportParameters() method on RtpTransportInterface. BUG=webrtc:7907 Review-Url: https://codereview.webrtc.org/2981513002 Cr-Commit-Position: refs/heads/master@{#19287} --- webrtc/api/ortc/ortcfactoryinterface.h | 8 +- webrtc/api/ortc/rtptransportinterface.h | 32 +++- webrtc/call/BUILD.gn | 1 + webrtc/call/call.cc | 4 +- webrtc/call/call.h | 6 - webrtc/call/call_unittest.cc | 1 + .../call/fake_rtp_transport_controller_send.h | 10 ++ webrtc/call/rtp_transport_controller_send.cc | 9 ++ webrtc/call/rtp_transport_controller_send.h | 5 + .../rtp_transport_controller_send_interface.h | 2 + webrtc/common_types.h | 8 +- webrtc/media/base/fakemediaengine.h | 27 +++- webrtc/media/base/test/mock_mediachannel.h | 8 +- webrtc/ortc/BUILD.gn | 1 + webrtc/ortc/ortcfactory.cc | 20 +-- webrtc/ortc/ortcfactory.h | 7 +- webrtc/ortc/ortcfactory_integrationtest.cc | 46 +++--- webrtc/ortc/ortcfactory_unittest.cc | 59 +++---- webrtc/ortc/ortcrtpreceiver_unittest.cc | 12 +- webrtc/ortc/ortcrtpsender_unittest.cc | 12 +- webrtc/ortc/rtptransport_unittest.cc | 146 ++++++++++++------ webrtc/ortc/rtptransportadapter.cc | 78 ++++++---- webrtc/ortc/rtptransportadapter.h | 14 +- webrtc/ortc/rtptransportcontrolleradapter.cc | 65 ++++++-- webrtc/ortc/rtptransportcontrolleradapter.h | 13 +- webrtc/ortc/srtptransport_unittest.cc | 4 +- webrtc/ortc/testrtpparameters.h | 8 +- webrtc/pc/rtptransport.cc | 26 ++-- webrtc/pc/rtptransport.h | 6 +- webrtc/pc/rtptransport_unittest.cc | 33 ++-- webrtc/pc/srtptransport.h | 8 +- webrtc/test/BUILD.gn | 1 + webrtc/test/call_test.cc | 17 +- webrtc/test/call_test.h | 4 + webrtc/video/BUILD.gn | 1 + webrtc/video/video_send_stream.cc | 22 +-- webrtc/video/video_send_stream.h | 3 +- webrtc/video/video_send_stream_tests.cc | 13 +- 38 files changed, 464 insertions(+), 276 deletions(-) diff --git a/webrtc/api/ortc/ortcfactoryinterface.h b/webrtc/api/ortc/ortcfactoryinterface.h index 4f0a9beb13..4880d9d9ca 100644 --- a/webrtc/api/ortc/ortcfactoryinterface.h +++ b/webrtc/api/ortc/ortcfactoryinterface.h @@ -113,16 +113,16 @@ class OrtcFactoryInterface { // |rtp| will be used for sending RTP packets, and |rtcp| for RTCP packets. // // |rtp| can't be null. |rtcp| must be non-null if and only if - // |rtcp_parameters.mux| is false, indicating that RTCP muxing isn't used. + // |rtp_parameters.rtcp.mux| is false, indicating that RTCP muxing isn't used. // Note that if RTCP muxing isn't enabled initially, it can still enabled - // later through SetRtcpParameters. + // later through SetParameters. // // If |transport_controller| is null, one will automatically be created, and // its lifetime managed by the returned RtpTransport. This should only be // done if a single RtpTransport is being used to communicate with the remote // endpoint. virtual RTCErrorOr> CreateRtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& rtp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) = 0; @@ -130,7 +130,7 @@ class OrtcFactoryInterface { // Creates an SrtpTransport which is an RTP transport that uses SRTP. virtual RTCErrorOr> CreateSrtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& rtp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) = 0; diff --git a/webrtc/api/ortc/rtptransportinterface.h b/webrtc/api/ortc/rtptransportinterface.h index 119d662edd..165daada6b 100644 --- a/webrtc/api/ortc/rtptransportinterface.h +++ b/webrtc/api/ortc/rtptransportinterface.h @@ -15,13 +15,14 @@ #include "webrtc/api/ortc/packettransportinterface.h" #include "webrtc/api/rtcerror.h" +#include "webrtc/common_types.h" #include "webrtc/rtc_base/optional.h" namespace webrtc { class RtpTransportAdapter; -struct RtcpParameters { +struct RtcpParameters final { // The SSRC to be used in the "SSRC of packet sender" field. If not set, one // will be chosen by the implementation. // TODO(deadbeef): Not implemented. @@ -34,7 +35,7 @@ struct RtcpParameters { // RtpTransports created by the same OrtcFactory will use the same generated // CNAME. // - // If empty when passed into SetRtcpParameters, the CNAME simply won't be + // If empty when passed into SetParameters, the CNAME simply won't be // modified. std::string cname; @@ -51,6 +52,21 @@ struct RtcpParameters { bool operator!=(const RtcpParameters& o) const { return !(*this == o); } }; +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; + } + bool operator!=(const RtpTransportParameters& o) const { + return !(*this == o); + } +}; + // Base class for different types of RTP transports that can be created by an // OrtcFactory. Used by RtpSenders/RtpReceivers. // @@ -74,16 +90,20 @@ class RtpTransportInterface { // RTCP multiplexing is being used, returns null. virtual PacketTransportInterface* GetRtcpPacketTransport() const = 0; - // Set/get RTCP params. Can be used to enable RTCP muxing or reduced-size - // RTCP if initially not enabled. + // Set/get RTP/RTCP transport params. Can be used to enable RTCP muxing or + // reduced-size RTCP if initially not enabled. // // Changing |mux| from "true" to "false" is not allowed, and changing the // CNAME is currently unsupported. - virtual RTCError SetRtcpParameters(const RtcpParameters& parameters) = 0; + // RTP keep-alive settings need to be set before before an RtpSender has + // started sending, altering the payload type or timeout interval after this + // point is not supported. The parameters must also match across all RTP + // transports for a given RTP transport controller. + virtual RTCError SetParameters(const RtpTransportParameters& parameters) = 0; // Returns last set or constructed-with parameters. If |cname| was empty in // construction, the generated CNAME will be present in the returned // parameters (see above). - virtual RtcpParameters GetRtcpParameters() const = 0; + virtual RtpTransportParameters GetParameters() const = 0; protected: // Only for internal use. Returns a pointer to an internal interface, for use diff --git a/webrtc/call/BUILD.gn b/webrtc/call/BUILD.gn index 813f92b4ab..7bd01048df 100644 --- a/webrtc/call/BUILD.gn +++ b/webrtc/call/BUILD.gn @@ -76,6 +76,7 @@ rtc_source_set("rtp_sender") { ] deps = [ ":rtp_interfaces", + "..:webrtc_common", "../modules/congestion_controller", "../rtc_base:rtc_base_approved", ] diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index 25dcf32a62..61ed66cf43 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -212,7 +212,6 @@ class Call : public webrtc::Call, void OnSentPacket(const rtc::SentPacket& sent_packet) override; - // Implements BitrateObserver. void OnNetworkChanged(uint32_t bitrate_bps, uint8_t fraction_loss, @@ -734,8 +733,7 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( num_cpu_cores_, module_process_thread_.get(), &worker_queue_, call_stats_.get(), transport_send_.get(), bitrate_allocator_.get(), video_send_delay_stats_.get(), event_log_, std::move(config), - std::move(encoder_config), suspended_video_send_ssrcs_, - config_.keepalive_config); + std::move(encoder_config), suspended_video_send_ssrcs_); { WriteLockScoped write_lock(*send_crit_); diff --git a/webrtc/call/call.h b/webrtc/call/call.h index f99ba851ab..86142f0fea 100644 --- a/webrtc/call/call.h +++ b/webrtc/call/call.h @@ -112,12 +112,6 @@ class Call { // RtcEventLog to use for this call. Required. // Use webrtc::RtcEventLog::CreateNull() for a null implementation. RtcEventLog* event_log = nullptr; - - // Enables periodic sending if empty keep-alive messages that helps prevent - // network time-out events. The packets adhere to RFC6263 section 4.6, and - // by default use payload type 20, as described in 3GPP TS 24.229, - // Appendix K.5.2.1. - RtpKeepAliveConfig keepalive_config; }; struct Stats { diff --git a/webrtc/call/call_unittest.cc b/webrtc/call/call_unittest.cc index 2524de6d19..75e5008846 100644 --- a/webrtc/call/call_unittest.cc +++ b/webrtc/call/call_unittest.cc @@ -23,6 +23,7 @@ #include "webrtc/modules/congestion_controller/include/mock/mock_send_side_congestion_controller.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" #include "webrtc/rtc_base/ptr_util.h" +#include "webrtc/test/fake_encoder.h" #include "webrtc/test/gtest.h" #include "webrtc/test/mock_audio_decoder_factory.h" #include "webrtc/test/mock_transport.h" diff --git a/webrtc/call/fake_rtp_transport_controller_send.h b/webrtc/call/fake_rtp_transport_controller_send.h index 2456228f53..ac612ce87e 100644 --- a/webrtc/call/fake_rtp_transport_controller_send.h +++ b/webrtc/call/fake_rtp_transport_controller_send.h @@ -12,6 +12,7 @@ #define WEBRTC_CALL_FAKE_RTP_TRANSPORT_CONTROLLER_SEND_H_ #include "webrtc/call/rtp_transport_controller_send_interface.h" +#include "webrtc/common_types.h" #include "webrtc/modules/congestion_controller/include/send_side_congestion_controller.h" #include "webrtc/modules/pacing/packet_router.h" @@ -39,9 +40,18 @@ class FakeRtpTransportControllerSend RtpPacketSender* packet_sender() override { return send_side_cc_->pacer(); } + const RtpKeepAliveConfig& keepalive_config() const override { + return keepalive_; + } + + void set_keepalive_config(const RtpKeepAliveConfig& keepalive_config) { + keepalive_ = keepalive_config; + } + private: PacketRouter* packet_router_; SendSideCongestionController* send_side_cc_; + RtpKeepAliveConfig keepalive_; }; } // namespace webrtc diff --git a/webrtc/call/rtp_transport_controller_send.cc b/webrtc/call/rtp_transport_controller_send.cc index 878aa191a3..b8b65a0d55 100644 --- a/webrtc/call/rtp_transport_controller_send.cc +++ b/webrtc/call/rtp_transport_controller_send.cc @@ -35,4 +35,13 @@ RtpPacketSender* RtpTransportControllerSend::packet_sender() { return send_side_cc_.pacer(); } +const RtpKeepAliveConfig& RtpTransportControllerSend::keepalive_config() const { + return keepalive_; +} + +void RtpTransportControllerSend::SetKeepAliveConfig( + const RtpKeepAliveConfig& config) { + keepalive_ = config; +} + } // namespace webrtc diff --git a/webrtc/call/rtp_transport_controller_send.h b/webrtc/call/rtp_transport_controller_send.h index da3ae007aa..28e0b2d7a7 100644 --- a/webrtc/call/rtp_transport_controller_send.h +++ b/webrtc/call/rtp_transport_controller_send.h @@ -12,6 +12,7 @@ #define WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_SEND_H_ #include "webrtc/call/rtp_transport_controller_send_interface.h" +#include "webrtc/common_types.h" #include "webrtc/modules/congestion_controller/include/send_side_congestion_controller.h" #include "webrtc/rtc_base/constructormagic.h" @@ -31,10 +32,14 @@ class RtpTransportControllerSend : public RtpTransportControllerSendInterface { SendSideCongestionController* send_side_cc() override; TransportFeedbackObserver* transport_feedback_observer() override; RtpPacketSender* packet_sender() override; + const RtpKeepAliveConfig& keepalive_config() const override; + + void SetKeepAliveConfig(const RtpKeepAliveConfig& config); private: PacketRouter packet_router_; SendSideCongestionController send_side_cc_; + RtpKeepAliveConfig keepalive_; RTC_DISALLOW_COPY_AND_ASSIGN(RtpTransportControllerSend); }; diff --git a/webrtc/call/rtp_transport_controller_send_interface.h b/webrtc/call/rtp_transport_controller_send_interface.h index 9d26e98dc2..bd71da02e9 100644 --- a/webrtc/call/rtp_transport_controller_send_interface.h +++ b/webrtc/call/rtp_transport_controller_send_interface.h @@ -15,6 +15,7 @@ namespace webrtc { class PacketRouter; class RtpPacketSender; +struct RtpKeepAliveConfig; class SendSideCongestionController; class TransportFeedbackObserver; @@ -50,6 +51,7 @@ class RtpTransportControllerSendInterface { virtual TransportFeedbackObserver* transport_feedback_observer() = 0; virtual RtpPacketSender* packet_sender() = 0; + virtual const RtpKeepAliveConfig& keepalive_config() const = 0; }; } // namespace webrtc diff --git a/webrtc/common_types.h b/webrtc/common_types.h index daaf1ff973..dc62b632a1 100644 --- a/webrtc/common_types.h +++ b/webrtc/common_types.h @@ -934,7 +934,7 @@ enum NetworkState { kNetworkDown, }; -struct RtpKeepAliveConfig { +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 @@ -943,6 +943,12 @@ struct RtpKeepAliveConfig { // 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 diff --git a/webrtc/media/base/fakemediaengine.h b/webrtc/media/base/fakemediaengine.h index 0082391d76..0b080afde7 100644 --- a/webrtc/media/base/fakemediaengine.h +++ b/webrtc/media/base/fakemediaengine.h @@ -23,11 +23,13 @@ #include "webrtc/media/base/mediaengine.h" #include "webrtc/media/base/rtputils.h" #include "webrtc/media/base/streamparams.h" +#include "webrtc/media/engine/webrtcvideoengine.h" #include "webrtc/modules/audio_processing/include/audio_processing.h" #include "webrtc/p2p/base/sessiondescription.h" #include "webrtc/rtc_base/checks.h" #include "webrtc/rtc_base/copyonwritebuffer.h" #include "webrtc/rtc_base/networkroute.h" +#include "webrtc/rtc_base/ptr_util.h" #include "webrtc/rtc_base/stringutils.h" using webrtc::RtpExtension; @@ -47,7 +49,10 @@ template class RtpHelper : public Base { fail_set_send_codecs_(false), fail_set_recv_codecs_(false), send_ssrc_(0), - ready_to_send_(false) {} + ready_to_send_(false), + transport_overhead_per_packet_(0), + num_network_route_changes_(0) {} + virtual ~RtpHelper() = default; const std::vector& recv_extensions() { return recv_extensions_; } @@ -299,7 +304,7 @@ template class RtpHelper : public Base { bool ready_to_send_; int transport_overhead_per_packet_; rtc::NetworkRoute last_network_route_; - int num_network_route_changes_ = 0; + int num_network_route_changes_; }; class FakeVoiceMediaChannel : public RtpHelper { @@ -517,8 +522,7 @@ inline bool CompareDtmfInfo(const FakeVoiceMediaChannel::DtmfInfo& info, class FakeVideoMediaChannel : public RtpHelper { public: - explicit FakeVideoMediaChannel(FakeVideoEngine* engine, - const VideoOptions& options) + FakeVideoMediaChannel(FakeVideoEngine* engine, const VideoOptions& options) : engine_(engine), max_bps_(-1) { SetOptions(options); } @@ -838,7 +842,9 @@ class FakeVideoEngine : public FakeBaseEngine { // sanity checks against that. codecs_.push_back(VideoCodec(0, "fake_video_codec")); } + void Init() {} + bool SetOptions(const VideoOptions& options) { options_ = options; options_changed_ = true; @@ -849,21 +855,26 @@ class FakeVideoEngine : public FakeBaseEngine { const MediaConfig& config, const VideoOptions& options) { if (fail_create_channel_) { - return NULL; + return nullptr; } FakeVideoMediaChannel* ch = new FakeVideoMediaChannel(this, options); - channels_.push_back(ch); + channels_.emplace_back(ch); return ch; } + FakeVideoMediaChannel* GetChannel(size_t index) { - return (channels_.size() > index) ? channels_[index] : NULL; + return (channels_.size() > index) ? channels_[index] : nullptr; } + void UnregisterChannel(VideoMediaChannel* channel) { - channels_.erase(std::find(channels_.begin(), channels_.end(), channel)); + auto it = std::find(channels_.begin(), channels_.end(), channel); + RTC_DCHECK(it != channels_.end()); + channels_.erase(it); } const std::vector& codecs() const { return codecs_; } + void SetCodecs(const std::vector codecs) { codecs_ = codecs; } bool SetCapture(bool capture) { diff --git a/webrtc/media/base/test/mock_mediachannel.h b/webrtc/media/base/test/mock_mediachannel.h index 6f7e2e5d05..2d49d4770c 100644 --- a/webrtc/media/base/test/mock_mediachannel.h +++ b/webrtc/media/base/test/mock_mediachannel.h @@ -18,15 +18,15 @@ namespace webrtc { class MockVideoMediaChannel : public cricket::FakeVideoMediaChannel { public: - MockVideoMediaChannel() : - cricket::FakeVideoMediaChannel(NULL, cricket::VideoOptions()) {} + MockVideoMediaChannel() + : cricket::FakeVideoMediaChannel(nullptr, cricket::VideoOptions()) {} MOCK_METHOD1(GetStats, bool(cricket::VideoMediaInfo*)); }; class MockVoiceMediaChannel : public cricket::FakeVoiceMediaChannel { public: - MockVoiceMediaChannel() : - cricket::FakeVoiceMediaChannel(NULL, cricket::AudioOptions()) {} + MockVoiceMediaChannel() + : cricket::FakeVoiceMediaChannel(nullptr, cricket::AudioOptions()) {} MOCK_METHOD1(GetStats, bool(cricket::VoiceMediaInfo*)); }; diff --git a/webrtc/ortc/BUILD.gn b/webrtc/ortc/BUILD.gn index 40c0b2a22b..199ee625c0 100644 --- a/webrtc/ortc/BUILD.gn +++ b/webrtc/ortc/BUILD.gn @@ -36,6 +36,7 @@ rtc_static_library("ortc") { "../api/audio_codecs:builtin_audio_decoder_factory", "../api/audio_codecs:builtin_audio_encoder_factory", "../call:call_interfaces", + "../call:rtp_sender", "../logging:rtc_event_log_api", "../media:rtc_media", "../media:rtc_media_base", diff --git a/webrtc/ortc/ortcfactory.cc b/webrtc/ortc/ortcfactory.cc index be1c4c7a76..d8b117bd2e 100644 --- a/webrtc/ortc/ortcfactory.cc +++ b/webrtc/ortc/ortcfactory.cc @@ -76,14 +76,14 @@ PROXY_METHOD0(RTCErrorOr>, CreateRtpTransportController) PROXY_METHOD4(RTCErrorOr>, CreateRtpTransport, - const RtcpParameters&, + const RtpTransportParameters&, PacketTransportInterface*, PacketTransportInterface*, RtpTransportControllerInterface*) PROXY_METHOD4(RTCErrorOr>, CreateSrtpTransport, - const RtcpParameters&, + const RtpTransportParameters&, PacketTransportInterface*, PacketTransportInterface*, RtpTransportControllerInterface*) @@ -226,14 +226,14 @@ OrtcFactory::CreateRtpTransportController() { RTCErrorOr> OrtcFactory::CreateRtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) { RTC_DCHECK_RUN_ON(signaling_thread_); - RtcpParameters copied_parameters = rtcp_parameters; - if (copied_parameters.cname.empty()) { - copied_parameters.cname = default_cname_; + RtpTransportParameters copied_parameters = parameters; + if (copied_parameters.rtcp.cname.empty()) { + copied_parameters.rtcp.cname = default_cname_; } if (transport_controller) { return transport_controller->GetInternal()->CreateProxiedRtpTransport( @@ -263,14 +263,14 @@ OrtcFactory::CreateRtpTransport( RTCErrorOr> OrtcFactory::CreateSrtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) { RTC_DCHECK_RUN_ON(signaling_thread_); - RtcpParameters copied_parameters = rtcp_parameters; - if (copied_parameters.cname.empty()) { - copied_parameters.cname = default_cname_; + RtpTransportParameters copied_parameters = parameters; + if (copied_parameters.rtcp.cname.empty()) { + copied_parameters.rtcp.cname = default_cname_; } if (transport_controller) { return transport_controller->GetInternal()->CreateProxiedSrtpTransport( diff --git a/webrtc/ortc/ortcfactory.h b/webrtc/ortc/ortcfactory.h index 2ce7be39cc..f93cb637dc 100644 --- a/webrtc/ortc/ortcfactory.h +++ b/webrtc/ortc/ortcfactory.h @@ -44,13 +44,13 @@ class OrtcFactory : public OrtcFactoryInterface { CreateRtpTransportController() override; RTCErrorOr> CreateRtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) override; RTCErrorOr> CreateSrtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerInterface* transport_controller) override; @@ -103,6 +103,9 @@ class OrtcFactory : public OrtcFactoryInterface { rtc::PacketSocketFactory* socket_factory, AudioDeviceModule* adm); + RTCErrorOr> + CreateRtpTransportController(const RtpTransportParameters& parameters); + // Thread::Invoke doesn't support move-only arguments, so we need to remove // the unique_ptr wrapper from media_engine. TODO(deadbeef): Fix this. static RTCErrorOr> Create_s( diff --git a/webrtc/ortc/ortcfactory_integrationtest.cc b/webrtc/ortc/ortcfactory_integrationtest.cc index e05c8f5ccb..513e972bdb 100644 --- a/webrtc/ortc/ortcfactory_integrationtest.cc +++ b/webrtc/ortc/ortcfactory_integrationtest.cc @@ -128,29 +128,29 @@ class OrtcFactoryIntegrationTest : public testing::Test { // empty if RTCP muxing is used. |transport_controllers| can be empty if // these transports are being created using a default transport controller. RtpTransportPair CreateRtpTransportPair( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, const UdpTransportPair& rtp_udp_transports, const UdpTransportPair& rtcp_udp_transports, const RtpTransportControllerPair& transport_controllers) { auto transport_result1 = ortc_factory1_->CreateRtpTransport( - rtcp_parameters, rtp_udp_transports.first.get(), + parameters, rtp_udp_transports.first.get(), rtcp_udp_transports.first.get(), transport_controllers.first.get()); auto transport_result2 = ortc_factory2_->CreateRtpTransport( - rtcp_parameters, rtp_udp_transports.second.get(), + parameters, rtp_udp_transports.second.get(), rtcp_udp_transports.second.get(), transport_controllers.second.get()); return {transport_result1.MoveValue(), transport_result2.MoveValue()}; } SrtpTransportPair CreateSrtpTransportPair( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, const UdpTransportPair& rtp_udp_transports, const UdpTransportPair& rtcp_udp_transports, const RtpTransportControllerPair& transport_controllers) { auto transport_result1 = ortc_factory1_->CreateSrtpTransport( - rtcp_parameters, rtp_udp_transports.first.get(), + parameters, rtp_udp_transports.first.get(), rtcp_udp_transports.first.get(), transport_controllers.first.get()); auto transport_result2 = ortc_factory2_->CreateSrtpTransport( - rtcp_parameters, rtp_udp_transports.second.get(), + parameters, rtp_udp_transports.second.get(), rtcp_udp_transports.second.get(), transport_controllers.second.get()); return {transport_result1.MoveValue(), transport_result2.MoveValue()}; } @@ -158,18 +158,18 @@ class OrtcFactoryIntegrationTest : public testing::Test { // For convenience when |rtcp_udp_transports| and |transport_controllers| // aren't needed. RtpTransportPair CreateRtpTransportPair( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, const UdpTransportPair& rtp_udp_transports) { - return CreateRtpTransportPair(rtcp_parameters, rtp_udp_transports, + return CreateRtpTransportPair(parameters, rtp_udp_transports, UdpTransportPair(), RtpTransportControllerPair()); } SrtpTransportPair CreateSrtpTransportPairAndSetKeys( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, const UdpTransportPair& rtp_udp_transports) { SrtpTransportPair srtp_transports = CreateSrtpTransportPair( - rtcp_parameters, rtp_udp_transports, UdpTransportPair(), + parameters, rtp_udp_transports, UdpTransportPair(), RtpTransportControllerPair()); EXPECT_TRUE(srtp_transports.first->SetSrtpSendKey(kTestCryptoParams1).ok()); EXPECT_TRUE( @@ -182,10 +182,10 @@ class OrtcFactoryIntegrationTest : public testing::Test { } SrtpTransportPair CreateSrtpTransportPairAndSetMismatchingKeys( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, const UdpTransportPair& rtp_udp_transports) { SrtpTransportPair srtp_transports = CreateSrtpTransportPair( - rtcp_parameters, rtp_udp_transports, UdpTransportPair(), + parameters, rtp_udp_transports, UdpTransportPair(), RtpTransportControllerPair()); EXPECT_TRUE(srtp_transports.first->SetSrtpSendKey(kTestCryptoParams1).ok()); EXPECT_TRUE( @@ -558,18 +558,18 @@ TEST_F(OrtcFactoryIntegrationTest, // transport controller. auto transport_controllers = CreateRtpTransportControllerPair(); - RtcpParameters audio_rtcp_parameters; - audio_rtcp_parameters.mux = false; - auto audio_srtp_transports = - CreateSrtpTransportPair(audio_rtcp_parameters, audio_rtp_udp_transports, - audio_rtcp_udp_transports, transport_controllers); + RtpTransportParameters audio_rtp_transport_parameters; + audio_rtp_transport_parameters.rtcp.mux = false; + auto audio_srtp_transports = CreateSrtpTransportPair( + audio_rtp_transport_parameters, audio_rtp_udp_transports, + audio_rtcp_udp_transports, transport_controllers); - RtcpParameters video_rtcp_parameters; - video_rtcp_parameters.mux = false; - video_rtcp_parameters.reduced_size = true; - auto video_srtp_transports = - CreateSrtpTransportPair(video_rtcp_parameters, video_rtp_udp_transports, - video_rtcp_udp_transports, transport_controllers); + RtpTransportParameters video_rtp_transport_parameters; + video_rtp_transport_parameters.rtcp.mux = false; + video_rtp_transport_parameters.rtcp.reduced_size = true; + auto video_srtp_transports = CreateSrtpTransportPair( + video_rtp_transport_parameters, video_rtp_udp_transports, + video_rtcp_udp_transports, transport_controllers); // Set keys for SRTP transports. audio_srtp_transports.first->SetSrtpSendKey(kTestCryptoParams1); diff --git a/webrtc/ortc/ortcfactory_unittest.cc b/webrtc/ortc/ortcfactory_unittest.cc index d964383321..ba8dfe9465 100644 --- a/webrtc/ortc/ortcfactory_unittest.cc +++ b/webrtc/ortc/ortcfactory_unittest.cc @@ -66,16 +66,14 @@ TEST_F(OrtcFactoryTest, CreateRtpTransportWithAndWithoutMux) { rtc::FakePacketTransport rtp("rtp"); rtc::FakePacketTransport rtcp("rtcp"); // With muxed RTCP. - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - auto result = ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, - nullptr, nullptr); + RtpTransportParameters parameters = MakeRtcpMuxParameters(); + auto result = + ortc_factory_->CreateRtpTransport(parameters, &rtp, nullptr, nullptr); EXPECT_TRUE(result.ok()); result.MoveValue().reset(); // With non-muxed RTCP. - rtcp_parameters.mux = false; - result = - ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + parameters.rtcp.mux = false; + result = ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr); EXPECT_TRUE(result.ok()); } @@ -84,16 +82,14 @@ TEST_F(OrtcFactoryTest, CreateSrtpTransport) { rtc::FakePacketTransport rtp("rtp"); rtc::FakePacketTransport rtcp("rtcp"); // With muxed RTCP. - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - auto result = ortc_factory_->CreateSrtpTransport(rtcp_parameters, &rtp, - nullptr, nullptr); + RtpTransportParameters parameters = MakeRtcpMuxParameters(); + auto result = + ortc_factory_->CreateSrtpTransport(parameters, &rtp, nullptr, nullptr); EXPECT_TRUE(result.ok()); result.MoveValue().reset(); // With non-muxed RTCP. - rtcp_parameters.mux = false; - result = - ortc_factory_->CreateSrtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + parameters.rtcp.mux = false; + result = ortc_factory_->CreateSrtpTransport(parameters, &rtp, &rtcp, nullptr); EXPECT_TRUE(result.ok()); } @@ -101,12 +97,10 @@ TEST_F(OrtcFactoryTest, CreateSrtpTransport) { // GetRtpParameters. TEST_F(OrtcFactoryTest, CreateRtpTransportGeneratesCname) { rtc::FakePacketTransport rtp("rtp"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - auto result = ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, + auto result = ortc_factory_->CreateRtpTransport(MakeRtcpMuxParameters(), &rtp, nullptr, nullptr); ASSERT_TRUE(result.ok()); - EXPECT_FALSE(result.value()->GetRtcpParameters().cname.empty()); + EXPECT_FALSE(result.value()->GetParameters().rtcp.cname.empty()); } // Extension of the above test; multiple transports created by the same factory @@ -114,20 +108,19 @@ TEST_F(OrtcFactoryTest, CreateRtpTransportGeneratesCname) { TEST_F(OrtcFactoryTest, MultipleRtpTransportsUseSameGeneratedCname) { rtc::FakePacketTransport packet_transport1("1"); rtc::FakePacketTransport packet_transport2("2"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; + RtpTransportParameters parameters = MakeRtcpMuxParameters(); // Sanity check. - ASSERT_TRUE(rtcp_parameters.cname.empty()); + ASSERT_TRUE(parameters.rtcp.cname.empty()); auto result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &packet_transport1, nullptr, nullptr); + parameters, &packet_transport1, nullptr, nullptr); ASSERT_TRUE(result.ok()); auto rtp_transport1 = result.MoveValue(); - result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &packet_transport2, nullptr, nullptr); + result = ortc_factory_->CreateRtpTransport(parameters, &packet_transport2, + nullptr, nullptr); ASSERT_TRUE(result.ok()); auto rtp_transport2 = result.MoveValue(); - RtcpParameters params1 = rtp_transport1->GetRtcpParameters(); - RtcpParameters params2 = rtp_transport2->GetRtcpParameters(); + RtcpParameters params1 = rtp_transport1->GetParameters().rtcp; + RtcpParameters params2 = rtp_transport2->GetParameters().rtcp; EXPECT_FALSE(params1.cname.empty()); EXPECT_EQ(params1.cname, params2.cname); } @@ -142,10 +135,10 @@ TEST_F(OrtcFactoryTest, CreateRtpTransportWithNoPacketTransport) { // packet transport are needed. TEST_F(OrtcFactoryTest, CreateRtpTransportWithMissingRtcpTransport) { rtc::FakePacketTransport rtp("rtp"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = false; - auto result = ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, - nullptr, nullptr); + RtpTransportParameters parameters; + parameters.rtcp.mux = false; + auto result = + ortc_factory_->CreateRtpTransport(parameters, &rtp, nullptr, nullptr); EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type()); } @@ -155,10 +148,8 @@ TEST_F(OrtcFactoryTest, CreateRtpTransportWithMissingRtcpTransport) { TEST_F(OrtcFactoryTest, CreateRtpTransportWithExtraneousRtcpTransport) { rtc::FakePacketTransport rtp("rtp"); rtc::FakePacketTransport rtcp("rtcp"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - auto result = - ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + auto result = ortc_factory_->CreateRtpTransport(MakeRtcpMuxParameters(), &rtp, + &rtcp, nullptr); EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type()); } diff --git a/webrtc/ortc/ortcrtpreceiver_unittest.cc b/webrtc/ortc/ortcrtpreceiver_unittest.cc index 8273be58c6..0e2b722b79 100644 --- a/webrtc/ortc/ortcrtpreceiver_unittest.cc +++ b/webrtc/ortc/ortcrtpreceiver_unittest.cc @@ -36,10 +36,10 @@ class OrtcRtpReceiverTest : public testing::Test { nullptr, nullptr, nullptr, nullptr, nullptr, std::unique_ptr(fake_media_engine_)); ortc_factory_ = ortc_factory_result.MoveValue(); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport_, nullptr, nullptr); + parameters, &fake_packet_transport_, nullptr, nullptr); rtp_transport_ = rtp_transport_result.MoveValue(); } @@ -97,10 +97,10 @@ TEST_F(OrtcRtpReceiverTest, GetTrack) { // test/tests for it. TEST_F(OrtcRtpReceiverTest, SetTransportFails) { rtc::FakePacketTransport fake_packet_transport("another_transport"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport, nullptr, nullptr); + parameters, &fake_packet_transport, nullptr, nullptr); auto rtp_transport = rtp_transport_result.MoveValue(); auto receiver_result = ortc_factory_->CreateRtpReceiver( diff --git a/webrtc/ortc/ortcrtpsender_unittest.cc b/webrtc/ortc/ortcrtpsender_unittest.cc index ab8d821468..a94ed76ecb 100644 --- a/webrtc/ortc/ortcrtpsender_unittest.cc +++ b/webrtc/ortc/ortcrtpsender_unittest.cc @@ -40,10 +40,10 @@ class OrtcRtpSenderTest : public testing::Test { nullptr, nullptr, nullptr, nullptr, nullptr, std::unique_ptr(fake_media_engine_)); ortc_factory_ = ortc_factory_result.MoveValue(); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport_, nullptr, nullptr); + parameters, &fake_packet_transport_, nullptr, nullptr); rtp_transport_ = rtp_transport_result.MoveValue(); } @@ -153,10 +153,10 @@ TEST_F(OrtcRtpSenderTest, SetTrackOfWrongKindFails) { // test/tests for it. TEST_F(OrtcRtpSenderTest, SetTransportFails) { rtc::FakePacketTransport fake_packet_transport("another_transport"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport, nullptr, nullptr); + parameters, &fake_packet_transport, nullptr, nullptr); auto rtp_transport = rtp_transport_result.MoveValue(); auto sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO, diff --git a/webrtc/ortc/rtptransport_unittest.cc b/webrtc/ortc/rtptransport_unittest.cc index 2fc1b23586..95ee1c79ac 100644 --- a/webrtc/ortc/rtptransport_unittest.cc +++ b/webrtc/ortc/rtptransport_unittest.cc @@ -45,18 +45,17 @@ TEST_F(RtpTransportTest, GetPacketTransports) { rtc::FakePacketTransport rtp("rtp"); rtc::FakePacketTransport rtcp("rtcp"); // With muxed RTCP. - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - auto result = ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, - nullptr, nullptr); + RtpTransportParameters parameters; + parameters.rtcp.mux = true; + auto result = + ortc_factory_->CreateRtpTransport(parameters, &rtp, nullptr, nullptr); ASSERT_TRUE(result.ok()); EXPECT_EQ(&rtp, result.value()->GetRtpPacketTransport()); EXPECT_EQ(nullptr, result.value()->GetRtcpPacketTransport()); result.MoveValue().reset(); // With non-muxed RTCP. - rtcp_parameters.mux = false; - result = - ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + parameters.rtcp.mux = false; + result = ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr); ASSERT_TRUE(result.ok()); EXPECT_EQ(&rtp, result.value()->GetRtpPacketTransport()); EXPECT_EQ(&rtcp, result.value()->GetRtcpPacketTransport()); @@ -70,16 +69,16 @@ TEST_F(RtpTransportTest, EnablingRtcpMuxingUnsetsRtcpTransport) { rtc::FakePacketTransport rtcp("rtcp"); // Create non-muxed. - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = false; + RtpTransportParameters parameters; + parameters.rtcp.mux = false; auto result = - ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr); ASSERT_TRUE(result.ok()); auto rtp_transport = result.MoveValue(); // Enable muxing. - rtcp_parameters.mux = true; - EXPECT_TRUE(rtp_transport->SetRtcpParameters(rtcp_parameters).ok()); + parameters.rtcp.mux = true; + EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok()); EXPECT_EQ(nullptr, rtp_transport->GetRtcpPacketTransport()); } @@ -87,39 +86,39 @@ TEST_F(RtpTransportTest, GetAndSetRtcpParameters) { rtc::FakePacketTransport rtp("rtp"); rtc::FakePacketTransport rtcp("rtcp"); // Start with non-muxed RTCP. - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = false; - rtcp_parameters.cname = "teST"; - rtcp_parameters.reduced_size = false; + RtpTransportParameters parameters; + parameters.rtcp.mux = false; + parameters.rtcp.cname = "teST"; + parameters.rtcp.reduced_size = false; auto result = - ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr); + ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr); ASSERT_TRUE(result.ok()); auto transport = result.MoveValue(); - EXPECT_EQ(rtcp_parameters, transport->GetRtcpParameters()); + EXPECT_EQ(parameters, transport->GetParameters()); // Changing the CNAME is currently unsupported. - rtcp_parameters.cname = "different"; + parameters.rtcp.cname = "different"; EXPECT_EQ(RTCErrorType::UNSUPPORTED_OPERATION, - transport->SetRtcpParameters(rtcp_parameters).type()); - rtcp_parameters.cname = "teST"; + transport->SetParameters(parameters).type()); + parameters.rtcp.cname = "teST"; // Enable RTCP muxing and reduced-size RTCP. - rtcp_parameters.mux = true; - rtcp_parameters.reduced_size = true; - EXPECT_TRUE(transport->SetRtcpParameters(rtcp_parameters).ok()); - EXPECT_EQ(rtcp_parameters, transport->GetRtcpParameters()); + parameters.rtcp.mux = true; + parameters.rtcp.reduced_size = true; + EXPECT_TRUE(transport->SetParameters(parameters).ok()); + EXPECT_EQ(parameters, transport->GetParameters()); // Empty CNAME should result in the existing CNAME being used. - rtcp_parameters.cname.clear(); - EXPECT_TRUE(transport->SetRtcpParameters(rtcp_parameters).ok()); - EXPECT_EQ("teST", transport->GetRtcpParameters().cname); + parameters.rtcp.cname.clear(); + EXPECT_TRUE(transport->SetParameters(parameters).ok()); + EXPECT_EQ("teST", transport->GetParameters().rtcp.cname); // Disabling RTCP muxing after enabling shouldn't be allowed, since enabling // muxing should have made the RTP transport forget about the RTCP packet // transport initially passed into it. - rtcp_parameters.mux = false; + parameters.rtcp.mux = false; EXPECT_EQ(RTCErrorType::INVALID_STATE, - transport->SetRtcpParameters(rtcp_parameters).type()); + transport->SetParameters(parameters).type()); } // When Send or Receive is called on a sender or receiver, the RTCP parameters @@ -129,12 +128,12 @@ TEST_F(RtpTransportTest, GetAndSetRtcpParameters) { TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) { // First, create video transport with reduced-size RTCP. rtc::FakePacketTransport fake_packet_transport1("1"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - rtcp_parameters.reduced_size = true; - rtcp_parameters.cname = "foo"; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; + parameters.rtcp.reduced_size = true; + parameters.rtcp.cname = "foo"; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport1, nullptr, nullptr); + parameters, &fake_packet_transport1, nullptr, nullptr); auto video_transport = rtp_transport_result.MoveValue(); // Create video sender and call Send, expecting parameters to be applied. @@ -163,10 +162,10 @@ TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) { // Create audio transport with non-reduced size RTCP. rtc::FakePacketTransport fake_packet_transport2("2"); - rtcp_parameters.reduced_size = false; - rtcp_parameters.cname = "bar"; + parameters.rtcp.reduced_size = false; + parameters.rtcp.cname = "bar"; rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport2, nullptr, nullptr); + parameters, &fake_packet_transport2, nullptr, nullptr); auto audio_transport = rtp_transport_result.MoveValue(); // Create audio sender and call Send, expecting parameters to be applied. @@ -195,17 +194,17 @@ TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) { EXPECT_FALSE(fake_voice_channel->recv_rtcp_parameters().reduced_size); } -// When SetRtcpParameters is called, the modified parameters should be applied +// When SetParameters is called, the modified parameters should be applied // to the media engine. // TODO(deadbeef): Once the implementation supports changing the CNAME, // test that here. TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) { rtc::FakePacketTransport fake_packet_transport("fake"); - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - rtcp_parameters.reduced_size = false; + RtpTransportParameters parameters; + parameters.rtcp.mux = true; + parameters.rtcp.reduced_size = false; auto rtp_transport_result = ortc_factory_->CreateRtpTransport( - rtcp_parameters, &fake_packet_transport, nullptr, nullptr); + parameters, &fake_packet_transport, nullptr, nullptr); auto rtp_transport = rtp_transport_result.MoveValue(); // Create video sender and call Send, applying an initial set of parameters. @@ -215,8 +214,8 @@ TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) { EXPECT_TRUE(sender->Send(MakeMinimalVp8Parameters()).ok()); // Modify parameters and expect them to be changed at the media engine level. - rtcp_parameters.reduced_size = true; - EXPECT_TRUE(rtp_transport->SetRtcpParameters(rtcp_parameters).ok()); + parameters.rtcp.reduced_size = true; + EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok()); cricket::FakeVideoMediaChannel* fake_video_channel = fake_media_engine_->GetVideoChannel(0); @@ -224,4 +223,61 @@ TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) { EXPECT_TRUE(fake_video_channel->send_rtcp_parameters().reduced_size); } +// SetParameters should set keepalive for all RTP transports. +// It is impossible to modify keepalive parameters if any streams are created. +// Note: This is an implementation detail for current way of configuring the +// keep-alive. It may change in the future. +TEST_F(RtpTransportTest, CantChangeKeepAliveAfterCreatedSendStreams) { + rtc::FakePacketTransport fake_packet_transport("fake"); + RtpTransportParameters parameters; + parameters.keepalive.timeout_interval_ms = 100; + auto rtp_transport_result = ortc_factory_->CreateRtpTransport( + parameters, &fake_packet_transport, nullptr, nullptr); + ASSERT_TRUE(rtp_transport_result.ok()); + std::unique_ptr rtp_transport = + rtp_transport_result.MoveValue(); + + // Updating keepalive parameters is ok, since no rtp sender created. + parameters.keepalive.timeout_interval_ms = 200; + EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok()); + + // Create video sender. Note: |sender_result| scope must extend past the + // SetParameters() call below. + auto sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_VIDEO, + rtp_transport.get()); + EXPECT_TRUE(sender_result.ok()); + + // Modify parameters second time after video send stream created. + parameters.keepalive.timeout_interval_ms = 10; + EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, + rtp_transport->SetParameters(parameters).type()); +} + +// Note: This is an implementation detail for current way of configuring the +// keep-alive. It may change in the future. +TEST_F(RtpTransportTest, KeepAliveMustBeSameAcrossTransportController) { + rtc::FakePacketTransport fake_packet_transport("fake"); + RtpTransportParameters parameters; + parameters.keepalive.timeout_interval_ms = 100; + + // Manually create a controller, that can be shared by multiple transports. + auto controller_result = ortc_factory_->CreateRtpTransportController(); + ASSERT_TRUE(controller_result.ok()); + std::unique_ptr controller = + controller_result.MoveValue(); + + // Create a first transport. + auto first_transport_result = ortc_factory_->CreateRtpTransport( + parameters, &fake_packet_transport, nullptr, controller.get()); + ASSERT_TRUE(first_transport_result.ok()); + + // Update the parameters, and create another transport for the same + // controller. + parameters.keepalive.timeout_interval_ms = 10; + auto seconds_transport_result = ortc_factory_->CreateRtpTransport( + parameters, &fake_packet_transport, nullptr, controller.get()); + EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, + seconds_transport_result.error().type()); +} + } // namespace webrtc diff --git a/webrtc/ortc/rtptransportadapter.cc b/webrtc/ortc/rtptransportadapter.cc index 1154ff629b..10e5e8fefc 100644 --- a/webrtc/ortc/rtptransportadapter.cc +++ b/webrtc/ortc/rtptransportadapter.cc @@ -24,8 +24,8 @@ BEGIN_OWNED_PROXY_MAP(RtpTransport) PROXY_SIGNALING_THREAD_DESTRUCTOR() PROXY_CONSTMETHOD0(PacketTransportInterface*, GetRtpPacketTransport) PROXY_CONSTMETHOD0(PacketTransportInterface*, GetRtcpPacketTransport) -PROXY_METHOD1(RTCError, SetRtcpParameters, const RtcpParameters&) -PROXY_CONSTMETHOD0(RtcpParameters, GetRtcpParameters) +PROXY_METHOD1(RTCError, SetParameters, const RtpTransportParameters&) +PROXY_CONSTMETHOD0(RtpTransportParameters, GetParameters) protected: RtpTransportAdapter* GetInternal() override { return internal(); @@ -36,8 +36,8 @@ BEGIN_OWNED_PROXY_MAP(SrtpTransport) PROXY_SIGNALING_THREAD_DESTRUCTOR() PROXY_CONSTMETHOD0(PacketTransportInterface*, GetRtpPacketTransport) PROXY_CONSTMETHOD0(PacketTransportInterface*, GetRtcpPacketTransport) -PROXY_METHOD1(RTCError, SetRtcpParameters, const RtcpParameters&) -PROXY_CONSTMETHOD0(RtcpParameters, GetRtcpParameters) +PROXY_METHOD1(RTCError, SetParameters, const RtpTransportParameters&) +PROXY_CONSTMETHOD0(RtpTransportParameters, GetParameters) PROXY_METHOD1(RTCError, SetSrtpSendKey, const cricket::CryptoParams&) PROXY_METHOD1(RTCError, SetSrtpReceiveKey, const cricket::CryptoParams&) protected: @@ -49,7 +49,7 @@ END_PROXY_MAP() // static RTCErrorOr> RtpTransportAdapter::CreateProxied( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller) { @@ -57,12 +57,12 @@ RtpTransportAdapter::CreateProxied( LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Must provide an RTP packet transport."); } - if (!rtcp_parameters.mux && !rtcp) { + if (!parameters.rtcp.mux && !rtcp) { LOG_AND_RETURN_ERROR( RTCErrorType::INVALID_PARAMETER, "Must provide an RTCP packet transport when RTCP muxing is not used."); } - if (rtcp_parameters.mux && rtcp) { + if (parameters.rtcp.mux && rtcp) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Creating an RtpTransport with RTCP muxing enabled, " "with a separate RTCP packet transport?"); @@ -74,17 +74,23 @@ RtpTransportAdapter::CreateProxied( LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Must provide an RTP transport controller."); } + std::unique_ptr transport_adapter( + new RtpTransportAdapter(parameters.rtcp, rtp, rtcp, + rtp_transport_controller, + false /*is_srtp_transport*/)); + RTCError params_result = transport_adapter->SetParameters(parameters); + if (!params_result.ok()) { + return std::move(params_result); + } + return RtpTransportProxyWithInternal::Create( rtp_transport_controller->signaling_thread(), - rtp_transport_controller->worker_thread(), - std::unique_ptr(new RtpTransportAdapter( - rtcp_parameters, rtp, rtcp, rtp_transport_controller, - /*is_srtp_transport*/ false))); + rtp_transport_controller->worker_thread(), std::move(transport_adapter)); } RTCErrorOr> RtpTransportAdapter::CreateSrtpProxied( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller) { @@ -92,12 +98,12 @@ RtpTransportAdapter::CreateSrtpProxied( LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Must provide an RTP packet transport."); } - if (!rtcp_parameters.mux && !rtcp) { + if (!parameters.rtcp.mux && !rtcp) { LOG_AND_RETURN_ERROR( RTCErrorType::INVALID_PARAMETER, "Must provide an RTCP packet transport when RTCP muxing is not used."); } - if (rtcp_parameters.mux && rtcp) { + if (parameters.rtcp.mux && rtcp) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Creating an RtpTransport with RTCP muxing enabled, " "with a separate RTCP packet transport?"); @@ -109,12 +115,18 @@ RtpTransportAdapter::CreateSrtpProxied( LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Must provide an RTP transport controller."); } + std::unique_ptr transport_adapter( + new RtpTransportAdapter(parameters.rtcp, rtp, rtcp, + rtp_transport_controller, + true /*is_srtp_transport*/)); + RTCError params_result = transport_adapter->SetParameters(parameters); + if (!params_result.ok()) { + return std::move(params_result); + } + return SrtpTransportProxyWithInternal::Create( rtp_transport_controller->signaling_thread(), - rtp_transport_controller->worker_thread(), - std::unique_ptr(new RtpTransportAdapter( - rtcp_parameters, rtp, rtcp, rtp_transport_controller, - /*is_srtp_transport*/ true))); + rtp_transport_controller->worker_thread(), std::move(transport_adapter)); } void RtpTransportAdapter::TakeOwnershipOfRtpTransportController( @@ -125,7 +137,7 @@ void RtpTransportAdapter::TakeOwnershipOfRtpTransportController( } RtpTransportAdapter::RtpTransportAdapter( - const RtcpParameters& rtcp_parameters, + const RtcpParameters& rtcp_params, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller, @@ -133,11 +145,11 @@ RtpTransportAdapter::RtpTransportAdapter( : rtp_packet_transport_(rtp), rtcp_packet_transport_(rtcp), rtp_transport_controller_(rtp_transport_controller), - rtcp_parameters_(rtcp_parameters), is_srtp_transport_(is_srtp_transport) { - RTC_DCHECK(rtp_transport_controller); + parameters_.rtcp = rtcp_params; // CNAME should have been filled by OrtcFactory if empty. - RTC_DCHECK(!rtcp_parameters_.cname.empty()); + RTC_DCHECK(!parameters_.rtcp.cname.empty()); + RTC_DCHECK(rtp_transport_controller); } RtpTransportAdapter::~RtpTransportAdapter() { @@ -152,27 +164,29 @@ PacketTransportInterface* RtpTransportAdapter::GetRtcpPacketTransport() const { return rtcp_packet_transport_; } -RTCError RtpTransportAdapter::SetRtcpParameters( - const RtcpParameters& parameters) { - if (!parameters.mux && rtcp_parameters_.mux) { +RTCError RtpTransportAdapter::SetParameters( + const RtpTransportParameters& parameters) { + if (!parameters.rtcp.mux && parameters_.rtcp.mux) { LOG_AND_RETURN_ERROR(webrtc::RTCErrorType::INVALID_STATE, "Can't disable RTCP muxing after enabling."); } - if (!parameters.cname.empty() && parameters.cname != rtcp_parameters_.cname) { + if (!parameters.rtcp.cname.empty() && + parameters.rtcp.cname != parameters_.rtcp.cname) { LOG_AND_RETURN_ERROR(webrtc::RTCErrorType::UNSUPPORTED_OPERATION, "Changing the RTCP CNAME is currently unsupported."); } // If the CNAME is empty, use the existing one. - RtcpParameters copy = parameters; - if (copy.cname.empty()) { - copy.cname = rtcp_parameters_.cname; + RtpTransportParameters copy = parameters; + if (copy.rtcp.cname.empty()) { + copy.rtcp.cname = parameters_.rtcp.cname; } - RTCError err = rtp_transport_controller_->SetRtcpParameters(copy, this); + RTCError err = + rtp_transport_controller_->SetRtpTransportParameters(copy, this); if (!err.ok()) { return err; } - rtcp_parameters_ = copy; - if (rtcp_parameters_.mux) { + parameters_ = copy; + if (parameters_.rtcp.mux) { rtcp_packet_transport_ = nullptr; } return RTCError::OK(); diff --git a/webrtc/ortc/rtptransportadapter.h b/webrtc/ortc/rtptransportadapter.h index 6fbc162e99..c71a4f41cf 100644 --- a/webrtc/ortc/rtptransportadapter.h +++ b/webrtc/ortc/rtptransportadapter.h @@ -36,13 +36,13 @@ class RtpTransportAdapter : public SrtpTransportInterface { // |rtp| can't be null. |rtcp| can if RTCP muxing is used immediately (meaning // |rtcp_parameters.mux| is also true). static RTCErrorOr> CreateProxied( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& rtcp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller); static RTCErrorOr> CreateSrtpProxied( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& rtcp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller); @@ -52,8 +52,8 @@ class RtpTransportAdapter : public SrtpTransportInterface { // RtpTransportInterface implementation. PacketTransportInterface* GetRtpPacketTransport() const override; PacketTransportInterface* GetRtcpPacketTransport() const override; - RTCError SetRtcpParameters(const RtcpParameters& parameters) override; - RtcpParameters GetRtcpParameters() const override { return rtcp_parameters_; } + RTCError SetParameters(const RtpTransportParameters& parameters) override; + RtpTransportParameters GetParameters() const override { return parameters_; } // SRTP specific implementation. RTCError SetSrtpSendKey(const cricket::CryptoParams& params) override; @@ -82,7 +82,7 @@ class RtpTransportAdapter : public SrtpTransportInterface { RtpTransportAdapter* GetInternal() override { return this; } private: - RtpTransportAdapter(const RtcpParameters& rtcp_parameters, + RtpTransportAdapter(const RtcpParameters& rtcp_params, PacketTransportInterface* rtp, PacketTransportInterface* rtcp, RtpTransportControllerAdapter* rtp_transport_controller, @@ -90,11 +90,11 @@ class RtpTransportAdapter : public SrtpTransportInterface { PacketTransportInterface* rtp_packet_transport_; PacketTransportInterface* rtcp_packet_transport_; - RtpTransportControllerAdapter* rtp_transport_controller_; + RtpTransportControllerAdapter* const rtp_transport_controller_; // Non-null if this class owns the transport controller. std::unique_ptr owned_rtp_transport_controller_; - RtcpParameters rtcp_parameters_; + RtpTransportParameters parameters_; // SRTP specific members. rtc::Optional send_key_; diff --git a/webrtc/ortc/rtptransportcontrolleradapter.cc b/webrtc/ortc/rtptransportcontrolleradapter.cc index 5e0b621123..f2ad995ab3 100644 --- a/webrtc/ortc/rtptransportcontrolleradapter.cc +++ b/webrtc/ortc/rtptransportcontrolleradapter.cc @@ -129,11 +129,16 @@ RtpTransportControllerAdapter::~RtpTransportControllerAdapter() { RTCErrorOr> RtpTransportControllerAdapter::CreateProxiedRtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp) { - auto result = - RtpTransportAdapter::CreateProxied(rtcp_parameters, rtp, rtcp, this); + if (!transport_proxies_.empty() && (parameters.keepalive != keepalive_)) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, + "Cannot create RtpTransport with different keep-alive " + "from the RtpTransports already associated with this " + "transport controller."); + } + auto result = RtpTransportAdapter::CreateProxied(parameters, rtp, rtcp, this); if (result.ok()) { transport_proxies_.push_back(result.value().get()); transport_proxies_.back()->GetInternal()->SignalDestroyed.connect( @@ -144,11 +149,11 @@ RtpTransportControllerAdapter::CreateProxiedRtpTransport( RTCErrorOr> RtpTransportControllerAdapter::CreateProxiedSrtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp) { auto result = - RtpTransportAdapter::CreateSrtpProxied(rtcp_parameters, rtp, rtcp, this); + RtpTransportAdapter::CreateSrtpProxied(parameters, rtp, rtcp, this); if (result.ok()) { transport_proxies_.push_back(result.value().get()); transport_proxies_.back()->GetInternal()->SignalDestroyed.connect( @@ -219,12 +224,26 @@ RtpTransportControllerAdapter::GetTransports() const { return transport_proxies_; } -RTCError RtpTransportControllerAdapter::SetRtcpParameters( - const RtcpParameters& parameters, +RTCError RtpTransportControllerAdapter::SetRtpTransportParameters( + const RtpTransportParameters& parameters, RtpTransportInterface* inner_transport) { + if ((video_channel_ != nullptr || voice_channel_ != nullptr) && + (parameters.keepalive != keepalive_)) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, + "Cannot change keep-alive settings after creating " + "media streams or additional transports for the same " + "transport controller."); + } + // Call must be configured on the worker thread. + worker_thread_->Invoke( + RTC_FROM_HERE, + rtc::Bind(&RtpTransportControllerAdapter::SetRtpTransportParameters_w, + this, parameters)); + do { if (inner_transport == inner_audio_transport_) { - CopyRtcpParametersToDescriptions(parameters, &local_audio_description_, + CopyRtcpParametersToDescriptions(parameters.rtcp, + &local_audio_description_, &remote_audio_description_); if (!voice_channel_->SetLocalContent(&local_audio_description_, cricket::CA_OFFER, nullptr)) { @@ -235,7 +254,8 @@ RTCError RtpTransportControllerAdapter::SetRtcpParameters( break; } } else if (inner_transport == inner_video_transport_) { - CopyRtcpParametersToDescriptions(parameters, &local_video_description_, + CopyRtcpParametersToDescriptions(parameters.rtcp, + &local_video_description_, &remote_video_description_); if (!video_channel_->SetLocalContent(&local_video_description_, cricket::CA_OFFER, nullptr)) { @@ -252,6 +272,11 @@ RTCError RtpTransportControllerAdapter::SetRtcpParameters( "Failed to apply new RTCP parameters."); } +void RtpTransportControllerAdapter::SetRtpTransportParameters_w( + const RtpTransportParameters& parameters) { + call_send_rtp_transport_controller_->SetKeepAliveConfig(parameters.keepalive); +} + RTCError RtpTransportControllerAdapter::ValidateAndApplyAudioSenderParameters( const RtpParameters& parameters, uint32_t* primary_ssrc) { @@ -270,7 +295,7 @@ RTCError RtpTransportControllerAdapter::ValidateAndApplyAudioSenderParameters( } auto stream_params_result = MakeSendStreamParamsVec( - parameters.encodings, inner_audio_transport_->GetRtcpParameters().cname, + parameters.encodings, inner_audio_transport_->GetParameters().rtcp.cname, local_audio_description_); if (!stream_params_result.ok()) { return stream_params_result.MoveError(); @@ -359,7 +384,7 @@ RTCError RtpTransportControllerAdapter::ValidateAndApplyVideoSenderParameters( } auto stream_params_result = MakeSendStreamParamsVec( - parameters.encodings, inner_video_transport_->GetRtcpParameters().cname, + parameters.encodings, inner_video_transport_->GetParameters().rtcp.cname, local_video_description_); if (!stream_params_result.ok()) { return stream_params_result.MoveError(); @@ -590,7 +615,8 @@ RtpTransportControllerAdapter::RtpTransportControllerAdapter( worker_thread_(worker_thread), media_config_(config), channel_manager_(channel_manager), - event_log_(event_log) { + event_log_(event_log), + call_send_rtp_transport_controller_(nullptr) { RTC_DCHECK_RUN_ON(signaling_thread_); RTC_DCHECK(channel_manager_); // Add "dummy" codecs to the descriptions, because the media engines @@ -626,11 +652,16 @@ void RtpTransportControllerAdapter::Init_w() { call_config.bitrate_config.start_bitrate_bps = kStartBandwidthBps; call_config.bitrate_config.max_bitrate_bps = kMaxBandwidthBps; - call_.reset(webrtc::Call::Create(call_config)); + call_send_rtp_transport_controller_ = + new RtpTransportControllerSend(Clock::GetRealTimeClock(), event_log_); + call_.reset(webrtc::Call::Create( + call_config, std::unique_ptr( + call_send_rtp_transport_controller_))); } void RtpTransportControllerAdapter::Close_w() { call_.reset(); + call_send_rtp_transport_controller_ = nullptr; } RTCError RtpTransportControllerAdapter::AttachAudioSender( @@ -656,7 +687,7 @@ RTCError RtpTransportControllerAdapter::AttachAudioSender( // If setting new transport, extract its RTCP parameters and create voice // channel. if (!inner_audio_transport_) { - CopyRtcpParametersToDescriptions(inner_transport->GetRtcpParameters(), + CopyRtcpParametersToDescriptions(inner_transport->GetParameters().rtcp, &local_audio_description_, &remote_audio_description_); inner_audio_transport_ = inner_transport; @@ -691,7 +722,7 @@ RTCError RtpTransportControllerAdapter::AttachVideoSender( // If setting new transport, extract its RTCP parameters and create video // channel. if (!inner_video_transport_) { - CopyRtcpParametersToDescriptions(inner_transport->GetRtcpParameters(), + CopyRtcpParametersToDescriptions(inner_transport->GetParameters().rtcp, &local_video_description_, &remote_video_description_); inner_video_transport_ = inner_transport; @@ -726,7 +757,7 @@ RTCError RtpTransportControllerAdapter::AttachAudioReceiver( // If setting new transport, extract its RTCP parameters and create voice // channel. if (!inner_audio_transport_) { - CopyRtcpParametersToDescriptions(inner_transport->GetRtcpParameters(), + CopyRtcpParametersToDescriptions(inner_transport->GetParameters().rtcp, &local_audio_description_, &remote_audio_description_); inner_audio_transport_ = inner_transport; @@ -761,7 +792,7 @@ RTCError RtpTransportControllerAdapter::AttachVideoReceiver( // If setting new transport, extract its RTCP parameters and create video // channel. if (!inner_video_transport_) { - CopyRtcpParametersToDescriptions(inner_transport->GetRtcpParameters(), + CopyRtcpParametersToDescriptions(inner_transport->GetParameters().rtcp, &local_video_description_, &remote_video_description_); inner_video_transport_ = inner_transport; diff --git a/webrtc/ortc/rtptransportcontrolleradapter.h b/webrtc/ortc/rtptransportcontrolleradapter.h index 9728c3968a..d4494d0d6c 100644 --- a/webrtc/ortc/rtptransportcontrolleradapter.h +++ b/webrtc/ortc/rtptransportcontrolleradapter.h @@ -21,6 +21,7 @@ #include "webrtc/api/ortc/rtptransportcontrollerinterface.h" #include "webrtc/api/ortc/srtptransportinterface.h" #include "webrtc/call/call.h" +#include "webrtc/call/rtp_transport_controller_send.h" #include "webrtc/logging/rtc_event_log/rtc_event_log.h" #include "webrtc/media/base/mediachannel.h" // For MediaConfig. #include "webrtc/pc/channelmanager.h" @@ -77,12 +78,12 @@ class RtpTransportControllerAdapter : public RtpTransportControllerInterface, // these methods return proxies that will safely call methods on the correct // thread. RTCErrorOr> CreateProxiedRtpTransport( - const RtcpParameters& rtcp_parameters, + const RtpTransportParameters& rtcp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp); RTCErrorOr> - CreateProxiedSrtpTransport(const RtcpParameters& rtcp_parameters, + CreateProxiedSrtpTransport(const RtpTransportParameters& rtcp_parameters, PacketTransportInterface* rtp, PacketTransportInterface* rtcp); @@ -100,8 +101,10 @@ class RtpTransportControllerAdapter : public RtpTransportControllerInterface, rtc::Thread* signaling_thread() const { return signaling_thread_; } rtc::Thread* worker_thread() const { return worker_thread_; } - RTCError SetRtcpParameters(const RtcpParameters& parameters, - RtpTransportInterface* inner_transport); + // |parameters.keepalive| will be set for ALL RTP transports in the call. + RTCError SetRtpTransportParameters(const RtpTransportParameters& parameters, + RtpTransportInterface* inner_transport); + void SetRtpTransportParameters_w(const RtpTransportParameters& parameters); cricket::VoiceChannel* voice_channel() { return voice_channel_; } cricket::VideoChannel* video_channel() { return video_channel_; } @@ -193,9 +196,11 @@ class RtpTransportControllerAdapter : public RtpTransportControllerInterface, RtpTransportInterface* inner_audio_transport_ = nullptr; RtpTransportInterface* inner_video_transport_ = nullptr; const cricket::MediaConfig media_config_; + RtpKeepAliveConfig keepalive_; cricket::ChannelManager* channel_manager_; webrtc::RtcEventLog* event_log_; std::unique_ptr call_; + webrtc::RtpTransportControllerSend* call_send_rtp_transport_controller_; // BaseChannel takes content descriptions as input, so we store them here // such that they can be updated when a new RtpSenderAdapter/ diff --git a/webrtc/ortc/srtptransport_unittest.cc b/webrtc/ortc/srtptransport_unittest.cc index 69d8a297e9..ea9b28a0c5 100644 --- a/webrtc/ortc/srtptransport_unittest.cc +++ b/webrtc/ortc/srtptransport_unittest.cc @@ -58,7 +58,7 @@ class SrtpTransportTest : public testing::Test { fake_packet_transport_.reset(new rtc::FakePacketTransport("fake")); auto srtp_transport_result = ortc_factory_->CreateSrtpTransport( - rtcp_parameters_, fake_packet_transport_.get(), nullptr, + rtp_transport_parameters_, fake_packet_transport_.get(), nullptr, rtp_transport_controller_.get()); srtp_transport_ = srtp_transport_result.MoveValue(); } @@ -69,7 +69,7 @@ class SrtpTransportTest : public testing::Test { std::unique_ptr ortc_factory_; std::unique_ptr rtp_transport_controller_; std::unique_ptr srtp_transport_; - RtcpParameters rtcp_parameters_; + RtpTransportParameters rtp_transport_parameters_; std::unique_ptr fake_packet_transport_; }; diff --git a/webrtc/ortc/testrtpparameters.h b/webrtc/ortc/testrtpparameters.h index 87108ca44f..042dab2738 100644 --- a/webrtc/ortc/testrtpparameters.h +++ b/webrtc/ortc/testrtpparameters.h @@ -30,10 +30,10 @@ namespace webrtc { // parameters is applied properly should construct the parameters in the test // itself. -inline RtcpParameters MakeRtcpMuxParameters() { - RtcpParameters rtcp_parameters; - rtcp_parameters.mux = true; - return rtcp_parameters; +inline RtpTransportParameters MakeRtcpMuxParameters() { + RtpTransportParameters parameters; + parameters.rtcp.mux = true; + return parameters; } RtpParameters MakeMinimalOpusParameters(); diff --git a/webrtc/pc/rtptransport.cc b/webrtc/pc/rtptransport.cc index 8a524dd07a..ac57eb887b 100644 --- a/webrtc/pc/rtptransport.cc +++ b/webrtc/pc/rtptransport.cc @@ -115,24 +115,30 @@ PacketTransportInterface* RtpTransport::GetRtcpPacketTransport() const { return rtcp_packet_transport_; } -RTCError RtpTransport::SetRtcpParameters(const RtcpParameters& parameters) { - if (rtcp_parameters_.mux && !parameters.mux) { +RTCError RtpTransport::SetParameters(const RtpTransportParameters& parameters) { + if (parameters_.rtcp.mux && !parameters.rtcp.mux) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, "Disabling RTCP muxing is not allowed."); } - - RtcpParameters new_parameters = parameters; - - if (new_parameters.cname.empty()) { - new_parameters.cname = rtcp_parameters_.cname; + 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."); } - rtcp_parameters_ = new_parameters; + RtpTransportParameters new_parameters = parameters; + + if (new_parameters.rtcp.cname.empty()) { + new_parameters.rtcp.cname = parameters_.rtcp.cname; + } + + parameters_ = new_parameters; return RTCError::OK(); } -RtcpParameters RtpTransport::GetRtcpParameters() const { - return rtcp_parameters_; +RtpTransportParameters RtpTransport::GetParameters() const { + return parameters_; } RtpTransportAdapter* RtpTransport::GetInternal() { diff --git a/webrtc/pc/rtptransport.h b/webrtc/pc/rtptransport.h index cf64a0003d..8b408cabfa 100644 --- a/webrtc/pc/rtptransport.h +++ b/webrtc/pc/rtptransport.h @@ -51,8 +51,8 @@ class RtpTransport : public RtpTransportInternal { PacketTransportInterface* GetRtcpPacketTransport() const override; // TODO(zstein): Use these RtcpParameters for configuration elsewhere. - RTCError SetRtcpParameters(const RtcpParameters& parameters) override; - RtcpParameters GetRtcpParameters() const override; + RTCError SetParameters(const RtpTransportParameters& parameters) override; + RtpTransportParameters GetParameters() const override; bool IsWritable(bool rtcp) const override; @@ -97,7 +97,7 @@ class RtpTransport : public RtpTransportInternal { bool rtp_ready_to_send_ = false; bool rtcp_ready_to_send_ = false; - RtcpParameters rtcp_parameters_; + RtpTransportParameters parameters_; cricket::BundleFilter bundle_filter_; }; diff --git a/webrtc/pc/rtptransport_unittest.cc b/webrtc/pc/rtptransport_unittest.cc index 27c77abcb5..0ed77ee463 100644 --- a/webrtc/pc/rtptransport_unittest.cc +++ b/webrtc/pc/rtptransport_unittest.cc @@ -22,23 +22,34 @@ constexpr bool kMuxEnabled = true; TEST(RtpTransportTest, SetRtcpParametersCantDisableRtcpMux) { RtpTransport transport(kMuxDisabled); - RtcpParameters params; - transport.SetRtcpParameters(params); - params.mux = false; - EXPECT_FALSE(transport.SetRtcpParameters(params).ok()); + RtpTransportParameters params; + transport.SetParameters(params); + params.rtcp.mux = false; + EXPECT_FALSE(transport.SetParameters(params).ok()); } TEST(RtpTransportTest, SetRtcpParametersEmptyCnameUsesExisting) { static const char kName[] = "name"; RtpTransport transport(kMuxDisabled); - RtcpParameters params_with_name; - params_with_name.cname = kName; - transport.SetRtcpParameters(params_with_name); - EXPECT_EQ(transport.GetRtcpParameters().cname, kName); + RtpTransportParameters params_with_name; + params_with_name.rtcp.cname = kName; + transport.SetParameters(params_with_name); + EXPECT_EQ(transport.GetParameters().rtcp.cname, kName); - RtcpParameters params_without_name; - transport.SetRtcpParameters(params_without_name); - EXPECT_EQ(transport.GetRtcpParameters().cname, kName); + RtpTransportParameters params_without_name; + transport.SetParameters(params_without_name); + 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<> { diff --git a/webrtc/pc/srtptransport.h b/webrtc/pc/srtptransport.h index be746d50c8..58ef205f87 100644 --- a/webrtc/pc/srtptransport.h +++ b/webrtc/pc/srtptransport.h @@ -78,12 +78,12 @@ class SrtpTransport : public RtpTransportInternal { rtp_transport_->AddHandledPayloadType(payload_type); } - RtcpParameters GetRtcpParameters() const override { - return rtp_transport_->GetRtcpParameters(); + RTCError SetParameters(const RtpTransportParameters& parameters) override { + return rtp_transport_->SetParameters(parameters); } - RTCError SetRtcpParameters(const RtcpParameters& parameters) override { - return rtp_transport_->SetRtcpParameters(parameters); + RtpTransportParameters GetParameters() const override { + return rtp_transport_->GetParameters(); } // TODO(zstein): Remove this when we remove RtpTransportAdapter. diff --git a/webrtc/test/BUILD.gn b/webrtc/test/BUILD.gn index 799642d1e1..ea82cb0b58 100644 --- a/webrtc/test/BUILD.gn +++ b/webrtc/test/BUILD.gn @@ -479,6 +479,7 @@ rtc_source_set("test_common") { "../api/video_codecs:video_codecs_api", "../audio", "../call", + "../call:rtp_sender", "../common_video", "../logging:rtc_event_log_api", "../modules/audio_device:mock_audio_device", diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 9dacb5b55e..294428eebe 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -14,12 +14,12 @@ #include "webrtc/api/audio_codecs/builtin_audio_decoder_factory.h" #include "webrtc/api/audio_codecs/builtin_audio_encoder_factory.h" +#include "webrtc/call/rtp_transport_controller_send.h" #include "webrtc/config.h" #include "webrtc/modules/audio_mixer/audio_mixer_impl.h" #include "webrtc/rtc_base/checks.h" #include "webrtc/test/testsupport/fileutils.h" #include "webrtc/voice_engine/include/voe_base.h" - namespace webrtc { namespace test { @@ -30,6 +30,7 @@ const int kVideoRotationRtpExtensionId = 4; CallTest::CallTest() : clock_(Clock::GetRealTimeClock()), event_log_(RtcEventLog::CreateNull()), + sender_call_transport_controller_(nullptr), video_send_config_(nullptr), video_send_stream_(nullptr), audio_send_config_(nullptr), @@ -66,6 +67,10 @@ void CallTest::RunBaseTest(BaseTest* test) { send_config.audio_state = AudioState::Create(audio_state_config); } CreateSenderCall(send_config); + if (sender_call_transport_controller_ != nullptr) { + test->OnRtpTransportControllerSendCreated( + sender_call_transport_controller_); + } if (test->ShouldCreateReceivers()) { Call::Config recv_config(test->GetReceiverCallConfig()); if (num_audio_streams_ > 0) { @@ -153,7 +158,12 @@ void CallTest::CreateCalls(const Call::Config& sender_config, } void CallTest::CreateSenderCall(const Call::Config& config) { - sender_call_.reset(Call::Create(config)); + sender_call_transport_controller_ = new RtpTransportControllerSend( + Clock::GetRealTimeClock(), config.event_log); + + sender_call_.reset( + Call::Create(config, std::unique_ptr( + sender_call_transport_controller_))); } void CallTest::CreateReceiverCall(const Call::Config& config) { @@ -505,6 +515,9 @@ Call::Config BaseTest::GetReceiverCallConfig() { return Call::Config(event_log_.get()); } +void BaseTest::OnRtpTransportControllerSendCreated( + RtpTransportControllerSend* controller) {} + void BaseTest::OnCallsCreated(Call* sender_call, Call* receiver_call) { } diff --git a/webrtc/test/call_test.h b/webrtc/test/call_test.h index 11d0198cb0..5186afa753 100644 --- a/webrtc/test/call_test.h +++ b/webrtc/test/call_test.h @@ -14,6 +14,7 @@ #include #include "webrtc/call/call.h" +#include "webrtc/call/rtp_transport_controller_send.h" #include "webrtc/logging/rtc_event_log/rtc_event_log.h" #include "webrtc/test/encoder_settings.h" #include "webrtc/test/fake_audio_device.h" @@ -108,6 +109,7 @@ class CallTest : public ::testing::Test { std::unique_ptr event_log_; std::unique_ptr sender_call_; + RtpTransportControllerSend* sender_call_transport_controller_; std::unique_ptr send_transport_; VideoSendStream::Config video_send_config_; VideoEncoderConfig video_encoder_config_; @@ -182,6 +184,8 @@ class BaseTest : public RtpRtcpObserver { virtual Call::Config GetSenderCallConfig(); virtual Call::Config GetReceiverCallConfig(); + virtual void OnRtpTransportControllerSendCreated( + RtpTransportControllerSend* controller); virtual void OnCallsCreated(Call* sender_call, Call* receiver_call); virtual test::PacketTransport* CreateSendTransport(Call* sender_call); diff --git a/webrtc/video/BUILD.gn b/webrtc/video/BUILD.gn index 02431e0a6d..28025e8939 100644 --- a/webrtc/video/BUILD.gn +++ b/webrtc/video/BUILD.gn @@ -267,6 +267,7 @@ if (rtc_include_tests) { "../call:call_interfaces", "../call:mock_rtp_interfaces", "../call:rtp_receiver", + "../call:rtp_sender", "../common_video", "../logging:rtc_event_log_api", "../media:rtc_media", diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index c2231311f0..3fc4976bc3 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -347,8 +347,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, const VideoSendStream::Config* config, int initial_encoder_max_bitrate, std::map suspended_ssrcs, - VideoEncoderConfig::ContentType content_type, - const RtpKeepAliveConfig& keepalive_config); + VideoEncoderConfig::ContentType content_type); ~VideoSendStreamImpl() override; // RegisterProcessThread register |module_process_thread| with those objects @@ -479,8 +478,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { const VideoSendStream::Config* config, int initial_encoder_max_bitrate, const std::map& suspended_ssrcs, - VideoEncoderConfig::ContentType content_type, - const RtpKeepAliveConfig& keepalive_config) + VideoEncoderConfig::ContentType content_type) : send_stream_(send_stream), done_event_(done_event), stats_proxy_(stats_proxy), @@ -493,8 +491,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { config_(config), initial_encoder_max_bitrate_(initial_encoder_max_bitrate), suspended_ssrcs_(suspended_ssrcs), - content_type_(content_type), - keepalive_config_(keepalive_config) {} + content_type_(content_type) {} ~ConstructionTask() override { done_event_->Set(); } @@ -504,7 +501,7 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { stats_proxy_, rtc::TaskQueue::Current(), call_stats_, transport_, bitrate_allocator_, send_delay_stats_, video_stream_encoder_, event_log_, config_, initial_encoder_max_bitrate_, - std::move(suspended_ssrcs_), content_type_, keepalive_config_)); + std::move(suspended_ssrcs_), content_type_)); return true; } @@ -521,7 +518,6 @@ class VideoSendStream::ConstructionTask : public rtc::QueuedTask { int initial_encoder_max_bitrate_; std::map suspended_ssrcs_; const VideoEncoderConfig::ContentType content_type_; - const RtpKeepAliveConfig& keepalive_config_; }; class VideoSendStream::DestructAndGetRtpStateTask : public rtc::QueuedTask { @@ -634,8 +630,7 @@ VideoSendStream::VideoSendStream( RtcEventLog* event_log, VideoSendStream::Config config, VideoEncoderConfig encoder_config, - const std::map& suspended_ssrcs, - const RtpKeepAliveConfig& keepalive_config) + const std::map& suspended_ssrcs) : worker_queue_(worker_queue), thread_sync_event_(false /* manual_reset */, false), stats_proxy_(Clock::GetRealTimeClock(), @@ -654,7 +649,7 @@ VideoSendStream::VideoSendStream( video_stream_encoder_.get(), module_process_thread, call_stats, transport, bitrate_allocator, send_delay_stats, event_log, &config_, encoder_config.max_bitrate_bps, suspended_ssrcs, - encoder_config.content_type, keepalive_config))); + encoder_config.content_type))); // Wait for ConstructionTask to complete so that |send_stream_| can be used. // |module_process_thread| must be registered and deregistered on the thread @@ -774,8 +769,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( const VideoSendStream::Config* config, int initial_encoder_max_bitrate, std::map suspended_ssrcs, - VideoEncoderConfig::ContentType content_type, - const RtpKeepAliveConfig& keepalive_config) + VideoEncoderConfig::ContentType content_type) : send_side_bwe_with_overhead_( webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")), stats_proxy_(stats_proxy), @@ -813,7 +807,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( transport->send_side_cc()->GetRetransmissionRateLimiter(), this, config_->rtp.ssrcs.size(), - keepalive_config)), + transport->keepalive_config())), payload_router_(rtp_rtcp_modules_, config_->encoder_settings.payload_type), weak_ptr_factory_(this), diff --git a/webrtc/video/video_send_stream.h b/webrtc/video/video_send_stream.h index db34539535..e105d706e3 100644 --- a/webrtc/video/video_send_stream.h +++ b/webrtc/video/video_send_stream.h @@ -58,8 +58,7 @@ class VideoSendStream : public webrtc::VideoSendStream { RtcEventLog* event_log, VideoSendStream::Config config, VideoEncoderConfig encoder_config, - const std::map& suspended_ssrcs, - const RtpKeepAliveConfig& keepalive_config); + const std::map& suspended_ssrcs); ~VideoSendStream() override; diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 8352d4f8db..0d5ba7d325 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -12,6 +12,7 @@ #include #include "webrtc/call/call.h" +#include "webrtc/call/rtp_transport_controller_send.h" #include "webrtc/common_video/include/frame_callback.h" #include "webrtc/common_video/include/video_frame.h" #include "webrtc/modules/rtp_rtcp/include/rtp_header_parser.h" @@ -3379,12 +3380,12 @@ TEST_F(VideoSendStreamTest, SendsKeepAlive) { public: KeepaliveObserver() : SendTest(kDefaultTimeoutMs) {} - Call::Config GetSenderCallConfig() override { - Call::Config config = SendTest::GetSenderCallConfig(); - config.keepalive_config.timeout_interval_ms = kTimeoutMs; - config.keepalive_config.payload_type = - CallTest::kDefaultKeepalivePayloadType; - return config; + void OnRtpTransportControllerSendCreated( + RtpTransportControllerSend* controller) override { + RtpKeepAliveConfig config; + config.timeout_interval_ms = kTimeoutMs; + config.payload_type = CallTest::kDefaultKeepalivePayloadType; + controller->SetKeepAliveConfig(config); } private: