Fix race in WebRtcVideoEngine2Tests, improve coverage.

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 584 matching lines @@
   EXPECT_TRUE(
       channel->AddSendStream(CreateSimStreamParams("cname", ssrcs)));
   EXPECT_TRUE(channel->SetSend(true));
 
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel->SetVideoSend(ssrcs.front(), true, nullptr, &capturer));
   EXPECT_EQ(cricket::CS_RUNNING,
             capturer.Start(capturer.GetSupportedFormats()->front()));
   EXPECT_TRUE(capturer.CaptureFrame());
 
-  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2));
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
 
   // Verify that encoders are configured for simulcast through adapter
   // (increasing resolution and only configured to send one stream each).
   int prev_width = -1;
   for (size_t i = 0; i < encoder_factory.encoders().size(); ++i) {
     ASSERT_TRUE(encoder_factory.encoders()[i]->WaitForInitEncode());
     webrtc::VideoCodec codec_settings =
         encoder_factory.encoders()[i]->GetCodecSettings();
     EXPECT_EQ(0, codec_settings.numberOfSimulcastStreams);
     EXPECT_GT(codec_settings.width, prev_width);
@@ skipping 66 matching lines @@
 
   // Send a fake frame, or else the media engine will configure the simulcast
   // encoder adapter at a low-enough size that it'll only create a single
   // encoder layer.
   cricket::FakeVideoCapturer capturer;
   EXPECT_TRUE(channel->SetVideoSend(ssrcs.front(), true, nullptr, &capturer));
   EXPECT_EQ(cricket::CS_RUNNING,
             capturer.Start(capturer.GetSupportedFormats()->front()));
   EXPECT_TRUE(capturer.CaptureFrame());
 
-  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2));
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
   ASSERT_TRUE(encoder_factory.encoders()[0]->WaitForInitEncode());
   EXPECT_EQ(webrtc::kVideoCodecVP8,
             encoder_factory.encoders()[0]->GetCodecSettings().codecType);
 
   channel.reset();
   // Make sure DestroyVideoEncoder was called on the factory.
   EXPECT_EQ(0u, encoder_factory.encoders().size());
 }
 
 TEST_F(WebRtcVideoEngine2Test,
@@ skipping 196 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, DISABLED_RecreatesEncoderOnContentTypeChange) {
+TEST_F(WebRtcVideoEngine2Test, ReconfiguresEncoderOnContentTypeChange) {
+  // This test uses actual webrtc streams, and verifies end-to-end that the
+  // encoder is updated with the new content type.
+  // The RecreatesEncoderOnContentTypeChange test below is similar but uses
+  // fake streams and verifies that new encoder instances are actually

[Taylor Brandstetter, 2017/03/02 04:53:07]

I think I'm confused about the various encoder objects... This test calls
WaitForCreatedVideoEncoders, so what encoder instances is it *not* verifying are
created? The simulcast encoder wrappers? And why does using fake streams make a
difference?

[sprang_webrtc, 2017/03/02 14:45:08]

Yes, this does not test the simulcast encoder adapter being created, which is
what I want. I really, what I wanted to test is which encoder is being
instantiated in WebRtcVideoSendStream::CreateVideoEncoder(), but that doesn't
seem to be directly testable. Instead we must go via an external encoder
factory, which is always wrapped in a simulcast encoder adapter.

So I'm leaving this as an integration test just to make sure that we configure
encoders of the right kind and don't trigger any dchecks in webrtc.

With the fake encoders below I set content type VP9, which doesn't have
simulcast and so no wrapper is created. That makes it possible to check for
direct allocations, and since we can also check how many send streams have been
created and how many times the have been reconfigured, we can check the exact
behavior we expect with regards to reconfigure vs recreate.

+  // created.
   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));
+  EXPECT_TRUE(encoder_factory.encoders().back()->WaitForInitEncode());
   EXPECT_EQ(webrtc::kRealtimeVideo,
             encoder_factory.encoders().back()->GetCodecSettings().mode);
 
-  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.
+  // Change to screen content, recreate encoder.
   ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2));
+  EXPECT_TRUE(encoder_factory.encoders().back()->WaitForInitEncode());
   EXPECT_EQ(webrtc::kScreensharing,
             encoder_factory.encoders().back()->GetCodecSettings().mode);
 
-  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);
   options.video_noise_reduction.emplace(false);
   EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
   // Change back to regular video content, update encoder. Also change
   // a non |is_screencast| option just to verify it doesn't affect recreation.
   EXPECT_TRUE(capturer.CaptureFrame());
   ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
+  EXPECT_TRUE(encoder_factory.encoders().back()->WaitForInitEncode());
   EXPECT_EQ(webrtc::kRealtimeVideo,
             encoder_factory.encoders().back()->GetCodecSettings().mode);
 
   // Remove stream previously added to free the external encoder instance.
   EXPECT_TRUE(channel->RemoveSendStream(kSsrc));
   EXPECT_EQ(0u, encoder_factory.encoders().size());
 }
 
