Recreate WebrtcVideoSendStream if screen content setting is changed.

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index c192b52402ee95ef3f2ffd9dbe25f65ded5ecf6d..02063954921b28a228a7de8a6dbc576c042a9154 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -1612,7 +1612,15 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(
   if (options_present) {
     VideoOptions old_options = parameters_.options;
     parameters_.options.SetAll(*options);
-    if (parameters_.options != old_options) {
+    if (parameters_.options.is_screencast.value_or(false) !=
+            old_options.is_screencast.value_or(false) &&
+        parameters_.codec_settings) {

[pthatcher1, 2017/02/22 22:24:14]

This needs to test that all the other parameters_.options haven't changed,
doesn't it?

[Taylor Brandstetter, 2017/02/23 00:46:49]

Why? A whole new webrtc::VideoEncoder will be created.

[pthatcher1, 2017/02/23 21:05:33]

First, it's not clear from the code that this case is a superset of the second
case.  

Second, because if someone adds something else to the second case (say... do
something funny when max bitrate changes), then it will surprisingly not happen
when both content type and max bitrate change.   

In other words, there's an implicit superset behavior that isn't clear that
could easily be broken by adding to the second case and making the first case no
longer a superset of behavior.

[sprang_webrtc, 2017/02/26 13:41:27]

Agree that it's not entirely obvious that ReconfigureEncoder() is a subset of
SetCodec(). At least partially. SetCodec() will recreate the streams (this seems
clear and has test coverage), and that definitely is a subset of reconfiguring
the encoder.
That the way the configuration is updated in ReconfigureEncoder() is a subset of
SetCodec() is however not obvious. It seems ReconfigureEncoder() (currently)
comprises calls to 
* CreateVideoEncoderConfig(), which is called at the beginning of SetCodec()
* ConfigureVideoEncoderSettings() which is called from RecreateWebRtcStream()
which is called at the end of SetCodec().

Writing a test to verify that no other parameters are missed will be very
difficult however. I propose simply calling ReconfigureEncoder() again if other
parameters have changed. I guess this may incur a slight performance hit when
switching content type and other options and the same time, but I think it's not
critical. The encoder will do most of the initialization on InitEncode() which
won't happen until the next frame anyway, so it may be that there won't even be
an extra reconfig.

+      // If screen content settings change, we may need to recreate the codec
+      // instance so that the correct type is used. Add a param to force the
+      // SetCodec() to not resuse the current instance.
+      allocated_encoder_.codec.params["recreate"] = "true";

[pthatcher1, 2017/02/22 22:24:14]

What is this codec parameter?  Is it already defined lower in the stack?  It
looks really hacky.

[sprang_webrtc, 2017/02/23 15:10:14]

It is kinda hacky, and is workaround just to avoid reuse in
WebRtcVideoSendStream::CreateVideoEncoder().

I guess the alternative is to add a screenshare flag to the AllocatedEncoder.
Or I can perhaps do
allocated_encoder_.codec.params["is_screenshare"] = 
  parameters_.options.is_screencast.value_or(false) ? "true" : "false";

[pthatcher1, 2017/02/23 21:05:33]

It seems like we should just have a small refactoring to avoid needing this
hack;

bool screencast_changed = ...;
if (screencast_changed && parameters_.codec_settings) {
  CreateAndSetEncoder(*parameters_.codec_settings);
} else {
  ReconfigureEncoder();
}

void CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings)
{
  // Moved from old CreateVideoEncoder
  if (codec_settings.codec == allocated_encoder_.codec &&
      allocated_encoder_.encoder != nullptr) {
  }
  CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings);
}

void CreateAndSetEncoder(const VideoCodecSettings& codec_settings) {
  // Same as old SetCodec, except with new CreateVideoEncoder which always
creates.
}

AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec) {
  // Same as old CreateVideoEncoder, except always creates (doesn't do the check
for allocated_encoder_).
}

[Taylor Brandstetter, 2017/02/24 00:13:55]

Or, just make CreateVideoEncoder a bit smarter. The issue is that it returns
early if the codec isn't changing, even though later it uses things other than
the codec:
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

[sprang_webrtc, 2017/02/26 13:41:27]

CreateVideoEncoder doesn't know about the change in parameters though, so in
order to detect a change there, we need to add a is_screenshare member to
AllocatedEncoder, which I guess is doable if you prefer that.

Instead of adding the suggest new versions of SetCodec(), is just added a
parameter to force an encoder allocation. 
wdyt?

+      SetCodec(*parameters_.codec_settings);
+    } else if (parameters_.options != old_options) {
       ReconfigureEncoder();
     }
   }