From 86196c4f481d7f515e54806988f763169e8b9206 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Mon, 16 Feb 2015 21:02:00 +0000 Subject: [PATCH] Setup encoders inexpensively before first frame. Modifies WebRtcVideoSendStream to use a default width/height of 16px. This significantly reduces SetRemoteDescription time under WebRtcVideoEngine2. Also preventing (expensive) reconfigurations due to incoming frames when the channel is not sending yet. Tests have been modified to generate a frame before expecting a certain encoder size to have been configured. Also adding tracing to WebRtcVideoSendStream::InputFrame as it can lead to reconfigurations of the encoder which is expensive and it should show up in chrome://tracing. BUG=1788 R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/42369004 Cr-Commit-Position: refs/heads/master@{#8381} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8381 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/webrtc/webrtcvideoengine2.cc | 15 ++++--- talk/media/webrtc/webrtcvideoengine2.h | 4 +- .../webrtc/webrtcvideoengine2_unittest.cc | 44 ++++++++++++++++--- .../codecs/vp8/simulcast_encoder_adapter.cc | 5 +++ 4 files changed, 55 insertions(+), 13 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 4ad0d31ee..cb025f268 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -1396,6 +1396,7 @@ static void ConvertToI420VideoFrame(const VideoFrame& frame, void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( VideoCapturer* capturer, const VideoFrame* frame) { + TRACE_EVENT0("webrtc", "WebRtcVideoSendStream::InputFrame"); LOG(LS_VERBOSE) << "InputFrame: " << frame->GetWidth() << "x" << frame->GetHeight(); // Lock before copying, can be called concurrently when swapping input source. @@ -1408,6 +1409,12 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( "configured, dropping."; return; } + + // Not sending, abort early to prevent expensive reconfigurations while + // setting up codecs etc. + if (!sending_) + return; + if (format_.width == 0) { // Dropping frames. assert(format_.height == 0); LOG(LS_VERBOSE) << "VideoFormat 0x0 set, Dropping frame."; @@ -1585,16 +1592,10 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::DestroyVideoEncoder( void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions( const VideoCodecSettings& codec_settings, const VideoOptions& options) { - if (last_dimensions_.width == -1) { - last_dimensions_.width = codec_settings.codec.width; - last_dimensions_.height = codec_settings.codec.height; - last_dimensions_.is_screencast = false; - } parameters_.encoder_config = CreateVideoEncoderConfig(last_dimensions_, codec_settings.codec); - if (parameters_.encoder_config.streams.empty()) { + if (parameters_.encoder_config.streams.empty()) return; - } format_ = VideoFormat(codec_settings.codec.width, codec_settings.codec.height, diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index 35ef592dc..d94c1bb48 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -319,7 +319,9 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, }; struct Dimensions { - Dimensions() : width(-1), height(-1), is_screencast(false) {} + // Use low width/height to make encoder creation (before first frame) + // cheap. + Dimensions() : width(16), height(16), is_screencast(false) {} int width; int height; bool is_screencast; diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 061126da0..d9eb7d596 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -572,13 +572,18 @@ TEST_F(WebRtcVideoEngine2Test, UseExternalFactoryForVp8WhenSupported) { EXPECT_EQ(cricket::CS_RUNNING, capturer.Start(capturer.GetSupportedFormats()->front())); EXPECT_TRUE(capturer.CaptureFrame()); - EXPECT_TRUE_WAIT(encoder_factory.encoders()[0]->GetNumEncodedFrames() > 0, kTimeout); - // Setting codecs of the same type should not reallocate the encoder. + // Sending one frame will have reallocated the encoder since input size + // changes from a small default to the actual frame width/height. + int num_created_encoders = encoder_factory.GetNumCreatedEncoders(); + EXPECT_EQ(num_created_encoders, 2); + + // Setting codecs of the same type should not reallocate any encoders + // (expecting a no-op). EXPECT_TRUE(channel->SetSendCodecs(codecs)); - EXPECT_EQ(1, encoder_factory.GetNumCreatedEncoders()); + EXPECT_EQ(num_created_encoders, encoder_factory.GetNumCreatedEncoders()); // Remove stream previously added to free the external encoder instance. EXPECT_TRUE(channel->RemoveSendStream(kSsrc)); @@ -626,6 +631,12 @@ TEST_F(WebRtcVideoEngine2Test, UsesSimulcastAdapterForVp8Factories) { channel->AddSendStream(CreateSimStreamParams("cname", ssrcs))); EXPECT_TRUE(channel->SetSend(true)); + cricket::FakeVideoCapturer capturer; + EXPECT_TRUE(channel->SetCapturer(ssrcs.front(), &capturer)); + EXPECT_EQ(cricket::CS_RUNNING, + capturer.Start(capturer.GetSupportedFormats()->front())); + EXPECT_TRUE(capturer.CaptureFrame()); + EXPECT_GT(encoder_factory.encoders().size(), 1u); // Verify that encoders are configured for simulcast through adapter @@ -638,6 +649,8 @@ TEST_F(WebRtcVideoEngine2Test, UsesSimulcastAdapterForVp8Factories) { EXPECT_GT(codec_settings.width, prev_width); prev_width = codec_settings.width; } + + EXPECT_TRUE(channel->SetCapturer(ssrcs.front(), NULL)); } TEST_F(WebRtcVideoEngine2Test, ChannelWithExternalH264CanChangeToInternalVp8) { @@ -1728,8 +1741,17 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsChangesExistingStreams) { std::vector codecs; codecs.push_back(kVp8Codec720p); ASSERT_TRUE(channel_->SetSendCodecs(codecs)); + channel_->SetSend(true); - std::vector streams = AddSendStream()->GetVideoStreams(); + FakeVideoSendStream* stream = AddSendStream(); + + cricket::FakeVideoCapturer capturer; + EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, &capturer)); + EXPECT_EQ(cricket::CS_RUNNING, + capturer.Start(capturer.GetSupportedFormats()->front())); + EXPECT_TRUE(capturer.CaptureFrame()); + + std::vector streams = stream->GetVideoStreams(); EXPECT_EQ(kVp8Codec720p.width, streams[0].width); EXPECT_EQ(kVp8Codec720p.height, streams[0].height); @@ -1739,6 +1761,7 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsChangesExistingStreams) { streams = fake_call_->GetVideoSendStreams()[0]->GetVideoStreams(); EXPECT_EQ(kVp8Codec360p.width, streams[0].width); EXPECT_EQ(kVp8Codec360p.height, streams[0].height); + EXPECT_TRUE(channel_->SetCapturer(last_ssrc_, NULL)); } TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithBitrates) { @@ -2078,7 +2101,7 @@ class WebRtcVideoEngine2SimulcastTest : public testing::Test { }; class WebRtcVideoChannel2SimulcastTest : public WebRtcVideoEngine2SimulcastTest, - public WebRtcCallFactory { + public WebRtcCallFactory { public: WebRtcVideoChannel2SimulcastTest() : fake_call_(NULL) {} @@ -2117,6 +2140,16 @@ class WebRtcVideoChannel2SimulcastTest : public WebRtcVideoEngine2SimulcastTest, FakeVideoSendStream* stream = 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_->SetCapturer(ssrcs.front(), &capturer)); + EXPECT_EQ(cricket::CS_RUNNING, capturer.Start(cricket::VideoFormat( + codec.width, codec.height, + cricket::VideoFormat::FpsToInterval(30), + cricket::FOURCC_I420))); + channel_->SetSend(true); + EXPECT_TRUE(capturer.CaptureFrame()); std::vector video_streams = stream->GetVideoStreams(); ASSERT_EQ(expected_num_streams, video_streams.size()); @@ -2160,6 +2193,7 @@ class WebRtcVideoChannel2SimulcastTest : public WebRtcVideoEngine2SimulcastTest, EXPECT_EQ(expected_streams[i].temporal_layer_thresholds_bps, video_streams[i].temporal_layer_thresholds_bps); } + EXPECT_TRUE(channel_->SetCapturer(ssrcs.front(), NULL)); } FakeVideoSendStream* AddSendStream() { diff --git a/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc b/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc index 15b6023d9..403541246 100644 --- a/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc +++ b/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc @@ -126,6 +126,11 @@ SimulcastEncoderAdapter::~SimulcastEncoderAdapter() { } int SimulcastEncoderAdapter::Release() { + // TODO(pbos): Keep the last encoder instance but call ::Release() on it, then + // re-use this instance in ::InitEncode(). This means that changing + // resolutions doesn't require reallocation of the first encoder, but only + // reinitialization, which makes sense. Then Destroy this instance instead in + // ~SimulcastEncoderAdapter(). while (!streaminfos_.empty()) { VideoEncoder* encoder = streaminfos_.back().encoder; factory_->Destroy(encoder);