Recreate WebrtcVideoSendStream if screen content setting is changed.

webrtc/media/engine/webrtcvideoengine2_unittest.cc

 /*
  *  Copyright (c) 2004 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 888 matching lines @@
   StreamParams sp = cricket::StreamParams::CreateLegacy(kSsrc);
   sp.id = "FakeStreamParamsId";
   EXPECT_TRUE(channel->AddRecvStream(sp));
   EXPECT_EQ(1u, decoder_factory.decoders().size());
 
   std::vector<cricket::VideoDecoderParams> params = decoder_factory.params();
   ASSERT_EQ(1u, params.size());
   EXPECT_EQ(sp.id, params[0].receive_stream_id);
 }
 
+TEST_F(WebRtcVideoEngine2Test, RecreatesEncoderOnContentTypeChange) {

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

I think you need a test for when both the content type changes and something
else from VideoOptions changes at the same time.

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

I added a change to another option just to verify i still recreates the stream.

However, I spent several hours today trying make a test where we can distinguish
between recreating the stream and actually changing the codec. This seems
impossible as we can't directly detect creation of the SimulcastEncoderAdapter
everything gets wrapped in. The FakeEncoder instances (which we can observe) are
called from InitEncode() in the adapter, and that is actually called on
recreating the stream...

+  cricket::FakeWebRtcVideoEncoderFactory encoder_factory;
+  encoder_factory.AddSupportedVideoCodecType("VP8");
+  std::unique_ptr<FakeCall> fake_call(
+      new FakeCall(webrtc::Call::Config(&event_log_)));
+  std::unique_ptr<VideoMediaChannel> channel(
+      SetUpForExternalEncoderFactory(&encoder_factory));
+  ASSERT_TRUE(
+      channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc)));
+  cricket::VideoCodec codec = GetEngineCodec("VP8");
+  cricket::VideoSendParameters parameters;
+  parameters.codecs.push_back(codec);
+  channel->OnReadyToSend(true);
+  channel->SetSend(true);
+  ASSERT_TRUE(channel->SetSendParameters(parameters));
+
+  cricket::FakeVideoCapturer capturer;
+  VideoOptions options;
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+
+  EXPECT_EQ(cricket::CS_RUNNING,
+            capturer.Start(capturer.GetSupportedFormats()->front()));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(1));

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

Should this be checking that the created encoder and/or send stream has the
right content type applied?

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

Done.

+
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  // No change in content type, keep current encoder.
+  EXPECT_EQ(1, encoder_factory.GetNumCreatedEncoders());
+
+  options.is_screencast.emplace(true);
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  // Change to screen content, recreate encoder. For the simulcast encoder
+  // adapter case, this will result in two calls since InitEncode triggers a
+  // a new instance.
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2));
+
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  // Still screen content, no need to update encoder.
+  EXPECT_EQ(2, encoder_factory.GetNumCreatedEncoders());
+
+  options.is_screencast.emplace(false);
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  // Change back to regular video content, update encoder.
+  EXPECT_TRUE(capturer.CaptureFrame());
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
+
+  // Remove stream previously added to free the external encoder instance.
+  EXPECT_TRUE(channel->RemoveSendStream(kSsrc));
+  EXPECT_EQ(0u, encoder_factory.encoders().size());
+}
+
 #define WEBRTC_BASE_TEST(test) \
   TEST_F(WebRtcVideoChannel2BaseTest, test) { Base::test(); }
 
 #define WEBRTC_DISABLED_BASE_TEST(test) \
   TEST_F(WebRtcVideoChannel2BaseTest, DISABLED_##test) { Base::test(); }
 
 WEBRTC_BASE_TEST(SetSend);
 WEBRTC_BASE_TEST(SetSendWithoutCodecs);
 WEBRTC_BASE_TEST(SetSendSetsTransportBufferSizes);
 
@@ skipping 742 matching lines @@
   EXPECT_EQ(0, encoder_config.min_transmit_bitrate_bps)
       << "Non-screenshare shouldn't use min-transmit bitrate.";
 
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr));
   EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
   VideoOptions screencast_options;
   screencast_options.is_screencast = rtc::Optional<bool>(true);
   EXPECT_TRUE(
       channel_->SetVideoSend(last_ssrc_, true, &screencast_options, &capturer));
   EXPECT_TRUE(capturer.CaptureFrame());
-  // Send stream not recreated after option change.
+  // Send stream recreated after option change.
   ASSERT_EQ(send_stream, fake_call_->GetVideoSendStreams().front());
-  EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
+  EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
 
   // Verify screencast settings.
   encoder_config = send_stream->GetEncoderConfig().Copy();
   EXPECT_EQ(webrtc::VideoEncoderConfig::ContentType::kScreen,
             encoder_config.content_type);
   EXPECT_EQ(kScreenshareMinBitrateKbps * 1000,
             encoder_config.min_transmit_bitrate_bps);
 
   streams = send_stream->GetVideoStreams();
   EXPECT_EQ(capture_format_hd.width, streams.front().width);
   EXPECT_EQ(capture_format_hd.height, streams.front().height);
   EXPECT_TRUE(streams[0].temporal_layer_thresholds_bps.empty());
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr));
 }
 
-TEST_F(WebRtcVideoChannel2Test, NoRecreateStreamForScreencast) {
+TEST_F(WebRtcVideoChannel2Test, RecreateStreamForScreencast) {

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

It seems odd to make this test the opposite of what it originally was intended
to test. Is there any value to having it now?

Also, from the comments on the original CL:

pbos: I still think you want to use [ReconfigureVideoEncoder] for screenshare
being turned on or not. SetCodecAndApplyOptions is significantly more expensive
(causes a recreate of the stream).

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

Maybe not, it's covered by RecreatesEncoderOnContentTypeChange() now.
Yes, that is a bit unfortunate, but something that will be needed. We need to
recreate the stream both for the stats thing and also to make sure the correct
encoder is created. For screenshare simulcast we currently need a simulcast
encoder adapter, or we'll get crashes.

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

Acknowledged.

   EXPECT_TRUE(channel_->SetSendParameters(send_parameters_));
   ASSERT_TRUE(
       channel_->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc)));
   EXPECT_TRUE(channel_->SetSend(true));
 
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, &capturer));
   EXPECT_EQ(cricket::CS_RUNNING,
             capturer.Start(capturer.GetSupportedFormats()->front()));
   EXPECT_TRUE(capturer.CaptureFrame());
 
   ASSERT_EQ(1, fake_call_->GetNumCreatedSendStreams());
   FakeVideoSendStream* stream = fake_call_->GetVideoSendStreams().front();
   webrtc::VideoEncoderConfig encoder_config = stream->GetEncoderConfig().Copy();
   EXPECT_EQ(webrtc::VideoEncoderConfig::ContentType::kRealtimeVideo,
             encoder_config.content_type);
 
   EXPECT_EQ(1, stream->GetNumberOfSwappedFrames());
 
   /* Switch to screencast source. We expect a reconfigure of the
-   * encoder, but no change of the send stream. */
+   * encoder, and an update of the send stream. */
   struct VideoOptions video_options;
   video_options.is_screencast = rtc::Optional<bool>(true);
   EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, &video_options, &capturer));
 
   EXPECT_TRUE(capturer.CaptureFrame());
