Update screen simulcast config and fix periodic encoder param update

webrtc/media/engine/simulcast.cc

Index: webrtc/media/engine/simulcast.cc
diff --git a/webrtc/media/engine/simulcast.cc b/webrtc/media/engine/simulcast.cc
index f1cd2ced459810b03b1737632f947cf5d08a0b6d..0007bfc4e9ec075f59efca337300769dfdf9fad9 100644
--- a/webrtc/media/engine/simulcast.cc
+++ b/webrtc/media/engine/simulcast.cc
@@ -176,8 +176,12 @@ std::vector<webrtc::VideoStream> GetSimulcastConfig(size_t max_streams,
                                                     bool is_screencast) {
   size_t num_simulcast_layers;
   if (is_screencast) {
-    num_simulcast_layers =
-        UseSimulcastScreenshare() ? kDefaultScreenshareSimulcastStreams : 1;
+    if (UseSimulcastScreenshare()) {
+      num_simulcast_layers =
+          std::min<int>(max_streams, kDefaultScreenshareSimulcastStreams);

[stefan-webrtc, 2017/04/05 15:06:48]

kDefaultScreenshareSimulcastStreams should maybe be called
kMaxScreenshareSimulcastStreams?

[sprang_webrtc, 2017/05/08 08:50:53]

Sure.

+    } else {
+      num_simulcast_layers = 1;
+    }
   } else {
     num_simulcast_layers = FindSimulcastMaxLayers(width, height);
   }
@@ -194,33 +198,57 @@ std::vector<webrtc::VideoStream> GetSimulcastConfig(size_t max_streams,
   std::vector<webrtc::VideoStream> streams;
   streams.resize(num_simulcast_layers);
 
-  if (!is_screencast) {
+  if (is_screencast) {

[stefan-webrtc, 2017/04/05 15:06:48]

I think it would be good to have a general comment of what the code below does.
Is the goal to configure "legacy" screenshare with two temporal layers if no
spatial layers are enabled, and if spatial layers configure up to two spatial
layers with 3 regular temporal layers each?

[sprang_webrtc, 2017/05/08 08:50:53]

Added comments.
In both cases we use the legacy screenshare setup for the lowest simulcast
stream. If simulcast is enabled, we add a second high quality stream on top of
the legacy one. This will use the normal 3TL setup. So the two streams will have
completely independent frame rate and temporal layer config. This is not changed
with this CL, the main change is that we can now explicitly set the config for
the higher stream without a bunch of checks for "is_screenshare && blah"

+    ScreenshareLayerConfig config = ScreenshareLayerConfig::GetDefault();
+    // For legacy screenshare in conference mode, tl0 and tl1 bitrates are
+    // piggybacked on the VideoCodec struct as target and max bitrates,
+    // respectively. See eg. webrtc::VP8EncoderImpl::SetRates().
+    streams[0].width = width;
+    streams[0].height = height;
+    streams[0].max_qp = max_qp;
+    streams[0].max_framerate = 5;
+    streams[0].min_bitrate_bps = kMinVideoBitrateKbps * 1000;
+    streams[0].target_bitrate_bps = config.tl0_bitrate_kbps * 1000;
+    streams[0].max_bitrate_bps = config.tl1_bitrate_kbps * 1000;
+    streams[0].temporal_layer_thresholds_bps.clear();
+    streams[0].temporal_layer_thresholds_bps.push_back(config.tl0_bitrate_kbps *
+                                                       1000);

[stefan-webrtc, 2017/04/05 15:06:48]

Looks like we're doing 2 temporal layers here, and 3 for the higher quality
stream if there simulcast is configured.

[sprang_webrtc, 2017/05/08 08:50:53]

Yes, that's the idea.

+
+    if (num_simulcast_layers == 2) {

[stefan-webrtc, 2017/04/05 15:06:48]

Should this be kDefaultScreenshareSimulcastStreams?

[sprang_webrtc, 2017/05/08 08:50:53]

Done.

+      // Add optional upper simulcast layer.
+      // Lowest temporal layers of a 3 layer setup will have 40% of the total
+      // bitrate allocation for that stream. Make sure the gap between the
+      // target of the lower stream and first temporal layer of the higher one
+      // is at most 2x the bitrate, so that upswitching is not hampered by
+      // stalled bitrate estimates.

[stefan-webrtc, 2017/04/05 15:06:48]

I don't fully understand this. Why would the bitrate estimate stall? Is that
something that needs fixing elsewhere? AFAIK we are padding to be able to reach
higher bitrates independent of what the codec produces?

Why do we not use some factor of streams[0].max_bitrate_bps for this? Should we
not be able to get to that layer before we get to the next spatial layer?

[sprang_webrtc, 2017/05/08 08:50:53]

We do at the source, yes. But there is no guarantee for this after pass-through
via an MCU.

+      int max_bitrate_bps = 2 * ((streams[0].target_bitrate_bps * 10) / 4);
+      // Cap max bitrate so it isn't overly high for the given resolution.
+      max_bitrate_bps = std::min<int>(
+          max_bitrate_bps, FindSimulcastMaxBitrateBps(width, height));
+
+      streams[1].width = width;
+      streams[1].height = height;
+      streams[1].max_qp = max_qp;
+      streams[1].max_framerate = max_framerate;
+      // Three temporal layers means two thresholds.
+      streams[1].temporal_layer_thresholds_bps.resize(2);
+      streams[1].min_bitrate_bps = streams[0].target_bitrate_bps * 2;
+      streams[1].target_bitrate_bps = max_bitrate_bps;
+      streams[1].max_bitrate_bps = max_bitrate_bps;
+    }
+  } else {
     // Format width and height has to be divisible by |2 ^ number_streams - 1|.
     width = NormalizeSimulcastSize(width, num_simulcast_layers);
     height = NormalizeSimulcastSize(height, num_simulcast_layers);
-  }
 
-  // Add simulcast sub-streams from lower resolution to higher resolutions.
-  // Add simulcast streams, from highest resolution (|s| = number_streams -1)
-  // to lowest resolution at |s| = 0.
-  for (size_t s = num_simulcast_layers - 1;; --s) {
-    streams[s].width = width;
-    streams[s].height = height;
-    // TODO(pbos): Fill actual temporal-layer bitrate thresholds.
-    streams[s].max_qp = max_qp;
-    if (is_screencast && s == 0) {
-      ScreenshareLayerConfig config = ScreenshareLayerConfig::GetDefault();
-      // For legacy screenshare in conference mode, tl0 and tl1 bitrates are
-      // piggybacked on the VideoCodec struct as target and max bitrates,
-      // respectively. See eg. webrtc::VP8EncoderImpl::SetRates().
-      streams[s].min_bitrate_bps = kMinVideoBitrateKbps * 1000;
-      streams[s].target_bitrate_bps = config.tl0_bitrate_kbps * 1000;
-      streams[s].max_bitrate_bps = config.tl1_bitrate_kbps * 1000;
-      streams[s].temporal_layer_thresholds_bps.clear();
-      streams[s].temporal_layer_thresholds_bps.push_back(
-          config.tl0_bitrate_kbps * 1000);
-      streams[s].max_framerate = 5;
-    } else {
+    // Add simulcast sub-streams from lower resolution to higher resolutions.
+    // Add simulcast streams, from highest resolution (|s| = number_streams -1)
+    // to lowest resolution at |s| = 0.
+    for (size_t s = num_simulcast_layers - 1;; --s) {
+      streams[s].width = width;
+      streams[s].height = height;
+      // TODO(pbos): Fill actual temporal-layer bitrate thresholds.
+      streams[s].max_qp = max_qp;
       streams[s].temporal_layer_thresholds_bps.resize(
           kDefaultConferenceNumberOfTemporalLayers[s] - 1);
       streams[s].max_bitrate_bps = FindSimulcastMaxBitrateBps(width, height);
@@ -228,20 +256,20 @@ std::vector<webrtc::VideoStream> GetSimulcastConfig(size_t max_streams,
           FindSimulcastTargetBitrateBps(width, height);
       streams[s].min_bitrate_bps = FindSimulcastMinBitrateBps(width, height);
       streams[s].max_framerate = max_framerate;
-    }
 
-    if (!is_screencast) {
-      width /= 2;
-      height /= 2;
+      if (!is_screencast) {

[stefan-webrtc, 2017/04/05 15:06:48]

This is always true AFAICT?

[sprang_webrtc, 2017/05/08 08:50:53]

Done.

+        width /= 2;
+        height /= 2;
+      }
+      if (s == 0)
+        break;
     }
-    if (s == 0)
-      break;
-  }
 
-  // Spend additional bits to boost the max stream.
-  int bitrate_left_bps = max_bitrate_bps - GetTotalMaxBitrateBps(streams);
-  if (bitrate_left_bps > 0) {
-    streams.back().max_bitrate_bps += bitrate_left_bps;
+    // Spend additional bits to boost the max stream.
+    int bitrate_left_bps = max_bitrate_bps - GetTotalMaxBitrateBps(streams);
+    if (bitrate_left_bps > 0) {
+      streams.back().max_bitrate_bps += bitrate_left_bps;
+    }
   }
 
   return streams;