From d153a37801d9b1258ffff9f878064fa0ddfd3b0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Tue, 10 Nov 2015 15:27:12 +0000 Subject: [PATCH] Remove contention between RTCP packets and encoding. Receiving RTCP often caused the worker thread to stall for >20 ms (>100ms observed) due to contention on VideoSender's send_crit_ (used to protect encoding). This change removes an unnecessary acquire of send_crit_ and caches encoder settings in ViEEncoder instead of acquiring them through ::SendCodec() in VCM (which is blocking). BUG=webrtc:5106 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/1433703002 . Cr-Commit-Position: refs/heads/master@{#10582} --- .../video_coding/main/source/video_sender.cc | 1 - webrtc/video/rampup_tests.cc | 6 +- webrtc/video/video_send_stream.cc | 10 +- webrtc/video_engine/vie_encoder.cc | 104 +++++------------- webrtc/video_engine/vie_encoder.h | 16 ++- 5 files changed, 41 insertions(+), 96 deletions(-) diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc index 98230b1e9e..c489ed7112 100644 --- a/webrtc/modules/video_coding/main/source/video_sender.cc +++ b/webrtc/modules/video_coding/main/source/video_sender.cc @@ -369,7 +369,6 @@ void VideoSender::SuspendBelowMinBitrate() { } bool VideoSender::VideoSuspended() const { - rtc::CritScope lock(&send_crit_); return _mediaOpt.IsVideoSuspended(); } } // namespace vcm diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc index e2dce3f3f8..cce0795b11 100644 --- a/webrtc/video/rampup_tests.cc +++ b/webrtc/video/rampup_tests.cc @@ -273,10 +273,8 @@ void RampUpTester::TriggerTestDone() { void RampUpTester::PerformTest() { test_start_ms_ = clock_->TimeInMilliseconds(); poller_thread_->Start(); - if (Wait() != kEventSignaled) { - printf("Timed out while waiting for ramp-up to complete."); - return; - } + EXPECT_EQ(kEventSignaled, Wait()) + << "Timed out while waiting for ramp-up to complete."; TriggerTestDone(); poller_thread_->Stop(); } diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index fd0906d657..289f735151 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -196,8 +196,8 @@ VideoSendStream::VideoSendStream( vie_channel_->SetProtectionMode(enable_protection_nack, enable_protection_fec, config_.rtp.fec.red_payload_type, config_.rtp.fec.ulpfec_payload_type); - vie_encoder_->UpdateProtectionMethod(enable_protection_nack, - enable_protection_fec); + vie_encoder_->SetProtectionMethod(enable_protection_nack, + enable_protection_fec); ConfigureSsrcs(); @@ -563,14 +563,8 @@ bool VideoSendStream::SetSendCodec(VideoCodec video_codec) { // to send on all SSRCs at once etc.) std::vector used_ssrcs = config_.rtp.ssrcs; used_ssrcs.resize(static_cast(video_codec.numberOfSimulcastStreams)); - - // Update used SSRCs. vie_encoder_->SetSsrcs(used_ssrcs); - // Update the protection mode, we might be switching NACK/FEC. - vie_encoder_->UpdateProtectionMethod(vie_encoder_->nack_enabled(), - vie_channel_->IsSendingFecEnabled()); - // Restart the media flow vie_encoder_->Restart(); diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index fad58439af..e09b89627c 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -122,15 +122,13 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores, pacer_(pacer), bitrate_allocator_(bitrate_allocator), time_of_last_frame_activity_ms_(0), - simulcast_enabled_(false), + encoder_config_(), min_transmit_bitrate_kbps_(0), last_observed_bitrate_bps_(0), target_delay_ms_(0), network_is_transmitting_(true), encoder_paused_(false), encoder_paused_and_dropped_frame_(false), - fec_enabled_(false), - nack_enabled_(false), module_process_thread_(module_process_thread), has_received_sli_(false), picture_id_sli_(0), @@ -231,9 +229,11 @@ int32_t ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec) { return -1; } + // Cache codec before calling AddBitrateObserver (which calls OnNetworkChanged + // that makes use of the number of simulcast streams configured). { CriticalSectionScoped cs(data_cs_.get()); - simulcast_enabled_ = video_codec.numberOfSimulcastStreams > 1; + encoder_config_ = video_codec; } // Add a bitrate observer to the allocator and update the start, max and @@ -254,11 +254,6 @@ int32_t ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec) { return 0; } -int32_t ViEEncoder::GetEncoder(VideoCodec* video_codec) { - *video_codec = vcm_->GetSendCodec(); - return 0; -} - int32_t ViEEncoder::ScaleInputImage(bool enable) { VideoFrameResampling resampling_mode = kFastRescaling; // TODO(mflodman) What? @@ -276,21 +271,19 @@ int ViEEncoder::GetPaddingNeededBps() const { int64_t time_of_last_frame_activity_ms; int min_transmit_bitrate_bps; int bitrate_bps; + VideoCodec send_codec; { CriticalSectionScoped cs(data_cs_.get()); - bool send_padding = simulcast_enabled_ || video_suspended_ || - min_transmit_bitrate_kbps_ > 0; + bool send_padding = encoder_config_.numberOfSimulcastStreams > 1 || + video_suspended_ || min_transmit_bitrate_kbps_ > 0; if (!send_padding) return 0; time_of_last_frame_activity_ms = time_of_last_frame_activity_ms_; min_transmit_bitrate_bps = 1000 * min_transmit_bitrate_kbps_; bitrate_bps = last_observed_bitrate_bps_; + send_codec = encoder_config_; } - VideoCodec send_codec; - if (vcm_->SendCodec(&send_codec) != 0) - return 0; - bool video_is_suspended = vcm_->VideoSuspended(); // Find the max amount of padding we can allow ourselves to send at this @@ -377,6 +370,7 @@ void ViEEncoder::DeliverFrame(VideoFrame video_frame) { // encoding. return; } + VideoCodecType codec_type; { CriticalSectionScoped cs(data_cs_.get()); time_of_last_frame_activity_ms_ = TickTime::MillisecondTimestamp(); @@ -385,6 +379,7 @@ void ViEEncoder::DeliverFrame(VideoFrame video_frame) { return; } TraceFrameDropEnd(); + codec_type = encoder_config_.codecType; } TRACE_EVENT_ASYNC_STEP0("webrtc", "Video", video_frame.render_time_ms(), @@ -421,7 +416,7 @@ void ViEEncoder::DeliverFrame(VideoFrame video_frame) { (decimated_frame != NULL) ? decimated_frame : &video_frame; #ifdef VIDEOCODEC_VP8 - if (vcm_->SendCodec() == webrtc::kVideoCodecVP8) { + if (codec_type == webrtc::kVideoCodecVP8) { webrtc::CodecSpecificInfo codec_specific_info; codec_specific_info.codecType = webrtc::kVideoCodecVP8; { @@ -461,46 +456,16 @@ int ViEEncoder::CodecTargetBitrate(uint32_t* bitrate) const { return 0; } -int32_t ViEEncoder::UpdateProtectionMethod(bool nack, bool fec) { - RTC_DCHECK(send_payload_router_ != NULL); - - if (fec_enabled_ == fec && nack_enabled_ == nack) { - // No change needed, we're already in correct state. - return 0; - } - fec_enabled_ = fec; - nack_enabled_ = nack; - +void ViEEncoder::SetProtectionMethod(bool nack, bool fec) { // Set Video Protection for VCM. VCMVideoProtection protection_mode; - if (fec_enabled_) { + if (fec) { protection_mode = - nack_enabled_ ? webrtc::kProtectionNackFEC : kProtectionFEC; + nack ? webrtc::kProtectionNackFEC : kProtectionFEC; } else { - protection_mode = nack_enabled_ ? kProtectionNack : kProtectionNone; + protection_mode = nack ? kProtectionNack : kProtectionNone; } vcm_->SetVideoProtection(protection_mode, true); - - if (fec_enabled_ || nack_enabled_) { - // The send codec must be registered to set correct MTU. - webrtc::VideoCodec codec; - if (vcm_->SendCodec(&codec) == 0) { - uint32_t current_bitrate_bps = 0; - if (vcm_->Bitrate(¤t_bitrate_bps) != 0) { - LOG_F(LS_WARNING) << - "Failed to get the current encoder target bitrate."; - } - // Convert to start bitrate in kbps. - codec.startBitrate = (current_bitrate_bps + 500) / 1000; - size_t max_payload_length = send_payload_router_->MaxPayloadLength(); - if (vcm_->RegisterSendCodec(&codec, number_of_cores_, - static_cast(max_payload_length)) != - 0) { - return -1; - } - } - } - return 0; } void ViEEncoder::SetSenderBufferingMode(int target_delay_ms) { @@ -619,16 +584,7 @@ void ViEEncoder::OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) { time_last_intra_request_ms_[new_ssrc] = last_intra_request_ms; } -bool ViEEncoder::SetSsrcs(const std::vector& ssrcs) { - VideoCodec codec; - if (vcm_->SendCodec(&codec) != 0) - return false; - - if (codec.numberOfSimulcastStreams > 0 && - ssrcs.size() != codec.numberOfSimulcastStreams) { - return false; - } - +void ViEEncoder::SetSsrcs(const std::vector& ssrcs) { CriticalSectionScoped cs(data_cs_.get()); ssrc_streams_.clear(); time_last_intra_request_ms_.clear(); @@ -636,7 +592,6 @@ bool ViEEncoder::SetSsrcs(const std::vector& ssrcs) { for (uint32_t ssrc : ssrcs) { ssrc_streams_[ssrc] = idx++; } - return true; } void ViEEncoder::SetMinTransmitBitrate(int min_transmit_bitrate_kbps) { @@ -655,28 +610,29 @@ void ViEEncoder::OnNetworkChanged(uint32_t bitrate_bps, RTC_DCHECK(send_payload_router_ != NULL); vcm_->SetChannelParameters(bitrate_bps, fraction_lost, round_trip_time_ms); bool video_is_suspended = vcm_->VideoSuspended(); - + bool video_suspension_changed; VideoCodec send_codec; - if (vcm_->SendCodec(&send_codec) != 0) { - return; + uint32_t first_ssrc; + { + CriticalSectionScoped cs(data_cs_.get()); + last_observed_bitrate_bps_ = bitrate_bps; + video_suspension_changed = video_suspended_ != video_is_suspended; + video_suspended_ = video_is_suspended; + send_codec = encoder_config_; + first_ssrc = ssrc_streams_.begin()->first; } + SimulcastStream* stream_configs = send_codec.simulcastStream; // Allocate the bandwidth between the streams. std::vector stream_bitrates = AllocateStreamBitrates( bitrate_bps, stream_configs, send_codec.numberOfSimulcastStreams); send_payload_router_->SetTargetSendBitrates(stream_bitrates); - { - CriticalSectionScoped cs(data_cs_.get()); - last_observed_bitrate_bps_ = bitrate_bps; - if (video_suspended_ == video_is_suspended) - return; - video_suspended_ = video_is_suspended; - - LOG(LS_INFO) << "Video suspend state changed " << video_is_suspended - << " for ssrc " << ssrc_streams_.begin()->first; - } + if (!video_suspension_changed) + return; // Video suspend-state changed, inform codec observer. + LOG(LS_INFO) << "Video suspend state changed " << video_is_suspended + << " for ssrc " << first_ssrc; if (stats_proxy_) stats_proxy_->OnSuspendChange(video_is_suspended); } diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index 66ac4a506b..6421504e95 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -87,7 +87,6 @@ class ViEEncoder : public RtcpIntraFrameObserver, bool internal_source); int32_t DeRegisterExternalEncoder(uint8_t pl_type); int32_t SetEncoder(const VideoCodec& video_codec); - int32_t GetEncoder(VideoCodec* video_codec); // Scale or crop/pad image. int32_t ScaleInputImage(bool enable); @@ -99,9 +98,11 @@ class ViEEncoder : public RtcpIntraFrameObserver, uint32_t LastObservedBitrateBps() const; int CodecTargetBitrate(uint32_t* bitrate) const; - // Loss protection. - int32_t UpdateProtectionMethod(bool nack, bool fec); - bool nack_enabled() const { return nack_enabled_; } + // Loss protection. Must be called before SetEncoder() to have max packet size + // updated according to protection. + // TODO(pbos): Set protection method on construction or extract vcm_ outside + // this class and set it on construction there. + void SetProtectionMethod(bool nack, bool fec); // Buffering mode. void SetSenderBufferingMode(int target_delay_ms); @@ -126,7 +127,7 @@ class ViEEncoder : public RtcpIntraFrameObserver, void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) override; // Sets SSRCs for all streams. - bool SetSsrcs(const std::vector& ssrcs); + void SetSsrcs(const std::vector& ssrcs); void SetMinTransmitBitrate(int min_transmit_bitrate_kbps); @@ -171,7 +172,7 @@ class ViEEncoder : public RtcpIntraFrameObserver, // track when video is stopped long enough that we also want to stop sending // padding. int64_t time_of_last_frame_activity_ms_ GUARDED_BY(data_cs_); - bool simulcast_enabled_ GUARDED_BY(data_cs_); + VideoCodec encoder_config_ GUARDED_BY(data_cs_); int min_transmit_bitrate_kbps_ GUARDED_BY(data_cs_); uint32_t last_observed_bitrate_bps_ GUARDED_BY(data_cs_); int target_delay_ms_ GUARDED_BY(data_cs_); @@ -181,9 +182,6 @@ class ViEEncoder : public RtcpIntraFrameObserver, std::map time_last_intra_request_ms_ GUARDED_BY(data_cs_); - bool fec_enabled_; - bool nack_enabled_; - ProcessThread* module_process_thread_; bool has_received_sli_ GUARDED_BY(data_cs_);