Reland "Fix minor regression caused by a8336d3"
This is a reland of 809198edfff416fce8d75b574a43afab5e67b1cd
A fix was made in https://webrtc-review.googlesource.com/c/src/+/154343
which fixed the regression issues caused by the original patch.
Original change's description:
> 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}
Bug: webrtc:10126
Change-Id: Iecc3ab6a5cd1193a1fa8e824dcf4f0b8165f9bf8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/154359
Commit-Queue: Evan Shrubsole <eshr@google.com>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29356}
diff --git a/api/video_codecs/video_encoder.cc b/api/video_codecs/video_encoder.cc
index d3f16a00..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 d91ee3c..1ae4476 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;
}
@@ -972,7 +969,7 @@
// the changes get propagated to all listeners.
EncoderRateSettings rate_settings = *last_encoder_rate_settings_;
last_encoder_rate_settings_.reset();
- rate_settings.framerate_fps = GetInputFramerateFps();
+ rate_settings.rate_control.framerate_fps = GetInputFramerateFps();
SetEncoderRates(UpdateBitrateAllocationAndNotifyObserver(rate_settings));
}
@@ -1173,7 +1170,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) {
@@ -1194,27 +1191,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;
}
@@ -1231,10 +1228,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;
}
@@ -1250,15 +1248,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));
}
}
@@ -1307,7 +1306,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 f226867..ba9f519 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 99a4b2b..31d3aa1 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -799,6 +799,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 {
@@ -865,6 +870,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) {
@@ -918,6 +924,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 {
@@ -4881,11 +4888,7 @@
// The rate settings should have been set again even though
// they haven't changed.
ASSERT_TRUE(current_rate_settings.has_value());
- EXPECT_EQ(prev_rate_settings->bitrate, current_rate_settings->bitrate);
- EXPECT_EQ(prev_rate_settings->framerate_fps,
- current_rate_settings->framerate_fps);
- EXPECT_EQ(prev_rate_settings->bandwidth_allocation,
- current_rate_settings->bandwidth_allocation);
+ EXPECT_EQ(prev_rate_settings, current_rate_settings);
video_stream_encoder_->Stop();
}
@@ -4975,4 +4978,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
