Recreate WebrtcVideoSendStream if screen content setting is changed.

webrtc/media/engine/webrtcvideoengine2_unittest.cc

Index: webrtc/media/engine/webrtcvideoengine2_unittest.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
index 5c560c159c54c9ecac926f2f18156551b736fabd..0f94ad8f743c1ffbfc57fed109b96c479dfd4173 100644
--- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -906,6 +906,60 @@ TEST_F(WebRtcVideoEngine2Test, StreamParamsIdPassedToDecoderFactory) {
   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(); }
 
@@ -1668,9 +1722,9 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) {
   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();
@@ -1686,7 +1740,7 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) {
   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)));
@@ -1707,15 +1761,15 @@ TEST_F(WebRtcVideoChannel2Test, NoRecreateStreamForScreencast) {
   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,
@@ -1726,9 +1780,9 @@ TEST_F(WebRtcVideoChannel2Test, NoRecreateStreamForScreencast) {
   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,
@@ -1910,7 +1964,7 @@ TEST_F(WebRtcVideoChannel2Test, SetIdenticalOptionsDoesntReconfigureEncoder) {
   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());
 
@@ -2181,6 +2235,7 @@ TEST_F(WebRtcVideoChannel2Test, PreviousAdaptationDoesNotApplyToScreenshare) {
   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());
@@ -2204,15 +2259,23 @@ TEST_F(WebRtcVideoChannel2Test, PreviousAdaptationDoesNotApplyToScreenshare) {
   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());
 
@@ -4182,13 +4245,14 @@ class WebRtcVideoChannel2SimulcastTest : public testing::Test {
     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),