Reland Change ViEEncoder to not reconfigure the encoder until the video resolution is known. This is the second reland. Patchset 1 contains the reverted cl. Patchset 2 revert the change to initialize the encoder with resolution 1*1pixels if an internal source is used. This is to to fix the problem reported in https://codereview.webrtc.org/2457203002/ https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds/35251 remoting. Fix has been verified to work in Chrome. This reverts commit 05a55b500d83e4212d4e54f0fecf13097e782ffa. BUG=webrtc:6371 b/32285861 TBR=pbos@webrtc.org, skvlad@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/2458363002 . Cr-Commit-Position: refs/heads/master@{#14833}
diff --git a/webrtc/call/call_perf_tests.cc b/webrtc/call/call_perf_tests.cc index 5a60ace..13cec9c 100644 --- a/webrtc/call/call_perf_tests.cc +++ b/webrtc/call/call_perf_tests.cc
@@ -648,14 +648,16 @@ size_t max_payload_size) override { ++encoder_inits_; if (encoder_inits_ == 1) { - // First time initialization. Frame size is not known. - EXPECT_EQ(kInitialBitrateKbps, config->startBitrate) - << "Encoder not initialized at expected bitrate."; - } else if (encoder_inits_ == 2) { // First time initialization. Frame size is known. + // |expected_bitrate| is affected by bandwidth estimation before the + // first frame arrives to the encoder. + uint32_t expected_bitrate = + last_set_bitrate_ > 0 ? last_set_bitrate_ : kInitialBitrateKbps; + EXPECT_EQ(expected_bitrate, config->startBitrate) + << "Encoder not initialized at expected bitrate."; EXPECT_EQ(kDefaultWidth, config->width); EXPECT_EQ(kDefaultHeight, config->height); - } else if (encoder_inits_ == 3) { + } else if (encoder_inits_ == 2) { EXPECT_EQ(2 * kDefaultWidth, config->width); EXPECT_EQ(2 * kDefaultHeight, config->height); EXPECT_GE(last_set_bitrate_, kReconfigureThresholdKbps); @@ -671,7 +673,7 @@ int32_t SetRates(uint32_t new_target_bitrate_kbps, uint32_t framerate) override { last_set_bitrate_ = new_target_bitrate_kbps; - if (encoder_inits_ == 2 && + if (encoder_inits_ == 1 && new_target_bitrate_kbps > kReconfigureThresholdKbps) { time_to_reconfigure_.Set(); } @@ -690,6 +692,7 @@ std::vector<VideoReceiveStream::Config>* receive_configs, VideoEncoderConfig* encoder_config) override { send_config->encoder_settings.encoder = this; + encoder_config->max_bitrate_bps = 2 * kReconfigureThresholdKbps * 1000; encoder_config->video_stream_factory = new rtc::RefCountedObject<VideoStreamFactory>();
diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 81bb1d3..5d7b2b3 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -344,24 +344,20 @@ EXPECT_TRUE( channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc))); - ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(1)); - ASSERT_EQ(1u, encoder_factory.encoders().size()); + EXPECT_EQ(0, encoder_factory.GetNumCreatedEncoders()); 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()); - // Sending one frame will have reallocated the encoder since input size - // changes from a small default to the actual frame width/height. Wait for - // that to happen then for the frame to be sent. - ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(2)); + // Sending one frame will have allocate the encoder. + ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(1)); EXPECT_TRUE_WAIT(encoder_factory.encoders()[0]->GetNumEncodedFrames() > 0, kTimeout); int num_created_encoders = encoder_factory.GetNumCreatedEncoders(); - EXPECT_EQ(num_created_encoders, 2); + EXPECT_EQ(num_created_encoders, 1); // Setting codecs of the same type should not reallocate any encoders // (expecting a no-op). @@ -669,6 +665,14 @@ EXPECT_TRUE( channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc))); ASSERT_EQ(1u, encoder_factory.encoders().size()); + + // Send a frame of 720p. This should trigger a "real" encoder initialization. + cricket::VideoFormat format( + 1280, 720, cricket::VideoFormat::FpsToInterval(30), cricket::FOURCC_I420); + cricket::FakeVideoCapturer capturer; + EXPECT_TRUE(channel->SetVideoSend(kSsrc, true, nullptr, &capturer)); + EXPECT_EQ(cricket::CS_RUNNING, capturer.Start(format)); + EXPECT_TRUE(capturer.CaptureFrame()); ASSERT_TRUE(encoder_factory.encoders()[0]->WaitForInitEncode()); EXPECT_EQ(webrtc::kVideoCodecH264, encoder_factory.encoders()[0]->GetCodecSettings().codecType);
diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 9d00e00..2663516 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc
@@ -1785,12 +1785,9 @@ void PerformTest() override { EXPECT_TRUE(Wait()) << "Timed out while waiting for Encode."; - // Expect |num_releases| == 1 since the encoder has been reconfigured - // once when the first frame is encoded. Not until at that point is the - // frame size known and the encoder can be properly initialized. - EXPECT_EQ(1u, num_releases()); + EXPECT_EQ(0u, num_releases()); stream_->ReconfigureVideoEncoder(std::move(encoder_config_)); - EXPECT_EQ(1u, num_releases()); + EXPECT_EQ(0u, num_releases()); stream_->Stop(); // Encoder should not be released before destroying the VideoSendStream. EXPECT_FALSE(IsReleased()); @@ -1812,7 +1809,7 @@ RunBaseTest(&test_encoder); EXPECT_TRUE(test_encoder.IsReleased()); - EXPECT_EQ(2u, test_encoder.num_releases()); + EXPECT_EQ(1u, test_encoder.num_releases()); } TEST_F(VideoSendStreamTest, EncoderSetupPropagatesCommonEncoderConfigValues) { @@ -1844,7 +1841,7 @@ int32_t InitEncode(const VideoCodec* config, int32_t number_of_cores, size_t max_payload_size) override { - if (num_initializations_ < 2) { + if (num_initializations_ == 0) { // Verify default values. EXPECT_EQ(kRealtimeVideo, config->mode); } else { @@ -1852,23 +1849,18 @@ EXPECT_EQ(kScreensharing, config->mode); } ++num_initializations_; - if (num_initializations_ > 1) { - // Skip the first two InitEncode events: one with QCIF resolution when - // the SendStream is created, the other with QVGA when the first frame - // is encoded. - init_encode_event_.Set(); - } + init_encode_event_.Set(); return FakeEncoder::InitEncode(config, number_of_cores, max_payload_size); } void PerformTest() override { EXPECT_TRUE(init_encode_event_.Wait(kDefaultTimeoutMs)); - EXPECT_EQ(2u, num_initializations_) << "VideoEncoder not initialized."; + EXPECT_EQ(1u, num_initializations_) << "VideoEncoder not initialized."; encoder_config_.content_type = VideoEncoderConfig::ContentType::kScreen; stream_->ReconfigureVideoEncoder(std::move(encoder_config_)); EXPECT_TRUE(init_encode_event_.Wait(kDefaultTimeoutMs)); - EXPECT_EQ(3u, num_initializations_) + EXPECT_EQ(2u, num_initializations_) << "ReconfigureVideoEncoder did not reinitialize the encoder with " "new encoder settings."; } @@ -1945,12 +1937,7 @@ EXPECT_EQ(video_codec_type_, config->codecType); VerifyCodecSpecifics(*config); ++num_initializations_; - if (num_initializations_ > 1) { - // Skip the first two InitEncode events: one with QCIF resolution when - // the SendStream is created, the other with QVGA when the first frame is - // encoded. - init_encode_event_.Set(); - } + init_encode_event_.Set(); return FakeEncoder::InitEncode(config, number_of_cores, max_payload_size); } @@ -1961,14 +1948,14 @@ void PerformTest() override { EXPECT_TRUE( init_encode_event_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); - ASSERT_EQ(2u, num_initializations_) << "VideoEncoder not initialized."; + ASSERT_EQ(1u, num_initializations_) << "VideoEncoder not initialized."; encoder_settings_.frameDroppingOn = true; encoder_config_.encoder_specific_settings = GetEncoderSpecificSettings(); stream_->ReconfigureVideoEncoder(std::move(encoder_config_)); ASSERT_TRUE( init_encode_event_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); - EXPECT_EQ(3u, num_initializations_) + EXPECT_EQ(2u, num_initializations_) << "ReconfigureVideoEncoder did not reinitialize the encoder with " "new encoder settings."; } @@ -2215,8 +2202,7 @@ size_t maxPayloadSize) override { EXPECT_GE(codecSettings->startBitrate, codecSettings->minBitrate); EXPECT_LE(codecSettings->startBitrate, codecSettings->maxBitrate); - // First reinitialization happens due to that the frame size is updated. - if (num_initializations_ == 0 || num_initializations_ == 1) { + if (num_initializations_ == 0) { EXPECT_EQ(static_cast<unsigned int>(kMinBitrateKbps), codecSettings->minBitrate); EXPECT_EQ(static_cast<unsigned int>(kStartBitrateKbps), @@ -2224,14 +2210,14 @@ EXPECT_EQ(static_cast<unsigned int>(kMaxBitrateKbps), codecSettings->maxBitrate); observation_complete_.Set(); - } else if (num_initializations_ == 2) { + } else if (num_initializations_ == 1) { EXPECT_EQ(static_cast<unsigned int>(kLowerMaxBitrateKbps), codecSettings->maxBitrate); // The start bitrate should be kept (-1) and capped to the max bitrate. // Since this is not an end-to-end call no receiver should have been // returning a REMB that could lower this estimate. EXPECT_EQ(codecSettings->startBitrate, codecSettings->maxBitrate); - } else if (num_initializations_ == 3) { + } else if (num_initializations_ == 2) { EXPECT_EQ(static_cast<unsigned int>(kIncreasedMaxBitrateKbps), codecSettings->maxBitrate); // The start bitrate will be whatever the rate BitRateController @@ -2239,9 +2225,8 @@ // bitrate. } ++num_initializations_; - if (num_initializations_ > 1) { - init_encode_event_.Set(); - } + init_encode_event_.Set(); + return FakeEncoder::InitEncode(codecSettings, numberOfCores, maxPayloadSize); } @@ -2334,7 +2319,7 @@ send_stream_->ReconfigureVideoEncoder(encoder_config_.Copy()); ASSERT_TRUE( init_encode_event_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); - EXPECT_EQ(3, num_initializations_) + EXPECT_EQ(2, num_initializations_) << "Encoder should have been reconfigured with the new value."; WaitForSetRates(kLowerMaxBitrateKbps); @@ -2342,7 +2327,7 @@ send_stream_->ReconfigureVideoEncoder(encoder_config_.Copy()); ASSERT_TRUE( init_encode_event_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); - EXPECT_EQ(4, num_initializations_) + EXPECT_EQ(3, num_initializations_) << "Encoder should have been reconfigured with the new value."; // Expected target bitrate is the start bitrate set in the call to // call_->SetBitrateConfig.
diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc index a3fd98d..d09e40b 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc
@@ -410,15 +410,14 @@ pending_encoder_reconfiguration_ = true; // Reconfigure the encoder now if the encoder has an internal source or - // if this is the first time the encoder is configured. - // Otherwise, the reconfiguration is deferred until the next frame to minimize - // the number of reconfigurations. The codec configuration depends on incoming - // video frame size. - if (!last_frame_info_ || settings_.internal_source) { - if (!last_frame_info_) { - last_frame_info_ = rtc::Optional<VideoFrameInfo>( - VideoFrameInfo(176, 144, kVideoRotation_0, false)); - } + // if the frame resolution is known. Otherwise, the reconfiguration is + // deferred until the next frame to minimize the number of reconfigurations. + // The codec configuration depends on incoming video frame size. + if (last_frame_info_) { + ReconfigureEncoder(); + } else if (settings_.internal_source) { + last_frame_info_ = rtc::Optional<VideoFrameInfo>( + VideoFrameInfo(176, 144, kVideoRotation_0, false)); ReconfigureEncoder(); } } @@ -542,7 +541,7 @@ if (pre_encode_callback_) pre_encode_callback_->OnFrame(video_frame); - if (video_frame.width() != last_frame_info_->width || + if (!last_frame_info_ || video_frame.width() != last_frame_info_->width || video_frame.height() != last_frame_info_->height || video_frame.rotation() != last_frame_info_->rotation || video_frame.is_texture() != last_frame_info_->is_texture) {
diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h index 68f043e..33caec4 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h
@@ -114,7 +114,7 @@ is_texture(is_texture) {} int width; int height; - webrtc::VideoRotation rotation; + VideoRotation rotation; bool is_texture; };
diff --git a/webrtc/video/vie_encoder_unittest.cc b/webrtc/video/vie_encoder_unittest.cc index 5382fbf..78eb62f 100644 --- a/webrtc/video/vie_encoder_unittest.cc +++ b/webrtc/video/vie_encoder_unittest.cc
@@ -286,13 +286,14 @@ TEST_F(ViEEncoderTest, ConfigureEncoderTriggersOnEncoderConfigurationChanged) { const int kTargetBitrateBps = 100000; vie_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0); + EXPECT_EQ(0, sink_.number_of_reconfigurations()); // Capture a frame and wait for it to synchronize with the encoder thread. video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr)); sink_.WaitForEncodedFrame(1); - // The encoder will have been configured twice. First time before the first - // frame has been received. Then a second time when the resolution is known. - EXPECT_EQ(2, sink_.number_of_reconfigurations()); + // The encoder will have been configured once when the first frame is + // received. + EXPECT_EQ(1, sink_.number_of_reconfigurations()); VideoEncoderConfig video_encoder_config; test::FillEncoderConfiguration(1, &video_encoder_config); @@ -302,7 +303,7 @@ // Capture a frame and wait for it to synchronize with the encoder thread. video_source_.IncomingCapturedFrame(CreateFrame(2, nullptr)); sink_.WaitForEncodedFrame(2); - EXPECT_EQ(3, sink_.number_of_reconfigurations()); + EXPECT_EQ(2, sink_.number_of_reconfigurations()); EXPECT_EQ(9999, sink_.last_min_transmit_bitrate()); vie_encoder_->Stop(); @@ -315,9 +316,8 @@ // Capture a frame and wait for it to synchronize with the encoder thread. video_source_.IncomingCapturedFrame(CreateFrame(1, nullptr)); sink_.WaitForEncodedFrame(1); - // The encoder will have been configured twice. First time before the first - // frame has been received. Then a second time when the resolution is known. - EXPECT_EQ(2, sink_.number_of_reconfigurations()); + // The encoder will have been configured once. + EXPECT_EQ(1, sink_.number_of_reconfigurations()); EXPECT_EQ(codec_width_, fake_encoder_.codec_config().width); EXPECT_EQ(codec_height_, fake_encoder_.codec_config().height); @@ -329,7 +329,7 @@ sink_.WaitForEncodedFrame(2); EXPECT_EQ(codec_width_, fake_encoder_.codec_config().width); EXPECT_EQ(codec_height_, fake_encoder_.codec_config().height); - EXPECT_EQ(3, sink_.number_of_reconfigurations()); + EXPECT_EQ(2, sink_.number_of_reconfigurations()); vie_encoder_->Stop(); }