-  ASSERT_EQ(1, fake_call_->GetNumCreatedSendStreams());
-  ASSERT_EQ(stream, fake_call_->GetVideoSendStreams().front());
-  EXPECT_EQ(2, stream->GetNumberOfSwappedFrames());
+  ASSERT_EQ(2, fake_call_->GetNumCreatedSendStreams());
+  stream = fake_call_->GetVideoSendStreams().front();
+  EXPECT_EQ(1, stream->GetNumberOfSwappedFrames());
 
   encoder_config = stream->GetEncoderConfig().Copy();
   EXPECT_EQ(webrtc::VideoEncoderConfig::ContentType::kScreen,
             encoder_config.content_type);
 
   /* Switch back. */
   video_options.is_screencast = rtc::Optional<bool>(false);
   EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, &video_options, &capturer));
 
   EXPECT_TRUE(capturer.CaptureFrame());
-  ASSERT_EQ(1, fake_call_->GetNumCreatedSendStreams());
-  ASSERT_EQ(stream, fake_call_->GetVideoSendStreams().front());
-  EXPECT_EQ(3, stream->GetNumberOfSwappedFrames());
+  ASSERT_EQ(3, fake_call_->GetNumCreatedSendStreams());
+  stream = fake_call_->GetVideoSendStreams().front();
+  EXPECT_EQ(1, stream->GetNumberOfSwappedFrames());
 
   encoder_config = stream->GetEncoderConfig().Copy();
   EXPECT_EQ(webrtc::VideoEncoderConfig::ContentType::kRealtimeVideo,
             encoder_config.content_type);
 
   EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, nullptr));
 }
 
 TEST_F(WebRtcVideoChannel2Test,
        ConferenceModeScreencastConfiguresTemporalLayer) {
@@ skipping 161 matching lines @@
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options, &capturer));
   EXPECT_TRUE(capturer.CaptureFrame());
   // Expect 1 reconfigurations at this point from the initial configuration.
   EXPECT_EQ(1, send_stream->num_encoder_reconfigurations());
 
   // Set the options one more time and expect no additional reconfigurations.
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options, &capturer));
   EXPECT_EQ(1, send_stream->num_encoder_reconfigurations());
 
   // Change |options| and expect 2 reconfigurations.
