Store qp limits for ScreenshareLayers only once

Bug: webrtc:9745
Change-Id: Ie38b9d4991100657c1dc54660b39b80d86cc64fa
Reviewed-on: https://webrtc-review.googlesource.com/99940
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24716}

modules/video_coding/codecs/vp8/screenshare_layers.cc

@@ -344,6 +344,13 @@ uint32_t ScreenshareLayers::GetCodecTargetBitrateKbps() const {
 }
 
 bool ScreenshareLayers::UpdateConfiguration(Vp8EncoderConfig* cfg) {
+  if (min_qp_ == -1 || max_qp_ == -1) {
+    // Store the valid qp range. This must not change during the lifetime of
+    // this class.
+    min_qp_ = cfg->rc_min_quantizer;
+    max_qp_ = cfg->rc_max_quantizer;
+  }
+
   bool cfg_updated = false;
   uint32_t target_bitrate_kbps = GetCodecTargetBitrateKbps();
 
@@ -364,8 +371,6 @@ bool ScreenshareLayers::UpdateConfiguration(Vp8EncoderConfig* cfg) {
     // Don't reconfigure qp limits during quality boost frames.
     if (active_layer_ == -1 ||
         layers_[active_layer_].state != TemporalLayer::State::kQualityBoost) {
-      min_qp_ = cfg->rc_min_quantizer;
-      max_qp_ = cfg->rc_max_quantizer;
       // After a dropped frame, a frame with max qp will be encoded and the
       // quality will then ramp up from there. To boost the speed of recovery,
       // encode the next frame with lower max qp, if there is sufficient
@@ -408,13 +413,14 @@ bool ScreenshareLayers::UpdateConfiguration(Vp8EncoderConfig* cfg) {
   // If layer is in the quality boost state (following a dropped frame), update
   // the configuration with the adjusted (lower) qp and set the state back to
   // normal.
-  unsigned int adjusted_max_qp;
-  if (layers_[active_layer_].state == TemporalLayer::State::kQualityBoost &&
-      layers_[active_layer_].enhanced_max_qp != -1) {
-    adjusted_max_qp = layers_[active_layer_].enhanced_max_qp;
+  unsigned int adjusted_max_qp = max_qp_;  // Set the normal max qp.
+  if (layers_[active_layer_].state == TemporalLayer::State::kQualityBoost) {
+    if (layers_[active_layer_].enhanced_max_qp != -1) {
+      // Bitrate is high enough for quality boost, update max qp.
+      adjusted_max_qp = layers_[active_layer_].enhanced_max_qp;
+    }
+    // Regardless of qp, reset the boost state for the next frame.
     layers_[active_layer_].state = TemporalLayer::State::kNormal;
-  } else {
-    adjusted_max_qp = max_qp_;  // Set the normal max qp.
   }
 
   if (adjusted_max_qp == cfg->rc_max_quantizer)

modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc

@@ -162,7 +162,7 @@ class ScreenshareLayerTest : public ::testing::Test {
   }
 
   int min_qp_;
-  int max_qp_;
+  uint32_t max_qp_;
   int frame_size_;
   SimulatedClock clock_;
   std::unique_ptr<ScreenshareLayers> layers_;
@@ -661,4 +661,54 @@ TEST_F(ScreenshareLayerTest, UpdatesConfigurationAfterRateChange) {
   EXPECT_TRUE(layers_->UpdateConfiguration(&cfg_));
 }
 
+TEST_F(ScreenshareLayerTest, MaxQpRestoredAfterDoubleDrop) {
+  // Run grace period so we have existing frames in both TL0 and Tl1.
+  EXPECT_TRUE(RunGracePeriod());
+
+  // Move ahead until we have a sync frame in TL1.
+  EXPECT_EQ(kTl1SyncFlags, SkipUntilTlAndSync(1, true));
+  ASSERT_TRUE(vp8_info_.layerSync);
+
+  // Simulate overshoot of this frame.
+  layers_->FrameEncoded(timestamp_, 0, -1);
+
+  // Simulate re-encoded frame.
+  layers_->PopulateCodecSpecific(false, tl_config_, &vp8_info_, timestamp_);
+  layers_->FrameEncoded(timestamp_, 1, max_qp_);
+
+  // Next frame, expect boosted quality.
+  // Slightly alter bitrate between each frame.
+  std::vector<uint32_t> kDefault2TlBitratesBpsAlt = kDefault2TlBitratesBps;
+  kDefault2TlBitratesBpsAlt[1] += 4000;
+  layers_->OnRatesUpdated(kDefault2TlBitratesBpsAlt, kFrameRate);
+  EXPECT_EQ(kTl1Flags, SkipUntilTlAndSync(1, false));
+  EXPECT_TRUE(config_updated_);
+  EXPECT_LT(cfg_.rc_max_quantizer, max_qp_);
+  uint32_t adjusted_qp = cfg_.rc_max_quantizer;
+
+  // Simulate overshoot of this frame.
+  layers_->FrameEncoded(timestamp_, 0, -1);
+
+  // Simulate re-encoded frame.
+  layers_->PopulateCodecSpecific(false, tl_config_, &vp8_info_, timestamp_);
+  layers_->FrameEncoded(timestamp_, frame_size_, max_qp_);
+
+  // A third frame, expect boosted quality.
+  layers_->OnRatesUpdated(kDefault2TlBitratesBps, kFrameRate);
+  EXPECT_EQ(kTl1Flags, SkipUntilTlAndSync(1, false));
+  EXPECT_TRUE(config_updated_);
+  EXPECT_LT(cfg_.rc_max_quantizer, max_qp_);
+  EXPECT_EQ(adjusted_qp, cfg_.rc_max_quantizer);
+
+  // Frame encoded.
+  layers_->PopulateCodecSpecific(false, tl_config_, &vp8_info_, timestamp_);
+  layers_->FrameEncoded(timestamp_, frame_size_, max_qp_);
+
+  // A fourth frame, max qp should be restored.
+  layers_->OnRatesUpdated(kDefault2TlBitratesBpsAlt, kFrameRate);
+  EXPECT_EQ(kTl1Flags, SkipUntilTlAndSync(1, false));
+  EXPECT_TRUE(config_updated_);
+  EXPECT_EQ(cfg_.rc_max_quantizer, max_qp_);
+}
+
 }  // namespace webrtc