Fix minor regression caused by a8336d3
VideoEncoder::SetRates was being called unnessesarily when the fields
appended to RateControlParameters were changed. Since SetRates only
cares about RateControlParameters, it should have only been called if
the RateControlParameters themselves were actually changed.
Bug: webrtc:10126
Change-Id: Ic47d67e642a3043307fec950e5fba970d9f95167
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/152829
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@google.com>
Cr-Commit-Position: refs/heads/master@{#29208}
diff --git a/api/video_codecs/video_encoder.cc b/api/video_codecs/video_encoder.cc
index d3f16a0..3a848f3 100644
--- a/api/video_codecs/video_encoder.cc
+++ b/api/video_codecs/video_encoder.cc
@@ -116,6 +116,17 @@
framerate_fps(framerate_fps),
bandwidth_allocation(bandwidth_allocation) {}
+bool VideoEncoder::RateControlParameters::operator==(
+ const VideoEncoder::RateControlParameters& rhs) const {
+ return std::tie(bitrate, framerate_fps, bandwidth_allocation) ==
+ std::tie(rhs.bitrate, rhs.framerate_fps, rhs.bandwidth_allocation);
+}
+
+bool VideoEncoder::RateControlParameters::operator!=(
+ const VideoEncoder::RateControlParameters& rhs) const {
+ return !(rhs == *this);
+}
+
VideoEncoder::RateControlParameters::~RateControlParameters() = default;
void VideoEncoder::SetFecControllerOverride(
diff --git a/api/video_codecs/video_encoder.h b/api/video_codecs/video_encoder.h
index 0ee5521..766ea75 100644
--- a/api/video_codecs/video_encoder.h
+++ b/api/video_codecs/video_encoder.h
@@ -239,6 +239,9 @@
// |bitrate.get_sum_bps()|, but may be higher if the application is not
// network constrained.
DataRate bandwidth_allocation;
+
+ bool operator==(const RateControlParameters& rhs) const;
+ bool operator!=(const RateControlParameters& rhs) const;
};
struct LossNotification {
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 8b576e8..9257f93 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -441,7 +441,7 @@
};
VideoStreamEncoder::EncoderRateSettings::EncoderRateSettings()
- : VideoEncoder::RateControlParameters(),
+ : rate_control(),
encoder_target(DataRate::Zero()),
stable_encoder_target(DataRate::Zero()) {}
@@ -451,16 +451,13 @@
DataRate bandwidth_allocation,
DataRate encoder_target,
DataRate stable_encoder_target)
- : VideoEncoder::RateControlParameters(bitrate,
- framerate_fps,
- bandwidth_allocation),
+ : rate_control(bitrate, framerate_fps, bandwidth_allocation),
encoder_target(encoder_target),
stable_encoder_target(stable_encoder_target) {}
bool VideoStreamEncoder::EncoderRateSettings::operator==(
const EncoderRateSettings& rhs) const {
- return bitrate == rhs.bitrate && framerate_fps == rhs.framerate_fps &&
- bandwidth_allocation == rhs.bandwidth_allocation &&
+ return rate_control == rhs.rate_control &&
encoder_target == rhs.encoder_target &&
stable_encoder_target == rhs.stable_encoder_target;
}
@@ -948,7 +945,8 @@
if (rate_allocator_ && last_encoder_rate_settings_) {
// We have a new rate allocator instance and already configured target
// bitrate. Update the rate allocation and notify observers.
- last_encoder_rate_settings_->framerate_fps = GetInputFramerateFps();
+ last_encoder_rate_settings_->rate_control.framerate_fps =
+ GetInputFramerateFps();
SetEncoderRates(
UpdateBitrateAllocationAndNotifyObserver(*last_encoder_rate_settings_));
}
@@ -1149,7 +1147,7 @@
if (rate_allocator_ && rate_settings.encoder_target > DataRate::Zero()) {
new_allocation = rate_allocator_->Allocate(VideoBitrateAllocationParameters(
rate_settings.encoder_target, rate_settings.stable_encoder_target,
- rate_settings.framerate_fps));
+ rate_settings.rate_control.framerate_fps));
}
if (bitrate_observer_ && new_allocation.get_sum_bps() > 0) {
@@ -1170,27 +1168,27 @@
}
EncoderRateSettings new_rate_settings = rate_settings;
- new_rate_settings.bitrate = new_allocation;
+ new_rate_settings.rate_control.bitrate = new_allocation;
// VideoBitrateAllocator subclasses may allocate a bitrate higher than the
// target in order to sustain the min bitrate of the video codec. In this
// case, make sure the bandwidth allocation is at least equal the allocation
// as that is part of the document contract for that field.
- new_rate_settings.bandwidth_allocation =
- std::max(new_rate_settings.bandwidth_allocation,
- DataRate::bps(new_rate_settings.bitrate.get_sum_bps()));
+ new_rate_settings.rate_control.bandwidth_allocation = std::max(
+ new_rate_settings.rate_control.bandwidth_allocation,
+ DataRate::bps(new_rate_settings.rate_control.bitrate.get_sum_bps()));
if (bitrate_adjuster_) {
VideoBitrateAllocation adjusted_allocation =
- bitrate_adjuster_->AdjustRateAllocation(new_rate_settings);
+ bitrate_adjuster_->AdjustRateAllocation(new_rate_settings.rate_control);
RTC_LOG(LS_VERBOSE) << "Adjusting allocation, fps = "
- << rate_settings.framerate_fps << ", from "
+ << rate_settings.rate_control.framerate_fps << ", from "
<< new_allocation.ToString() << ", to "
<< adjusted_allocation.ToString();
- new_rate_settings.bitrate = adjusted_allocation;
+ new_rate_settings.rate_control.bitrate = adjusted_allocation;
}
encoder_stats_observer_->OnBitrateAllocationUpdated(
- send_codec_, new_rate_settings.bitrate);
+ send_codec_, new_rate_settings.rate_control.bitrate);
return new_rate_settings;
}
@@ -1207,10 +1205,11 @@
void VideoStreamEncoder::SetEncoderRates(
const EncoderRateSettings& rate_settings) {
- RTC_DCHECK_GT(rate_settings.framerate_fps, 0.0);
- const bool settings_changes = !last_encoder_rate_settings_ ||
- rate_settings != *last_encoder_rate_settings_;
- if (settings_changes) {
+ RTC_DCHECK_GT(rate_settings.rate_control.framerate_fps, 0.0);
+ bool rate_control_changed =
+ (!last_encoder_rate_settings_.has_value() ||
+ last_encoder_rate_settings_->rate_control != rate_settings.rate_control);
+ if (last_encoder_rate_settings_ != rate_settings) {
last_encoder_rate_settings_ = rate_settings;
}
@@ -1226,15 +1225,16 @@
// bitrate.
// TODO(perkj): Make sure all known encoder implementations handle zero
// target bitrate and remove this check.
- if (!HasInternalSource() && rate_settings.bitrate.get_sum_bps() == 0) {
+ if (!HasInternalSource() &&
+ rate_settings.rate_control.bitrate.get_sum_bps() == 0) {
return;
}
- if (settings_changes) {
- encoder_->SetRates(rate_settings);
+ if (rate_control_changed) {
+ encoder_->SetRates(rate_settings.rate_control);
frame_encode_metadata_writer_.OnSetRates(
- rate_settings.bitrate,
- static_cast<uint32_t>(rate_settings.framerate_fps + 0.5));
+ rate_settings.rate_control.bitrate,
+ static_cast<uint32_t>(rate_settings.rate_control.framerate_fps + 0.5));
}
}
@@ -1283,7 +1283,8 @@
// |last_encoder_rate_setings_|, triggering the call to SetRate() on the
// encoder.
EncoderRateSettings new_rate_settings = *last_encoder_rate_settings_;
- new_rate_settings.framerate_fps = static_cast<double>(framerate_fps);
+ new_rate_settings.rate_control.framerate_fps =
+ static_cast<double>(framerate_fps);
SetEncoderRates(
UpdateBitrateAllocationAndNotifyObserver(new_rate_settings));
}
diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h
index 4db92f5..46df362 100644
--- a/video/video_stream_encoder.h
+++ b/video/video_stream_encoder.h
@@ -121,7 +121,7 @@
int pixel_count() const { return width * height; }
};
- struct EncoderRateSettings : public VideoEncoder::RateControlParameters {
+ struct EncoderRateSettings {
EncoderRateSettings();
EncoderRateSettings(const VideoBitrateAllocation& bitrate,
double framerate_fps,
@@ -131,6 +131,7 @@
bool operator==(const EncoderRateSettings& rhs) const;
bool operator!=(const EncoderRateSettings& rhs) const;
+ VideoEncoder::RateControlParameters rate_control;
// This is the scalar target bitrate before the VideoBitrateAllocator, i.e.
// the |target_bitrate| argument of the OnBitrateUpdated() method. This is
// needed because the bitrate allocator may truncate the total bitrate and a
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 6f19edc..6eba930 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -782,6 +782,11 @@
return num_encoder_initializations_;
}
+ int GetNumSetRates() const {
+ rtc::CritScope lock(&local_crit_sect_);
+ return num_set_rates_;
+ }
+
private:
int32_t Encode(const VideoFrame& input_image,
const std::vector<VideoFrameType>* frame_types) override {
@@ -848,6 +853,7 @@
void SetRates(const RateControlParameters& parameters) {
rtc::CritScope lock(&local_crit_sect_);
+ num_set_rates_++;
VideoBitrateAllocation adjusted_rate_allocation;
for (size_t si = 0; si < kMaxSpatialLayers; ++si) {
for (size_t ti = 0; ti < kMaxTemporalStreams; ++ti) {
@@ -901,6 +907,7 @@
int num_encoder_initializations_ RTC_GUARDED_BY(local_crit_sect_) = 0;
std::vector<ResolutionBitrateLimits> resolution_bitrate_limits_
RTC_GUARDED_BY(local_crit_sect_);
+ int num_set_rates_ RTC_GUARDED_BY(local_crit_sect_) = 0;
};
class TestSink : public VideoStreamEncoder::EncoderSink {
@@ -4875,4 +4882,75 @@
video_stream_encoder_->Stop();
}
+TEST_F(VideoStreamEncoderTest,
+ AllocationPropegratedToEncoderWhenTargetRateChanged) {
+ const int kFrameWidth = 320;
+ const int kFrameHeight = 180;
+
+ // Set initial rate.
+ auto rate = DataRate::kbps(100);
+ video_stream_encoder_->OnBitrateUpdated(
+ /*target_bitrate=*/rate,
+ /*stable_target_bitrate=*/rate,
+ /*link_allocation=*/rate,
+ /*fraction_lost=*/0,
+ /*rtt_ms=*/0);
+
+ // Insert a first video frame so that encoder gets configured.
+ int64_t timestamp_ms = fake_clock_.TimeNanos() / rtc::kNumNanosecsPerMillisec;
+ VideoFrame frame = CreateFrame(timestamp_ms, kFrameWidth, kFrameHeight);
+ frame.set_rotation(kVideoRotation_270);
+ video_source_.IncomingCapturedFrame(frame);
+ WaitForEncodedFrame(timestamp_ms);
+ EXPECT_EQ(1, fake_encoder_.GetNumSetRates());
+
+ // Change of target bitrate propagates to the encoder.
+ auto new_stable_rate = rate - DataRate::kbps(5);
+ video_stream_encoder_->OnBitrateUpdated(
+ /*target_bitrate=*/new_stable_rate,
+ /*stable_target_bitrate=*/new_stable_rate,
+ /*link_allocation=*/rate,
+ /*fraction_lost=*/0,
+ /*rtt_ms=*/0);
+ video_stream_encoder_->WaitUntilTaskQueueIsIdle();
+ EXPECT_EQ(2, fake_encoder_.GetNumSetRates());
+ video_stream_encoder_->Stop();
+}
+
+TEST_F(VideoStreamEncoderTest,
+ AllocationNotPropegratedToEncoderWhenTargetRateUnchanged) {
+ const int kFrameWidth = 320;
+ const int kFrameHeight = 180;
+
+ // Set initial rate.
+ auto rate = DataRate::kbps(100);
+ video_stream_encoder_->OnBitrateUpdated(
+ /*target_bitrate=*/rate,
+ /*stable_target_bitrate=*/rate,
+ /*link_allocation=*/rate,
+ /*fraction_lost=*/0,
+ /*rtt_ms=*/0);
+
+ // Insert a first video frame so that encoder gets configured.
+ int64_t timestamp_ms = fake_clock_.TimeNanos() / rtc::kNumNanosecsPerMillisec;
+ VideoFrame frame = CreateFrame(timestamp_ms, kFrameWidth, kFrameHeight);
+ frame.set_rotation(kVideoRotation_270);
+ video_source_.IncomingCapturedFrame(frame);
+ WaitForEncodedFrame(timestamp_ms);
+ EXPECT_EQ(1, fake_encoder_.GetNumSetRates());
+
+ // Set a higher target rate without changing the link_allocation. Should not
+ // reset encoder's rate.
+ auto new_stable_rate = rate - DataRate::kbps(5);
+ video_stream_encoder_->OnBitrateUpdated(
+ /*target_bitrate=*/rate,
+ /*stable_target_bitrate=*/new_stable_rate,
+ /*link_allocation=*/rate,
+ /*fraction_lost=*/0,
+ /*rtt_ms=*/0);
+ video_stream_encoder_->WaitUntilTaskQueueIsIdle();
+ EXPECT_EQ(1, fake_encoder_.GetNumSetRates());
+ video_stream_encoder_->Stop();
+}
+
} // namespace webrtc