-  options.is_screencast = rtc::Optional<bool>(true);
+  options.video_noise_reduction = rtc::Optional<bool>(true);
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, &options, &capturer));
   EXPECT_EQ(2, send_stream->num_encoder_reconfigurations());
 
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr));
 }
 
 class Vp9SettingsTest : public WebRtcVideoChannel2Test {
  public:
   Vp9SettingsTest() : Vp9SettingsTest("") {}
   explicit Vp9SettingsTest(const char* field_trials)
@@ skipping 250 matching lines @@
   ASSERT_TRUE(channel_->SetSend(true));
   cricket::VideoOptions camera_options;
   camera_options.is_screencast = rtc::Optional<bool>(false);
   channel_->SetVideoSend(last_ssrc_, true /* enable */, &camera_options,
                          &capturer);
 
   ASSERT_EQ(1u, fake_call_->GetVideoSendStreams().size());
   FakeVideoSendStream* send_stream = fake_call_->GetVideoSendStreams().front();
 
   EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
+  EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams());

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

This expectation doesn't seem necessary for this test

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

This one, no. It's already assert just above.

   EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
   EXPECT_EQ(1280, send_stream->GetLastWidth());
   EXPECT_EQ(720, send_stream->GetLastHeight());
 
   // Trigger overuse.
   rtc::VideoSinkWants wants;
   wants.max_pixel_count = rtc::Optional<int>(
       send_stream->GetLastWidth() * send_stream->GetLastHeight() - 1);
   send_stream->InjectVideoSinkWants(wants);
   EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
   EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
   EXPECT_EQ(1280 * 3 / 4, send_stream->GetLastWidth());
   EXPECT_EQ(720 * 3 / 4, send_stream->GetLastHeight());
 
   // Switch to screen share. Expect no CPU adaptation.
   cricket::FakeVideoCapturer screen_share(true);
   ASSERT_EQ(cricket::CS_RUNNING,
             screen_share.Start(screen_share.GetSupportedFormats()->front()));
   cricket::VideoOptions screenshare_options;
   screenshare_options.is_screencast = rtc::Optional<bool>(true);
   channel_->SetVideoSend(last_ssrc_, true /* enable */, &screenshare_options,
                          &screen_share);
   EXPECT_TRUE(screen_share.CaptureCustomFrame(1284, 724, cricket::FOURCC_I420));
