From 55718120e684bd4861f7a94e362a8c1a2e1c7a66 Mon Sep 17 00:00:00 2001 From: Jiawei Ou Date: Fri, 9 Nov 2018 13:17:39 -0800 Subject: [PATCH] Adding rtcp report interval into RTCConfiguration. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a follow up of https://webrtc-review.googlesource.com/c/src/+/43201. Issue 43201 didn't do the job properly. 1. The audio rtcp report interval is not properly hooked up. 2. We don't need to propagate audio rtcp interval into video send stream or vice versa. 3. We don't need to propagate rtcp report interval to any receiving streams. Bug: webrtc:8789 Change-Id: I1f637d6e5173608564ef0702d7eda6fc93b3200f Reviewed-on: https://webrtc-review.googlesource.com/c/110105 Reviewed-by: Erik Språng Reviewed-by: Magnus Flodman Reviewed-by: Niels Moller Commit-Queue: Jiawei Ou Cr-Commit-Position: refs/heads/master@{#25610} --- api/peerconnectioninterface.h | 16 ++++++++++++++++ audio/audio_send_stream.cc | 9 ++++++--- audio/audio_send_stream_unittest.cc | 5 +++-- audio/channel_send.cc | 5 ++++- audio/channel_send.h | 3 ++- call/audio_send_stream.cc | 1 + call/audio_send_stream.h | 3 +++ call/rtp_config.cc | 14 -------------- call/rtp_config.h | 12 ------------ call/rtp_transport_controller_send.cc | 6 +++--- call/rtp_transport_controller_send.h | 2 +- call/rtp_transport_controller_send_interface.h | 2 +- call/rtp_video_sender.cc | 12 +++++------- call/rtp_video_sender.h | 2 +- call/rtp_video_sender_unittest.cc | 2 +- call/test/mock_rtp_transport_controller_send.h | 2 +- call/video_send_stream.cc | 2 +- call/video_send_stream.h | 3 ++- media/base/mediaconfig.h | 13 ++++++++++++- media/engine/webrtcvideoengine.cc | 1 + media/engine/webrtcvoiceengine.cc | 6 +++++- media/engine/webrtcvoiceengine.h | 2 ++ video/video_send_stream_impl.cc | 2 +- 23 files changed, 72 insertions(+), 53 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 044a48039c..3f0176010d 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -340,6 +340,22 @@ class PeerConnectionInterface : public rtc::RefCountInterface { media_config.video.experiment_cpu_load_estimator = enable; } + int audio_rtcp_report_interval_ms() const { + return media_config.audio.rtcp_report_interval_ms; + } + void set_audio_rtcp_report_interval_ms(int audio_rtcp_report_interval_ms) { + media_config.audio.rtcp_report_interval_ms = + audio_rtcp_report_interval_ms; + } + + int video_rtcp_report_interval_ms() const { + return media_config.video.rtcp_report_interval_ms; + } + void set_video_rtcp_report_interval_ms(int video_rtcp_report_interval_ms) { + media_config.video.rtcp_report_interval_ms = + video_rtcp_report_interval_ms; + } + static const int kUndefined = -1; // Default maximum number of packets in the audio jitter buffer. static const int kAudioJitterBufferMaxPackets = 50; diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 37f89c5546..7824653434 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -66,11 +66,13 @@ std::unique_ptr CreateChannelAndProxy( RtcEventLog* event_log, FrameEncryptorInterface* frame_encryptor, const webrtc::CryptoOptions& crypto_options, - bool extmap_allow_mixed) { + bool extmap_allow_mixed, + int rtcp_report_interval_ms) { return absl::make_unique( absl::make_unique( worker_queue, module_process_thread, media_transport, rtcp_rtt_stats, - event_log, frame_encryptor, crypto_options, extmap_allow_mixed)); + event_log, frame_encryptor, crypto_options, extmap_allow_mixed, + rtcp_report_interval_ms)); } void UpdateEventLogStreamConfig(RtcEventLog* event_log, @@ -157,7 +159,8 @@ AudioSendStream::AudioSendStream( event_log, config.frame_encryptor, config.crypto_options, - config.rtp.extmap_allow_mixed)) {} + config.rtp.extmap_allow_mixed, + config.rtcp_report_interval_ms)) {} AudioSendStream::AudioSendStream( const webrtc::AudioSendStream::Config& config, diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index 6a9232998b..ed94283ac3 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -334,11 +334,12 @@ TEST(AudioSendStreamTest, ConfigToString) { config.rtp.extmap_allow_mixed = true; config.rtp.extensions.push_back( RtpExtension(RtpExtension::kAudioLevelUri, kAudioLevelId)); + config.rtcp_report_interval_ms = 2500; EXPECT_EQ( "{rtp: {ssrc: 1234, extmap-allow-mixed: true, extensions: [{uri: " "urn:ietf:params:rtp-hdrext:ssrc-audio-level, id: 2}], nack: " - "{rtp_history_ms: 0}, c_name: foo_name}, send_transport: null, " - "media_transport: null, " + "{rtp_history_ms: 0}, c_name: foo_name}, rtcp_report_interval_ms: 2500, " + "send_transport: null, media_transport: null, " "min_bitrate_bps: 12000, max_bitrate_bps: 34000, " "send_codec_spec: {nack_enabled: true, transport_cc_enabled: false, " "cng_payload_type: 42, payload_type: 103, " diff --git a/audio/channel_send.cc b/audio/channel_send.cc index c0de939c5c..a4a4977441 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -454,7 +454,8 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, RtcEventLog* rtc_event_log, FrameEncryptorInterface* frame_encryptor, const webrtc::CryptoOptions& crypto_options, - bool extmap_allow_mixed) + bool extmap_allow_mixed, + int rtcp_report_interval_ms) : event_log_(rtc_event_log), _timeStamp(0), // This is just an offset, RTP module will add it's own // random offset @@ -498,6 +499,8 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, configuration.retransmission_rate_limiter = retransmission_rate_limiter_.get(); configuration.extmap_allow_mixed = extmap_allow_mixed; + configuration.rtcp_interval_config.audio_interval_ms = + rtcp_report_interval_ms; _rtpRtcpModule.reset(RtpRtcp::CreateRtpRtcp(configuration)); _rtpRtcpModule->SetSendingMediaStatus(false); diff --git a/audio/channel_send.h b/audio/channel_send.h index 407303f54f..6013d1e8c4 100644 --- a/audio/channel_send.h +++ b/audio/channel_send.h @@ -125,7 +125,8 @@ class ChannelSend RtcEventLog* rtc_event_log, FrameEncryptorInterface* frame_encryptor, const webrtc::CryptoOptions& crypto_options, - bool extmap_allow_mixed); + bool extmap_allow_mixed, + int rtcp_report_interval_ms); virtual ~ChannelSend(); diff --git a/call/audio_send_stream.cc b/call/audio_send_stream.cc index 372126858f..7fc6fa30a2 100644 --- a/call/audio_send_stream.cc +++ b/call/audio_send_stream.cc @@ -34,6 +34,7 @@ std::string AudioSendStream::Config::ToString() const { char buf[1024]; rtc::SimpleStringBuilder ss(buf); ss << "{rtp: " << rtp.ToString(); + ss << ", rtcp_report_interval_ms: " << rtcp_report_interval_ms; ss << ", send_transport: " << (send_transport ? "(Transport)" : "null"); ss << ", media_transport: " << (media_transport ? "(Transport)" : "null"); ss << ", min_bitrate_bps: " << min_bitrate_bps; diff --git a/call/audio_send_stream.h b/call/audio_send_stream.h index ba0d652fdf..0cd0894c13 100644 --- a/call/audio_send_stream.h +++ b/call/audio_send_stream.h @@ -96,6 +96,9 @@ class AudioSendStream { std::string c_name; } rtp; + // Time interval between RTCP report for audio + int rtcp_report_interval_ms = 5000; + // Transport for outgoing packets. The transport is expected to exist for // the entire life of the AudioSendStream and is owned by the API client. Transport* send_transport = nullptr; diff --git a/call/rtp_config.cc b/call/rtp_config.cc index 30631f59cb..cce78ad64c 100644 --- a/call/rtp_config.cc +++ b/call/rtp_config.cc @@ -112,18 +112,4 @@ std::string RtpConfig::Rtx::ToString() const { ss << '}'; return ss.str(); } - -RtcpConfig::RtcpConfig() = default; -RtcpConfig::RtcpConfig(const RtcpConfig&) = default; -RtcpConfig::~RtcpConfig() = default; - -std::string RtcpConfig::ToString() const { - char buf[1024]; - rtc::SimpleStringBuilder ss(buf); - ss << "{video_report_interval_ms: " << video_report_interval_ms; - ss << ", audio_report_interval_ms: " << audio_report_interval_ms; - ss << '}'; - return ss.str(); -} - } // namespace webrtc diff --git a/call/rtp_config.h b/call/rtp_config.h index 5d2218297f..97ae951765 100644 --- a/call/rtp_config.h +++ b/call/rtp_config.h @@ -134,17 +134,5 @@ struct RtpConfig { // RTCP CNAME, see RFC 3550. std::string c_name; }; - -struct RtcpConfig { - RtcpConfig(); - RtcpConfig(const RtcpConfig&); - ~RtcpConfig(); - std::string ToString() const; - - // Time interval between RTCP report for video - int64_t video_report_interval_ms = 1000; - // Time interval between RTCP report for audio - int64_t audio_report_interval_ms = 5000; -}; } // namespace webrtc #endif // CALL_RTP_CONFIG_H_ diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index f225bed59c..5265c95afb 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -85,15 +85,15 @@ RtpVideoSenderInterface* RtpTransportControllerSend::CreateRtpVideoSender( std::map suspended_ssrcs, const std::map& states, const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, const RtpSenderObservers& observers, RtcEventLog* event_log, std::unique_ptr fec_controller, const RtpSenderFrameEncryptionConfig& frame_encryption_config) { video_rtp_senders_.push_back(absl::make_unique( - ssrcs, suspended_ssrcs, states, rtp_config, rtcp_config, send_transport, - observers, + ssrcs, suspended_ssrcs, states, rtp_config, rtcp_report_interval_ms, + send_transport, observers, // TODO(holmer): Remove this circular dependency by injecting // the parts of RtpTransportControllerSendInterface that are really used. this, event_log, &retransmission_rate_limiter_, std::move(fec_controller), diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index c6b23629a7..d62cfb6120 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -54,7 +54,7 @@ class RtpTransportControllerSend final const std::map& states, // move states into RtpTransportControllerSend const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, const RtpSenderObservers& observers, RtcEventLog* event_log, diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index e93e6caf32..219c36de35 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -104,7 +104,7 @@ class RtpTransportControllerSendInterface { // TODO(holmer): Move states into RtpTransportControllerSend. const std::map& states, const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, const RtpSenderObservers& observers, RtcEventLog* event_log, diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 214fe96603..c93f93da30 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -41,7 +41,7 @@ static const size_t kPathMTU = 1500; std::vector> CreateRtpRtcpModules( const std::vector& ssrcs, const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, RtcpIntraFrameObserver* intra_frame_callback, RtcpBandwidthObserver* bandwidth_callback, @@ -82,14 +82,12 @@ std::vector> CreateRtpRtcpModules( configuration.retransmission_rate_limiter = retransmission_rate_limiter; configuration.overhead_observer = overhead_observer; configuration.keepalive_config = keepalive_config; - configuration.rtcp_interval_config.video_interval_ms = - rtcp_config.video_report_interval_ms; - configuration.rtcp_interval_config.audio_interval_ms = - rtcp_config.audio_report_interval_ms; configuration.frame_encryptor = frame_encryptor; configuration.require_frame_encryption = crypto_options.sframe.require_frame_encryption; configuration.extmap_allow_mixed = rtp_config.extmap_allow_mixed; + configuration.rtcp_interval_config.video_interval_ms = + rtcp_report_interval_ms; std::vector> modules; const std::vector& flexfec_protected_ssrcs = @@ -186,7 +184,7 @@ RtpVideoSender::RtpVideoSender( std::map suspended_ssrcs, const std::map& states, const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, const RtpSenderObservers& observers, RtpTransportControllerSendInterface* transport, @@ -204,7 +202,7 @@ RtpVideoSender::RtpVideoSender( fec_controller_(std::move(fec_controller)), rtp_modules_(CreateRtpRtcpModules(ssrcs, rtp_config, - rtcp_config, + rtcp_report_interval_ms, send_transport, observers.intra_frame_callback, transport->GetBandwidthObserver(), diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index 91e5fd67cb..d0965fd351 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -54,7 +54,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, std::map suspended_ssrcs, const std::map& states, const RtpConfig& rtp_config, - const RtcpConfig& rtcp_config, + int rtcp_report_interval_ms, Transport* send_transport, const RtpSenderObservers& observers, RtpTransportControllerSendInterface* transport, diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 00b61fe31c..8ab89d6104 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -102,7 +102,7 @@ class RtpVideoSenderTestFixture { std::map suspended_ssrcs; router_ = absl::make_unique( config_.rtp.ssrcs, suspended_ssrcs, suspended_payload_states, - config_.rtp, config_.rtcp, &transport_, + config_.rtp, config_.rtcp_report_interval_ms, &transport_, CreateObservers(&call_stats_, &encoder_feedback_, &stats_proxy_, &stats_proxy_, &stats_proxy_, &stats_proxy_, &stats_proxy_, &stats_proxy_, &send_delay_stats_), diff --git a/call/test/mock_rtp_transport_controller_send.h b/call/test/mock_rtp_transport_controller_send.h index 621447dae5..4d34d06e05 100644 --- a/call/test/mock_rtp_transport_controller_send.h +++ b/call/test/mock_rtp_transport_controller_send.h @@ -38,7 +38,7 @@ class MockRtpTransportControllerSend std::map, const std::map&, const RtpConfig&, - const RtcpConfig&, + int rtcp_report_interval_ms, Transport*, const RtpSenderObservers&, RtcEventLog*, diff --git a/call/video_send_stream.cc b/call/video_send_stream.cc index 08bede5ded..c8ba308fa5 100644 --- a/call/video_send_stream.cc +++ b/call/video_send_stream.cc @@ -79,7 +79,7 @@ std::string VideoSendStream::Config::ToString() const { ss << "{encoder_settings: { experiment_cpu_load_estimator: " << (encoder_settings.experiment_cpu_load_estimator ? "on" : "off") << "}}"; ss << ", rtp: " << rtp.ToString(); - ss << ", rtcp: " << rtcp.ToString(); + ss << ", rtcp_report_interval_ms: " << rtcp_report_interval_ms; ss << ", pre_encode_callback: " << (pre_encode_callback ? "(VideoSinkInterface)" : "nullptr"); ss << ", render_delay_ms: " << render_delay_ms; diff --git a/call/video_send_stream.h b/call/video_send_stream.h index 162622e5f5..e91882c18e 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -113,7 +113,8 @@ class VideoSendStream { RtpConfig rtp; - RtcpConfig rtcp; + // Time interval between RTCP report for video + int rtcp_report_interval_ms = 1000; // Transport for outgoing packets. Transport* send_transport = nullptr; diff --git a/media/base/mediaconfig.h b/media/base/mediaconfig.h index eda387e319..c8fc5212a0 100644 --- a/media/base/mediaconfig.h +++ b/media/base/mediaconfig.h @@ -59,8 +59,17 @@ struct MediaConfig { // TODO(bugs.webrtc.org/8504): If all goes well, the flag will be removed // together with the old method of estimation. bool experiment_cpu_load_estimator = false; + + // Time interval between RTCP report for video + int rtcp_report_interval_ms = 1000; } video; + // Audio-specific config. + struct Audio { + // Time interval between RTCP report for audio + int rtcp_report_interval_ms = 5000; + } audio; + bool operator==(const MediaConfig& o) const { return enable_dscp == o.enable_dscp && video.enable_cpu_adaptation == o.video.enable_cpu_adaptation && @@ -71,7 +80,9 @@ struct MediaConfig { video.periodic_alr_bandwidth_probing == o.video.periodic_alr_bandwidth_probing && video.experiment_cpu_load_estimator == - o.video.experiment_cpu_load_estimator; + o.video.experiment_cpu_load_estimator && + video.rtcp_report_interval_ms == o.video.rtcp_report_interval_ms && + audio.rtcp_report_interval_ms == o.audio.rtcp_report_interval_ms; } bool operator!=(const MediaConfig& o) const { return !(*this == o); } diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc index c796b4f292..74aa55be00 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -1082,6 +1082,7 @@ bool WebRtcVideoChannel::AddSendStream(const StreamParams& sp) { bitrate_allocator_factory_; config.crypto_options = crypto_options_; config.rtp.extmap_allow_mixed = ExtmapAllowMixed(); + config.rtcp_report_interval_ms = video_config_.rtcp_report_interval_ms; WebRtcVideoSendStream* stream = new WebRtcVideoSendStream( call_, sp, std::move(config), default_send_options_, diff --git a/media/engine/webrtcvoiceengine.cc b/media/engine/webrtcvoiceengine.cc index b753a4253e..eaf8746aee 100644 --- a/media/engine/webrtcvoiceengine.cc +++ b/media/engine/webrtcvoiceengine.cc @@ -708,6 +708,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream bool extmap_allow_mixed, const std::vector& extensions, int max_send_bitrate_bps, + int rtcp_report_interval_ms, const absl::optional& audio_network_adaptor_config, webrtc::Call* call, webrtc::Transport* send_transport, @@ -737,6 +738,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream config_.track_id = track_id; config_.frame_encryptor = frame_encryptor; config_.crypto_options = crypto_options; + config_.rtcp_report_interval_ms = rtcp_report_interval_ms; rtp_parameters_.encodings[0].ssrc = ssrc; rtp_parameters_.rtcp.cname = c_name; rtp_parameters_.header_extensions = extensions; @@ -1258,6 +1260,7 @@ WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel( : VoiceMediaChannel(config), engine_(engine), call_(call), + audio_config_(config.audio), crypto_options_(crypto_options) { RTC_LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel"; RTC_DCHECK(call); @@ -1810,7 +1813,8 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) { GetAudioNetworkAdaptorConfig(options_); WebRtcAudioSendStream* stream = new WebRtcAudioSendStream( ssrc, mid_, sp.cname, sp.id, send_codec_spec_, ExtmapAllowMixed(), - send_rtp_extensions_, max_send_bitrate_bps_, audio_network_adaptor_config, + send_rtp_extensions_, max_send_bitrate_bps_, + audio_config_.rtcp_report_interval_ms, audio_network_adaptor_config, call_, this, media_transport(), engine()->encoder_factory_, codec_pair_id_, nullptr, crypto_options_); send_streams_.insert(std::make_pair(ssrc, stream)); diff --git a/media/engine/webrtcvoiceengine.h b/media/engine/webrtcvoiceengine.h index d8eff150ec..7a5343e55d 100644 --- a/media/engine/webrtcvoiceengine.h +++ b/media/engine/webrtcvoiceengine.h @@ -276,6 +276,8 @@ class WebRtcVoiceMediaChannel final : public VoiceMediaChannel, bool send_ = false; webrtc::Call* const call_ = nullptr; + const MediaConfig::Audio audio_config_; + // Queue of unsignaled SSRCs; oldest at the beginning. std::vector unsignaled_recv_ssrcs_; diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index e6d97e98bb..1cb4dbc6f2 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -251,7 +251,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( suspended_ssrcs, suspended_payload_states, config_->rtp, - config_->rtcp, + config_->rtcp_report_interval_ms, config_->send_transport, CreateObservers(call_stats, &encoder_feedback_,