Smoother frame dropping when screenshare_layers limits fps
This is a reland of https://webrtc-review.googlesource.com/34380
The main problem with that CL was that we used frame timestamps as basis
for frame dropping, but those might not be continuous or even populated
in some circumstances.
Additionally, I found that the bitrate was off since the encoder does
not not take the dropped frames into account, so if we drop every other
frame continiusoly, the bitrate sent will be around half of the target.
Patch set 1 is the original CL, subsequent patch sets cotains fixes.
Bug: webrtc:4172
Change-Id: I8ec8dddcebf4ce44f28dd9055cf9c46bbd68e4a6
Reviewed-on: https://webrtc-review.googlesource.com/39201
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21601}diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.cc b/modules/video_coding/codecs/vp8/screenshare_layers.cc
index 782dc77..444c0cc 100644
--- a/modules/video_coding/codecs/vp8/screenshare_layers.cc
+++ b/modules/video_coding/codecs/vp8/screenshare_layers.cc
@@ -85,6 +85,7 @@
last_timestamp_(-1),
last_sync_timestamp_(-1),
last_emitted_tl0_timestamp_(-1),
+ last_frame_time_ms_(-1),
min_qp_(-1),
max_qp_(-1),
max_debt_bytes_(0),
@@ -113,14 +114,6 @@
}
const int64_t now_ms = clock_->TimeInMilliseconds();
- if (target_framerate_.value_or(0) > 0 &&
- encode_framerate_.Rate(now_ms).value_or(0) > *target_framerate_) {
- // Max framerate exceeded, drop frame.
- return TemporalLayers::FrameConfig(kNone, kNone, kNone);
- }
-
- if (stats_.first_frame_time_ms_ == -1)
- stats_.first_frame_time_ms_ = now_ms;
int64_t unwrapped_timestamp = time_wrap_handler_.Unwrap(timestamp);
int64_t ts_diff;
@@ -129,10 +122,31 @@
} else {
ts_diff = unwrapped_timestamp - last_timestamp_;
}
+
+ if (target_framerate_) {
+ // If input frame rate exceeds target frame rate, either over a one second
+ // averaging window, or if frame interval is below 90% of desired value,
+ // drop frame.
+ // Use real-time clock rather than timestamps, in case there is a
+ // discontinuity in the timestamps sequence.
+ if (encode_framerate_.Rate(now_ms).value_or(0) > *target_framerate_)
+ return TemporalLayers::FrameConfig(kNone, kNone, kNone);
+
+ int64_t expected_frame_interval_ms = 1000 / *target_framerate_;
+ if (last_frame_time_ms_ != -1 &&
+ now_ms - last_frame_time_ms_ < (9 * expected_frame_interval_ms) / 10) {
+ return TemporalLayers::FrameConfig(kNone, kNone, kNone);
+ }
+ }
+
+ if (stats_.first_frame_time_ms_ == -1)
+ stats_.first_frame_time_ms_ = now_ms;
+
// Make sure both frame droppers leak out bits.
layers_[0].UpdateDebt(ts_diff / 90);
layers_[1].UpdateDebt(ts_diff / 90);
last_timestamp_ = timestamp;
+ last_frame_time_ms_ = now_ms;
TemporalLayerState layer_state = TemporalLayerState::kDrop;
@@ -360,8 +374,20 @@
bool ScreenshareLayers::UpdateConfiguration(vpx_codec_enc_cfg_t* cfg) {
bool cfg_updated = false;
uint32_t target_bitrate_kbps = GetCodecTargetBitrateKbps();
- if (bitrate_updated_ || cfg->rc_target_bitrate != target_bitrate_kbps) {
- cfg->rc_target_bitrate = target_bitrate_kbps;
+
+ // TODO(sprang): We _really_ need to make an overhaul of this class. :(
+ // If we're dropping frames in order to meet a target framerate, adjust the
+ // bitrate assigned to the encoder so the total average bitrate is correct.
+ float encoder_config_bitrate_kbps = target_bitrate_kbps;
+ if (target_framerate_ && capture_framerate_ &&
+ *target_framerate_ < *capture_framerate_) {
+ encoder_config_bitrate_kbps *=
+ static_cast<float>(*capture_framerate_) / *target_framerate_;
+ }
+
+ if (bitrate_updated_ ||
+ cfg->rc_target_bitrate != encoder_config_bitrate_kbps) {
+ cfg->rc_target_bitrate = encoder_config_bitrate_kbps;
// Don't reconfigure qp limits during quality boost frames.
if (active_layer_ == -1 ||
diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.h b/modules/video_coding/codecs/vp8/screenshare_layers.h
index 81db90a..7143351 100644
--- a/modules/video_coding/codecs/vp8/screenshare_layers.h
+++ b/modules/video_coding/codecs/vp8/screenshare_layers.h
@@ -71,6 +71,7 @@
int64_t last_timestamp_;
int64_t last_sync_timestamp_;
int64_t last_emitted_tl0_timestamp_;
+ int64_t last_frame_time_ms_;
rtc::TimestampWrapAroundHandler time_wrap_handler_;
int min_qp_;
int max_qp_;
diff --git a/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc b/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
index 754235a..422905d 100644
--- a/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
+++ b/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
@@ -70,7 +70,7 @@
}
int ConfigureFrame(bool key_frame) {
- tl_config_ = layers_->UpdateLayerConfig(timestamp_);
+ tl_config_ = UpdateLayerConfig(timestamp_);
EXPECT_EQ(0, tl_config_.encoder_layer_id)
<< "ScreenshareLayers always encodes using the bitrate allocator for "
"layer 0, but may reference different buffers and packetize "
@@ -86,6 +86,12 @@
return flags;
}
+ TemporalLayers::FrameConfig UpdateLayerConfig(uint32_t timestamp) {
+ int64_t timestamp_ms = timestamp / 90;
+ clock_.AdvanceTimeMilliseconds(timestamp_ms - clock_.TimeInMilliseconds());
+ return layers_->UpdateLayerConfig(timestamp);
+ }
+
int FrameSizeForBitrate(int bitrate_kbps) {
return ((bitrate_kbps * 1000) / 8) / kFrameRate;
}
@@ -206,7 +212,7 @@
std::vector<int> sync_times;
const int kNumFrames = kMaxSyncPeriodSeconds * kFrameRate * 2 - 1;
for (int i = 0; i < kNumFrames; ++i) {
- tl_config_ = layers_->UpdateLayerConfig(timestamp_);
+ tl_config_ = UpdateLayerConfig(timestamp_);
config_updated_ = layers_->UpdateConfiguration(&cfg_);
layers_->PopulateCodecSpecific(false, tl_config_, &vp8_info_, timestamp_);
@@ -431,8 +437,8 @@
layers_->OnRatesUpdated(kLowBitrateKbps, kLowBitrateKbps, 5);
layers_->UpdateConfiguration(&cfg_);
- EXPECT_EQ(kTl0Flags, VP8EncoderImpl::EncodeFlags(
- layers_->UpdateLayerConfig(kStartTimestamp)));
+ EXPECT_EQ(kTl0Flags,
+ VP8EncoderImpl::EncodeFlags(UpdateLayerConfig(kStartTimestamp)));
layers_->FrameEncoded(kLargeFrameSizeBytes, kDefaultQp);
const uint32_t kTwoSecondsLater =
@@ -442,11 +448,16 @@
ASSERT_GT(kStartTimestamp + 90 * (kLargeFrameSizeBytes * 8) / kLowBitrateKbps,
kStartTimestamp + (ScreenshareLayers::kMaxFrameIntervalMs * 90));
- EXPECT_TRUE(layers_->UpdateLayerConfig(kTwoSecondsLater).drop_frame);
+ // Expect drop one frame interval before the two second timeout. If we try
+ // any later, the frame will be dropped anyway by the frame rate throttling
+ // logic.
+ EXPECT_TRUE(
+ UpdateLayerConfig(kTwoSecondsLater - kTimestampDelta5Fps).drop_frame);
+
// More than two seconds has passed since last frame, one should be emitted
// even if bitrate target is then exceeded.
EXPECT_EQ(kTl0Flags, VP8EncoderImpl::EncodeFlags(
- layers_->UpdateLayerConfig(kTwoSecondsLater + 90)));
+ UpdateLayerConfig(kTwoSecondsLater + 90)));
}
TEST_F(ScreenshareLayerTest, UpdatesHistograms) {
@@ -459,7 +470,7 @@
for (int64_t timestamp = 0;
timestamp < kTimestampDelta5Fps * 5 * metrics::kMinRunTimeInSeconds;
timestamp += kTimestampDelta5Fps) {
- tl_config_ = layers_->UpdateLayerConfig(timestamp);
+ tl_config_ = UpdateLayerConfig(timestamp);
if (tl_config_.drop_frame) {
dropped_frame = true;
continue;
@@ -472,8 +483,6 @@
// Simulate one overshoot.
layers_->FrameEncoded(0, 0);
overshoot = true;
- flags =
- VP8EncoderImpl::EncodeFlags(layers_->UpdateLayerConfig(timestamp));
}
if (flags == kTl0Flags) {
@@ -547,7 +556,7 @@
// Send at regular rate - no drops expected.
for (int64_t i = 0; i < kTestSpanMs; i += kFrameIntervalsMs) {
- if (layers_->UpdateLayerConfig(timestamp).drop_frame) {
+ if (UpdateLayerConfig(timestamp).drop_frame) {
++num_discarded_frames;
} else {
size_t frame_size_bytes = kDefaultTl0BitrateKbps * kFrameIntervalsMs / 8;
@@ -563,7 +572,7 @@
num_input_frames = 0;
num_discarded_frames = 0;
for (int64_t i = 0; i < kTestSpanMs; i += kFrameIntervalsMs / 2) {
- if (layers_->UpdateLayerConfig(timestamp).drop_frame) {
+ if (UpdateLayerConfig(timestamp).drop_frame) {
++num_discarded_frames;
} else {
size_t frame_size_bytes = kDefaultTl0BitrateKbps * kFrameIntervalsMs / 8;
@@ -589,13 +598,6 @@
// Simulate overshoot of this frame.
layers_->FrameEncoded(0, -1);
- // Reencode, frame config, flags and codec specific info should remain the
- // same as for the dropped frame.
- timestamp_ -= kTimestampDelta5Fps; // Undo last timestamp increment.
- TemporalLayers::FrameConfig new_tl_config =
- layers_->UpdateLayerConfig(timestamp_);
- EXPECT_EQ(tl_config_, new_tl_config);
-
config_updated_ = layers_->UpdateConfiguration(&cfg_);
EXPECT_EQ(kTl1SyncFlags, VP8EncoderImpl::EncodeFlags(tl_config_));
@@ -604,4 +606,42 @@
EXPECT_TRUE(new_vp8_info.layerSync);
}
+TEST_F(ScreenshareLayerTest, DropOnTooShortFrameInterval) {
+ // Run grace period so we have existing frames in both TL0 and Tl1.
+ EXPECT_TRUE(RunGracePeriod());
+
+ // Add a large gap, so there's plenty of room in the rate tracker.
+ timestamp_ += kTimestampDelta5Fps * 3;
+ EXPECT_FALSE(UpdateLayerConfig(timestamp_).drop_frame);
+ layers_->FrameEncoded(frame_size_, kDefaultQp);
+
+ // Frame interval below 90% if desired time is not allowed, try inserting
+ // frame just before this limit.
+ const int64_t kMinFrameInterval = (kTimestampDelta5Fps * 9) / 10;
+ timestamp_ += kMinFrameInterval - 90;
+ EXPECT_TRUE(UpdateLayerConfig(timestamp_).drop_frame);
+
+ // Try again at the limit, now it should pass.
+ timestamp_ += 90;
+ EXPECT_FALSE(UpdateLayerConfig(timestamp_).drop_frame);
+}
+
+TEST_F(ScreenshareLayerTest, AdjustsBitrateWhenDroppingFrames) {
+ const uint32_t kTimestampDelta10Fps = kTimestampDelta5Fps / 2;
+ const int kNumFrames = 30;
+ uint32_t default_bitrate = cfg_.rc_target_bitrate;
+ layers_->OnRatesUpdated(kDefaultTl0BitrateKbps, kDefaultTl1BitrateKbps, 10);
+
+ int num_dropped_frames = 0;
+ for (int i = 0; i < kNumFrames; ++i) {
+ if (EncodeFrame(false) == -1)
+ ++num_dropped_frames;
+ timestamp_ += kTimestampDelta10Fps;
+ }
+ layers_->UpdateConfiguration(&cfg_);
+
+ EXPECT_EQ(num_dropped_frames, kNumFrames / 2);
+ EXPECT_EQ(cfg_.rc_target_bitrate, default_bitrate * 2);
+}
+
} // namespace webrtc