-  EXPECT_EQ(3, send_stream->GetNumberOfSwappedFrames());
+  EXPECT_EQ(2, fake_call_->GetNumCreatedSendStreams());

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

Same here

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

I actually changed this to an assert, since we will need to do 
  send_stream = fake_call_->GetVideoSendStreams().front();
to make sure |send_stream| points the last stream create, probably the only
instance that is actually valid.

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

Acknowledged.

+  send_stream = fake_call_->GetVideoSendStreams().front();
+  EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
   EXPECT_EQ(1284, send_stream->GetLastWidth());
   EXPECT_EQ(724, send_stream->GetLastHeight());
 
   // Switch back to the normal capturer. Expect the frame to be CPU adapted.
   channel_->SetVideoSend(last_ssrc_, true /* enable */, &camera_options,
                          &capturer);
+  send_stream = fake_call_->GetVideoSendStreams().front();
+  // We have a new fake send stream, so it doesn't remember the old sink wants.
+  // In practice, it will be populated from
+  // ViEEncoder::VideoSourceProxy::SetSource(), so simulate that here.
+  send_stream->InjectVideoSinkWants(wants);
   EXPECT_TRUE(capturer.CaptureCustomFrame(1280, 720, cricket::FOURCC_I420));
-  EXPECT_EQ(4, send_stream->GetNumberOfSwappedFrames());
+  EXPECT_EQ(3, fake_call_->GetNumCreatedSendStreams());
+  EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
   EXPECT_EQ(1280 * 3 / 4, send_stream->GetLastWidth());
   EXPECT_EQ(720 * 3 / 4, send_stream->GetLastHeight());
 
   EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr));
 }
 
 void WebRtcVideoChannel2Test::TestCpuAdaptation(bool enable_overuse,
                                                 bool is_screenshare) {
   cricket::VideoCodec codec = GetEngineCodec("VP8");
   cricket::VideoSendParameters parameters;
@@ skipping 1949 matching lines @@
     parameters.conference_mode = conference_mode;
     if (screenshare) {
       options.is_screencast = rtc::Optional<bool>(screenshare);
     }
     ASSERT_TRUE(channel_->SetSendParameters(parameters));
 
     std::vector<uint32_t> ssrcs = MAKE_VECTOR(kSsrcs3);
     RTC_DCHECK(num_configured_streams <= ssrcs.size());
     ssrcs.resize(num_configured_streams);
 
-    FakeVideoSendStream* stream =
-        AddSendStream(CreateSimStreamParams("cname", ssrcs));
+    AddSendStream(CreateSimStreamParams("cname", ssrcs));
     // Send a full-size frame to trigger a stream reconfiguration to use all
     // expected simulcast layers.
     cricket::FakeVideoCapturer capturer;
     EXPECT_TRUE(
         channel_->SetVideoSend(ssrcs.front(), true, &options, &capturer));
+    // Fetch the new stream created for the screenshare content.

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

|screenshare| can be false, so this comment may be misleading.

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

Done.

+    FakeVideoSendStream* stream = fake_call_.GetVideoSendStreams().front();
     EXPECT_EQ(cricket::CS_RUNNING, capturer.Start(cricket::VideoFormat(
                                        capture_width, capture_height,
                                        cricket::VideoFormat::FpsToInterval(30),
                                        cricket::FOURCC_I420)));
     channel_->SetSend(true);
     EXPECT_TRUE(capturer.CaptureFrame());
 
     std::vector<webrtc::VideoStream> video_streams = stream->GetVideoStreams();
     ASSERT_EQ(expected_num_streams, video_streams.size());
 
@@ skipping 140 matching lines @@
 }
 
 TEST_F(WebRtcVideoChannel2SimulcastTest, SetSendCodecsForSimulcastScreenshare) {
   webrtc::test::ScopedFieldTrials override_field_trials_(
       "WebRTC-SimulcastScreenshare/Enabled/");
   VerifySimulcastSettings(cricket::VideoCodec("VP8"), 1280, 720, 3, 2, true,
                           true);
 }
 
 }  // namespace cricket