Recreate WebrtcVideoSendStream if screen content setting is changed.

webrtc/media/engine/webrtcvideoengine2.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 1594 matching lines @@
     rtc::VideoSourceInterface<webrtc::VideoFrame>* source) {
   TRACE_EVENT0("webrtc", "WebRtcVideoSendStream::SetVideoSend");
   RTC_DCHECK_RUN_ON(&thread_checker_);
 
   // Ignore |options| pointer if |enable| is false.
   bool options_present = enable && options;
 
   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();
     }
   }
 
   if (source_ && stream_) {
     stream_->SetSource(
         nullptr, webrtc::VideoSendStream::DegradationPreference::kBalanced);
   }
   // Switch to the new source.
   source_ = source;
@@ skipping 269 matching lines @@
 
   RTC_CHECK(parameters_.codec_settings);
   VideoCodecSettings codec_settings = *parameters_.codec_settings;
 
   webrtc::VideoEncoderConfig encoder_config =
       CreateVideoEncoderConfig(codec_settings.codec);
 
   encoder_config.encoder_specific_settings = ConfigureVideoEncoderSettings(
       codec_settings.codec);
 
   stream_->ReconfigureVideoEncoder(encoder_config.Copy());

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

If ReconfigureVideoEncoder doesn't support changing the screencast flag, should
it DCHECK that the flag is unchanged?

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

Good idea. I'll add it to webrtc::VideoSendStream itself, since it's the one
counting on it.

 
   encoder_config.encoder_specific_settings = NULL;
 
   parameters_.encoder_config = std::move(encoder_config);
 }
 
 void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSend(bool send) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   sending_ = send;
   UpdateSendState();
@@ skipping 611 matching lines @@
         rtx_mapping[video_codecs[i].codec.id] !=
             ulpfec_config.red_payload_type) {
       video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id];
     }
   }
 
   return video_codecs;
 }
 
 }  // namespace cricket