Update screen simulcast config and fix periodic encoder param update

webrtc/media/engine/simulcast.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 158 matching lines @@
 
 std::vector<webrtc::VideoStream> GetSimulcastConfig(size_t max_streams,
                                                     int width,
                                                     int height,
                                                     int max_bitrate_bps,
                                                     int max_qp,
                                                     int max_framerate,
                                                     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);
   }
 
   if (num_simulcast_layers > max_streams) {
     // If the number of SSRCs in the group differs from our target
     // number of simulcast streams for current resolution, switch down
     // to a resolution that matches our number of SSRCs.
     if (!SlotSimulcastMaxResolution(max_streams, &width, &height)) {
       return std::vector<webrtc::VideoStream>();
     }
     num_simulcast_layers = 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);
       streams[s].target_bitrate_bps =
           FindSimulcastTargetBitrateBps(width, height);
       streams[s].min_bitrate_bps = FindSimulcastMinBitrateBps(width, height);
       streams[s].max_framerate = max_framerate;
+
+      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 (!is_screencast) {
-      width /= 2;
-      height /= 2;
+    // 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;
     }
-    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;
   }
 
   return streams;
 }
 
 static const int kScreenshareMinBitrateKbps = 50;
 static const int kScreenshareMaxBitrateKbps = 6000;
 static const int kScreenshareDefaultTl0BitrateKbps = 200;
 static const int kScreenshareDefaultTl1BitrateKbps = 1000;
 
@@ skipping 41 matching lines @@
   config->tl1_bitrate_kbps = tl1_bitrate;
 
   return true;
 }
 
 bool UseSimulcastScreenshare() {
   return webrtc::field_trial::IsEnabled(kSimulcastScreenshareFieldTrialName);
 }
 
 }  // namespace cricket