+TEST_F(WebRtcVideoEngine2Test, RecreatesEncoderOnContentTypeChange) {
+  cricket::FakeWebRtcVideoEncoderFactory encoder_factory;
+  // Set another codec than VP8, so that the encoder factory won't be wrapped
+  // in a WebRtcSimulcastEncoderFactory, which prevents us from properly
+  // detecting the codec be recreated (encoder_factory won't be called until
+  // InitEncode in that case, and will be indistinguishable from just calling
+  // ReconfigureEncoder).
+  encoder_factory.AddSupportedVideoCodecType("VP9");
+  FakeCall fake_call((webrtc::Call::Config(&event_log_)));
+  call_.reset(&fake_call);
+  std::unique_ptr<VideoMediaChannel> channel(
+      SetUpForExternalEncoderFactory(&encoder_factory));
+  ASSERT_TRUE(
+      channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc)));
+  cricket::VideoCodec codec = GetEngineCodec("VP9");
+  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));
+  ASSERT_EQ(1, fake_call.GetNumCreatedSendStreams());
+  EXPECT_EQ(
+      1,
+      fake_call.GetVideoSendStreams().back()->num_encoder_reconfigurations());
+  EXPECT_EQ(
+      webrtc::VideoEncoderConfig::ContentType::kRealtimeVideo,
+      fake_call.GetVideoSendStreams().back()->GetEncoderConfig().content_type);
+  webrtc::VideoCodecVP9 vp9_settings;
+  EXPECT_TRUE(
+      fake_call.GetVideoSendStreams().back()->GetVp9Settings(&vp9_settings));
+  EXPECT_EQ(false, vp9_settings.denoisingOn);
+
+  // No change in content type, keep current encoder, but reinitialize it
+  // because of option change.
+  options.video_noise_reduction.emplace(true);
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  EXPECT_EQ(1, encoder_factory.GetNumCreatedEncoders());
+  ASSERT_EQ(1, fake_call.GetNumCreatedSendStreams());
+  EXPECT_EQ(
+      2,
+      fake_call.GetVideoSendStreams().back()->num_encoder_reconfigurations());
+  EXPECT_EQ(
+      webrtc::VideoEncoderConfig::ContentType::kRealtimeVideo,
+      fake_call.GetVideoSendStreams().back()->GetEncoderConfig().content_type);
+  EXPECT_TRUE(
+      fake_call.GetVideoSendStreams().back()->GetVp9Settings(&vp9_settings));
+  EXPECT_EQ(true, vp9_settings.denoisingOn);
+
+  // Change to screen content, recreate encoder, and update denoiser option.
+  options.is_screencast.emplace(true);
+  options.video_noise_reduction.emplace(false);
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2));
+  EXPECT_EQ(2, encoder_factory.GetNumCreatedEncoders());
+  ASSERT_EQ(2, fake_call.GetNumCreatedSendStreams());
+  // Encoder will get configured twice, once one recreating, and again when

[Taylor Brandstetter, 2017/03/02 04:53:07]

Did you mean to say "once when"? Also, does the encoder really *need* to be
configured twice, or should there be a TODO to optimize this?

[sprang_webrtc, 2017/03/02 14:45:07]

"once when", yes.

It doesn't technically need to. It's the case we discussed in the last cl
regarding if ReconfigureEncoder() is guaranteed to be a subset of SetCodec() or
not.
I can add a todo at the call site of that extraneous reconfiguration.

[Taylor Brandstetter, 2017/03/02 19:36:19]

That sounds good.

+  // applying the changed options (video_noise_reduction).
+  EXPECT_EQ(
+      2,
+      fake_call.GetVideoSendStreams().back()->num_encoder_reconfigurations());
+  EXPECT_EQ(
+      webrtc::VideoEncoderConfig::ContentType::kScreen,
+      fake_call.GetVideoSendStreams().back()->GetEncoderConfig().content_type);
+  EXPECT_TRUE(
+      fake_call.GetVideoSendStreams().back()->GetVp9Settings(&vp9_settings));
+  EXPECT_EQ(false, vp9_settings.denoisingOn);
+
+  // Change back to regular video content, update encoder. Don't change
+  // denoiser option.
+  options.is_screencast.emplace(false);
+  EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, &options, &capturer));
+  EXPECT_TRUE(capturer.CaptureFrame());
+  ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(3));
+  EXPECT_EQ(3, encoder_factory.GetNumCreatedEncoders());
+  ASSERT_EQ(3, fake_call.GetNumCreatedSendStreams());
+  EXPECT_EQ(
+      1,
+      fake_call.GetVideoSendStreams().back()->num_encoder_reconfigurations());
+  EXPECT_EQ(
+      webrtc::VideoEncoderConfig::ContentType::kRealtimeVideo,
+      fake_call.GetVideoSendStreams().back()->GetEncoderConfig().content_type);
+  EXPECT_TRUE(
+      fake_call.GetVideoSendStreams().back()->GetVp9Settings(&vp9_settings));
+  EXPECT_EQ(false, vp9_settings.denoisingOn);
+
+  // Remove stream previously added to free the external encoder instance.
+  EXPECT_TRUE(channel->RemoveSendStream(kSsrc));
+  EXPECT_EQ(0u, encoder_factory.encoders().size());
+  call_.release();
+}
+
 #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 3385 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