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}
diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc
index 98230b1..c489ed7 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 @@
}
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 e2dce3f..cce0795 100644
--- a/webrtc/video/rampup_tests.cc
+++ b/webrtc/video/rampup_tests.cc
@@ -273,10 +273,8 @@
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 fd0906d..289f735 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -196,8 +196,8 @@
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 @@
// to send on all SSRCs at once etc.)
std::vector<uint32_t> used_ssrcs = config_.rtp.ssrcs;
used_ssrcs.resize(static_cast<size_t>(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 fad5843..e09b896 100644
--- a/webrtc/video_engine/vie_encoder.cc
+++ b/webrtc/video_engine/vie_encoder.cc
@@ -122,15 +122,13 @@
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 @@
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 @@
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 @@
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 @@
// encoding.
return;
}
+ VideoCodecType codec_type;
{
CriticalSectionScoped cs(data_cs_.get());
time_of_last_frame_activity_ms_ = TickTime::MillisecondTimestamp();
@@ -385,6 +379,7 @@
return;
}
TraceFrameDropEnd();
+ codec_type = encoder_config_.codecType;
}
TRACE_EVENT_ASYNC_STEP0("webrtc", "Video", video_frame.render_time_ms(),
@@ -421,7 +416,7 @@
(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 @@
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<uint32_t>(max_payload_length)) !=
- 0) {
- return -1;
- }
- }
- }
- return 0;
}
void ViEEncoder::SetSenderBufferingMode(int target_delay_ms) {
@@ -619,16 +584,7 @@
time_last_intra_request_ms_[new_ssrc] = last_intra_request_ms;
}
-bool ViEEncoder::SetSsrcs(const std::vector<uint32_t>& 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<uint32_t>& ssrcs) {
CriticalSectionScoped cs(data_cs_.get());
ssrc_streams_.clear();
time_last_intra_request_ms_.clear();
@@ -636,7 +592,6 @@
for (uint32_t ssrc : ssrcs) {
ssrc_streams_[ssrc] = idx++;
}
- return true;
}
void ViEEncoder::SetMinTransmitBitrate(int min_transmit_bitrate_kbps) {
@@ -655,28 +610,29 @@
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<uint32_t> 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 66ac4a5..6421504 100644
--- a/webrtc/video_engine/vie_encoder.h
+++ b/webrtc/video_engine/vie_encoder.h
@@ -87,7 +87,6 @@
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 @@
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 @@
void OnLocalSsrcChanged(uint32_t old_ssrc, uint32_t new_ssrc) override;
// Sets SSRCs for all streams.
- bool SetSsrcs(const std::vector<uint32_t>& ssrcs);
+ void SetSsrcs(const std::vector<uint32_t>& ssrcs);
void SetMinTransmitBitrate(int min_transmit_bitrate_kbps);
@@ -171,7 +172,7 @@
// 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 @@
std::map<unsigned int, int64_t> 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_);
