From bbcc356084e1434b357f0a7504445bdbff39100b Mon Sep 17 00:00:00 2001 From: emircan Date: Fri, 18 Aug 2017 00:28:40 -0700 Subject: [PATCH] =?UTF-8?q?Reland=20of=20Turn=20off=20error=20resilience?= =?UTF-8?q?=20for=20VP9=20if=20no=20spatial=20or=20temporal=20layers=20are?= =?UTF-8?q?=20configured=20and=20NACK=20is=20enabl=E2=80=A6=20(patchset=20?= =?UTF-8?q?#1=20id:1=20of=20https://codereview.webrtc.org/2995173002/=20)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 > > TBR=brandtr@webrtc.org,asapersson@webrtc.org > # 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 TBR=brandtr@webrtc.org,asapersson@webrtc.org # 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} --- .../video_coding/video_codec_initializer.cc | 24 +++- webrtc/video/video_stream_encoder_unittest.cc | 109 +++++++++++++++++- 2 files changed, 121 insertions(+), 12 deletions(-) diff --git a/webrtc/modules/video_coding/video_codec_initializer.cc b/webrtc/modules/video_coding/video_codec_initializer.cc index c759e84a36..df8f136541 100644 --- a/webrtc/modules/video_coding/video_codec_initializer.cc +++ b/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& 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( 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( 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: { diff --git a/webrtc/video/video_stream_encoder_unittest.cc b/webrtc/video/video_stream_encoder_unittest.cc index 6407bcb34a..9e7fa77b8f 100644 --- a/webrtc/video/video_stream_encoder_unittest.cc +++ b/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(