Reland of Turn off error resilience for VP9 if no spatial or temporal layers are configured and NACK is enabl… (patchset #1 id:1 of https://codereview.webrtc.org/2995173002/ )

Reason for revert:
Speculative revert didn't help, see for the actual reason https://bugs.chromium.org/p/chromium/issues/detail?id=756741.

Original issue's description:
> Revert of Turn off error resilience for VP9 if no spatial or temporal layers are configured and NACK is enabl… (patchset #2 id:20001 of https://codereview.webrtc.org/2925253002/ )
>
> Reason for revert:
> Failing WebRtcVideoQualityBrowserTest.MANUAL_TestVideoQualityVp* tests.
>
> Mac #19383-19392
> https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds/42197
> Win8 #19383-19385
> https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win8%20Tester/builds/1496
> Win7 #19383-19385
> https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/builds/9807
> Win10 #19383-19385
> https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win10%20Tester/builds/8452
>
> Original issue's description:
> > Turn off error resilience for VP9 if no spatial or temporal layers are configured and NACK is enabled.
> >
> > Error resilience is currently always enabled for VP9 which reduces quality.
> >
> > Reland of https://codereview.webrtc.org/2532053002
> >
> > BUG=webrtc:6783
> >
> > Review-Url: https://codereview.webrtc.org/2925253002
> > Cr-Commit-Position: refs/heads/master@{#19385}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/6b463faccbf145b54b8fb2666bfeab868256df08
>
> [email protected],[email protected]
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6783
>
> Review-Url: https://codereview.webrtc.org/2995173002
> Cr-Commit-Position: refs/heads/master@{#19399}
> Committed: https://chromium.googlesource.com/external/webrtc/+/7b532db9ad08a86678c85b67179b3c444ee0a8b2

[email protected],[email protected]
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=webrtc:6783

Review-Url: https://codereview.webrtc.org/3002933002
Cr-Commit-Position: refs/heads/master@{#19402}

Author: uysalere

webrtc/modules/video_coding/video_codec_initializer.cc

@@ -22,6 +22,15 @@
 #include "webrtc/system_wrappers/include/clock.h"
 
 namespace webrtc {
+namespace {
+bool TemporalLayersConfigured(const std::vector<VideoStream>& streams) {
+  for (const VideoStream& stream : streams) {
+    if (stream.temporal_layer_thresholds_bps.size() > 0)
+      return true;
+  }
+  return false;
+}
+}  // namespace
 
 bool VideoCodecInitializer::SetupCodec(
     const VideoEncoderConfig& config,
@@ -121,12 +130,8 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec(
         *video_codec.VP8() = VideoEncoder::GetDefaultVp8Settings();
       video_codec.VP8()->numberOfTemporalLayers = static_cast<unsigned char>(
           streams.back().temporal_layer_thresholds_bps.size() + 1);
-      bool temporal_layers_configured = false;
-      for (const VideoStream& stream : streams) {
-        if (stream.temporal_layer_thresholds_bps.size() > 0)
-          temporal_layers_configured = true;
-      }
-      if (nack_enabled && !temporal_layers_configured) {
+
+      if (nack_enabled && !TemporalLayersConfigured(streams)) {
         LOG(LS_INFO) << "No temporal layers and nack enabled -> resilience off";
         video_codec.VP8()->resilience = kResilienceOff;
       }
@@ -144,6 +149,13 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec(
       }
       video_codec.VP9()->numberOfTemporalLayers = static_cast<unsigned char>(
           streams.back().temporal_layer_thresholds_bps.size() + 1);
+
+      if (nack_enabled && !TemporalLayersConfigured(streams) &&
+          video_codec.VP9()->numberOfSpatialLayers == 1) {
+        LOG(LS_INFO) << "No temporal or spatial layers and nack enabled -> "
+                     << "resilience off";
+        video_codec.VP9()->resilienceOn = false;
+      }
       break;
     }
     case kVideoCodecH264: {

webrtc/video/video_stream_encoder_unittest.cc

@@ -32,6 +32,7 @@ namespace {
 const int kMinPixelsPerFrame = 320 * 180;
 const int kMinFramerateFps = 2;
 const int64_t kFrameTimeoutMs = 100;
+const unsigned char kNumSlDummy = 0;
 }  // namespace
 
 namespace webrtc {
@@ -315,6 +316,7 @@ class VideoStreamEncoderTest : public ::testing::Test {
   void ResetEncoder(const std::string& payload_name,
                     size_t num_streams,
                     size_t num_temporal_layers,
+                    unsigned char num_spatial_layers,
                     bool nack_enabled,
                     bool screenshare) {
     video_send_config_.encoder_settings.payload_name = payload_name;
@@ -328,6 +330,13 @@ class VideoStreamEncoderTest : public ::testing::Test {
     video_encoder_config.content_type =
         screenshare ? VideoEncoderConfig::ContentType::kScreen
                     : VideoEncoderConfig::ContentType::kRealtimeVideo;
+    if (payload_name == "VP9") {
+      VideoCodecVP9 vp9_settings = VideoEncoder::GetDefaultVp9Settings();
+      vp9_settings.numberOfSpatialLayers = num_spatial_layers;
+      video_encoder_config.encoder_specific_settings =
+          new rtc::RefCountedObject<
+              VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);
+    }
     ConfigureEncoder(std::move(video_encoder_config), nack_enabled);
   }
 
@@ -797,7 +806,7 @@ TEST_F(VideoStreamEncoderTest, Vp8ResilienceIsOffFor1S1TLWithNackEnabled) {
   const bool kNackEnabled = true;
   const size_t kNumStreams = 1;
   const size_t kNumTl = 1;
-  ResetEncoder("VP8", kNumStreams, kNumTl, kNackEnabled, false);
+  ResetEncoder("VP8", kNumStreams, kNumTl, kNumSlDummy, kNackEnabled, false);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
 
   // Capture a frame and wait for it to synchronize with the encoder thread.
@@ -817,7 +826,7 @@ TEST_F(VideoStreamEncoderTest, Vp8ResilienceIsOffFor2S1TlWithNackEnabled) {
   const bool kNackEnabled = true;
   const size_t kNumStreams = 2;
   const size_t kNumTl = 1;
-  ResetEncoder("VP8", kNumStreams, kNumTl, kNackEnabled, false);
+  ResetEncoder("VP8", kNumStreams, kNumTl, kNumSlDummy, kNackEnabled, false);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
 
   // Capture a frame and wait for it to synchronize with the encoder thread.
@@ -837,7 +846,7 @@ TEST_F(VideoStreamEncoderTest, Vp8ResilienceIsOnFor1S1TLWithNackDisabled) {
   const bool kNackEnabled = false;
   const size_t kNumStreams = 1;
   const size_t kNumTl = 1;
-  ResetEncoder("VP8", kNumStreams, kNumTl, kNackEnabled, false);
+  ResetEncoder("VP8", kNumStreams, kNumTl, kNumSlDummy, kNackEnabled, false);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
 
   // Capture a frame and wait for it to synchronize with the encoder thread.
@@ -857,7 +866,7 @@ TEST_F(VideoStreamEncoderTest, Vp8ResilienceIsOnFor1S2TlWithNackEnabled) {
   const bool kNackEnabled = true;
   const size_t kNumStreams = 1;
   const size_t kNumTl = 2;
-  ResetEncoder("VP8", kNumStreams, kNumTl, kNackEnabled, false);
+  ResetEncoder("VP8", kNumStreams, kNumTl, kNumSlDummy, kNackEnabled, false);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
 
   // Capture a frame and wait for it to synchronize with the encoder thread.
@@ -873,6 +882,94 @@ TEST_F(VideoStreamEncoderTest, Vp8ResilienceIsOnFor1S2TlWithNackEnabled) {
   video_stream_encoder_->Stop();
 }
 
+TEST_F(VideoStreamEncoderTest, Vp9ResilienceIsOffFor1SL1TLWithNackEnabled) {
+  const bool kNackEnabled = true;
+  const size_t kNumStreams = 1;
+  const size_t kNumTl = 1;
+  const unsigned char kNumSl = 1;
+  ResetEncoder("VP9", kNumStreams, kNumTl, kNumSl, kNackEnabled, false);
+  video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
+
+  // Capture a frame and wait for it to synchronize with the encoder thread.
+  video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr));
+  sink_.WaitForEncodedFrame(1);
+  // The encoder have been configured once when the first frame is received.
+  EXPECT_EQ(1, sink_.number_of_reconfigurations());
+  EXPECT_EQ(kVideoCodecVP9, fake_encoder_.codec_config().codecType);
+  EXPECT_EQ(kNumStreams, fake_encoder_.codec_config().numberOfSimulcastStreams);
+  EXPECT_EQ(kNumTl, fake_encoder_.codec_config().VP9()->numberOfTemporalLayers);
+  EXPECT_EQ(kNumSl, fake_encoder_.codec_config().VP9()->numberOfSpatialLayers);
+  // Resilience is off for no spatial and temporal layers with nack on.
+  EXPECT_FALSE(fake_encoder_.codec_config().VP9()->resilienceOn);
+  video_stream_encoder_->Stop();
+}
+
+TEST_F(VideoStreamEncoderTest, Vp9ResilienceIsOnFor1SL1TLWithNackDisabled) {
+  const bool kNackEnabled = false;
+  const size_t kNumStreams = 1;
+  const size_t kNumTl = 1;
+  const unsigned char kNumSl = 1;
+  ResetEncoder("VP9", kNumStreams, kNumTl, kNumSl, kNackEnabled, false);
+  video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
+
+  // Capture a frame and wait for it to synchronize with the encoder thread.
+  video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr));
+  sink_.WaitForEncodedFrame(1);
+  // The encoder have been configured once when the first frame is received.
+  EXPECT_EQ(1, sink_.number_of_reconfigurations());
+  EXPECT_EQ(kVideoCodecVP9, fake_encoder_.codec_config().codecType);
+  EXPECT_EQ(kNumStreams, fake_encoder_.codec_config().numberOfSimulcastStreams);
+  EXPECT_EQ(kNumTl, fake_encoder_.codec_config().VP9()->numberOfTemporalLayers);
+  EXPECT_EQ(kNumSl, fake_encoder_.codec_config().VP9()->numberOfSpatialLayers);
+  // Resilience is on if nack is off.
+  EXPECT_TRUE(fake_encoder_.codec_config().VP9()->resilienceOn);
+  video_stream_encoder_->Stop();
+}
+
+TEST_F(VideoStreamEncoderTest, Vp9ResilienceIsOnFor2SL1TLWithNackEnabled) {
+  const bool kNackEnabled = true;
+  const size_t kNumStreams = 1;
+  const size_t kNumTl = 1;
+  const unsigned char kNumSl = 2;
+  ResetEncoder("VP9", kNumStreams, kNumTl, kNumSl, kNackEnabled, false);
+  video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
+
+  // Capture a frame and wait for it to synchronize with the encoder thread.
+  video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr));
+  sink_.WaitForEncodedFrame(1);
+  // The encoder have been configured once when the first frame is received.
+  EXPECT_EQ(1, sink_.number_of_reconfigurations());
+  EXPECT_EQ(kVideoCodecVP9, fake_encoder_.codec_config().codecType);
+  EXPECT_EQ(kNumStreams, fake_encoder_.codec_config().numberOfSimulcastStreams);
+  EXPECT_EQ(kNumTl, fake_encoder_.codec_config().VP9()->numberOfTemporalLayers);
+  EXPECT_EQ(kNumSl, fake_encoder_.codec_config().VP9()->numberOfSpatialLayers);
+  // Resilience is on for spatial layers.
+  EXPECT_TRUE(fake_encoder_.codec_config().VP9()->resilienceOn);
+  video_stream_encoder_->Stop();
+}
+
+TEST_F(VideoStreamEncoderTest, Vp9ResilienceIsOnFor1SL2TLWithNackEnabled) {
+  const bool kNackEnabled = true;
+  const size_t kNumStreams = 1;
+  const size_t kNumTl = 2;
+  const unsigned char kNumSl = 1;
+  ResetEncoder("VP9", kNumStreams, kNumTl, kNumSl, kNackEnabled, false);
+  video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
+
+  // Capture a frame and wait for it to synchronize with the encoder thread.
+  video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr));
+  sink_.WaitForEncodedFrame(1);
+  // The encoder have been configured once when the first frame is received.
+  EXPECT_EQ(1, sink_.number_of_reconfigurations());
+  EXPECT_EQ(kVideoCodecVP9, fake_encoder_.codec_config().codecType);
+  EXPECT_EQ(kNumStreams, fake_encoder_.codec_config().numberOfSimulcastStreams);
+  EXPECT_EQ(kNumTl, fake_encoder_.codec_config().VP9()->numberOfTemporalLayers);
+  EXPECT_EQ(kNumSl, fake_encoder_.codec_config().VP9()->numberOfSpatialLayers);
+  // Resilience is on for temporal layers.
+  EXPECT_TRUE(fake_encoder_.codec_config().VP9()->resilienceOn);
+  video_stream_encoder_->Stop();
+}
+
 TEST_F(VideoStreamEncoderTest, SwitchSourceDeregisterEncoderAsSink) {
   EXPECT_TRUE(video_source_.has_sinks());
   test::FrameForwarder new_video_source;
@@ -2461,7 +2558,7 @@ TEST_F(VideoStreamEncoderTest,
 TEST_F(VideoStreamEncoderTest, FailingInitEncodeDoesntCauseCrash) {
   fake_encoder_.ForceInitEncodeFailure(true);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
-  ResetEncoder("VP8", 2, 1, true, false);
+  ResetEncoder("VP8", 2, 1, 1, true, false);
   const int kFrameWidth = 1280;
   const int kFrameHeight = 720;
   video_source_.IncomingCapturedFrame(
@@ -2608,7 +2705,7 @@ TEST_F(VideoStreamEncoderTest, DoesntAdaptDownPastMinFramerate) {
 
   // Reconfigure encoder with two temporal layers and screensharing, which will
   // disable frame dropping and make testing easier.
-  ResetEncoder("VP8", 1, 2, true, true);
+  ResetEncoder("VP8", 1, 2, 1, true, true);
 
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
   video_stream_encoder_->SetSource(