From d533aec3b8425edd81c2743f7683c12b74c307b5 Mon Sep 17 00:00:00 2001 From: perkj Date: Fri, 13 Jan 2017 05:57:25 -0800 Subject: [PATCH] Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame on source removal. BUG=webrtc:6371,webrtc:6983 Review-Url: https://codereview.webrtc.org/2469993003 Cr-Commit-Position: refs/heads/master@{#16048} --- webrtc/media/base/videoengine_unittest.h | 4 +- webrtc/media/engine/webrtcvideoengine2.cc | 116 ++++-------------- webrtc/media/engine/webrtcvideoengine2.h | 39 ++---- .../engine/webrtcvideoengine2_unittest.cc | 5 +- 4 files changed, 32 insertions(+), 132 deletions(-) diff --git a/webrtc/media/base/videoengine_unittest.h b/webrtc/media/base/videoengine_unittest.h index a673d3553a..a571bbc4bf 100644 --- a/webrtc/media/base/videoengine_unittest.h +++ b/webrtc/media/base/videoengine_unittest.h @@ -830,14 +830,12 @@ class VideoMediaChannelTest : public testing::Test, rtc::Thread::Current()->ProcessMessages(30); // Remove the capturer. EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, nullptr)); - // Wait for one black frame for removing the capturer. - EXPECT_FRAME_WAIT(2, kVideoWidth, kVideoHeight, kTimeout); // No capturer was added, so this SetVideoSend shouldn't do anything. EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, nullptr)); rtc::Thread::Current()->ProcessMessages(300); // Verify no more frames were sent. - EXPECT_EQ(2, renderer_.num_rendered_frames()); + EXPECT_EQ(1, renderer_.num_rendered_frames()); } // Tests that we can add and remove capturer as unique sources. diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index f09b2541d9..ca65e6541e 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1553,8 +1553,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( parameters_(std::move(config), options, max_bitrate_bps, codec_settings), rtp_parameters_(CreateRtpParametersWithOneEncoding()), allocated_encoder_(nullptr, cricket::VideoCodec(), false), - sending_(false), - last_frame_timestamp_us_(0) { + sending_(false) { parameters_.config.rtp.max_packet_size = kVideoMtu; parameters_.conference_mode = send_params.conference_mode; @@ -1612,43 +1611,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() { DestroyVideoEncoder(&allocated_encoder_); } -void WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame( - const webrtc::VideoFrame& frame) { - TRACE_EVENT0("webrtc", "WebRtcVideoSendStream::OnFrame"); - webrtc::VideoFrame video_frame(frame.video_frame_buffer(), - frame.rotation(), - frame.timestamp_us()); - - rtc::CritScope cs(&lock_); - - if (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) { - last_frame_info_.width = video_frame.width(); - last_frame_info_.height = video_frame.height(); - last_frame_info_.rotation = video_frame.rotation(); - last_frame_info_.is_texture = video_frame.is_texture(); - - LOG(LS_INFO) << "Video frame parameters changed: dimensions=" - << last_frame_info_.width << "x" << last_frame_info_.height - << ", rotation=" << last_frame_info_.rotation - << ", texture=" << last_frame_info_.is_texture; - } - - if (encoder_sink_ == NULL) { - // Frame input before send codecs are configured, dropping frame. - return; - } - - last_frame_timestamp_us_ = video_frame.timestamp_us(); - - // Forward frame to the encoder regardless if we are sending or not. This is - // to ensure that the encoder can be reconfigured with the correct frame size - // as quickly as possible. - encoder_sink_->OnFrame(video_frame); -} - bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend( bool enable, const VideoOptions* options, @@ -1658,7 +1620,6 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend( // Ignore |options| pointer if |enable| is false. bool options_present = enable && options; - bool source_changing = source_ != source; if (options_present) { VideoOptions old_options = parameters_.options; @@ -1668,29 +1629,6 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend( } } - if (source_changing) { - rtc::CritScope cs(&lock_); - if (source == nullptr && last_frame_info_.width > 0 && encoder_sink_) { - LOG(LS_VERBOSE) << "Disabling capturer, sending black frame."; - // Force this black frame not to be dropped due to timestamp order - // check. As IncomingCapturedFrame will drop the frame if this frame's - // timestamp is less than or equal to last frame's timestamp, it is - // necessary to give this black frame a larger timestamp than the - // previous one. - last_frame_timestamp_us_ += rtc::kNumMicrosecsPerMillisec; - rtc::scoped_refptr black_buffer( - webrtc::I420Buffer::Create(last_frame_info_.width, - last_frame_info_.height)); - webrtc::I420Buffer::SetBlack(black_buffer); - - encoder_sink_->OnFrame(webrtc::VideoFrame( - black_buffer, last_frame_info_.rotation, last_frame_timestamp_us_)); - } - } - - // TODO(perkj, nisse): Remove |source_| and directly call - // |stream_|->SetSource(source) once the video frame types have been - // merged. if (source_ && stream_) { stream_->SetSource( nullptr, webrtc::VideoSendStream::DegradationPreference::kBalanced); @@ -1700,6 +1638,8 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend( if (source && stream_) { // Do not adapt resolution for screen content as this will likely // result in blurry and unreadable text. + // |this| acts like a VideoSource to make sure SinkWants are handled on the + // correct thread. stream_->SetSource( this, enable_cpu_overuse_detection_ && !parameters_.options.is_screencast.value_or(false) @@ -1973,45 +1913,35 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSend(bool send) { } void WebRtcVideoChannel2::WebRtcVideoSendStream::RemoveSink( - VideoSinkInterface* sink) { + rtc::VideoSinkInterface* sink) { RTC_DCHECK_RUN_ON(&thread_checker_); - { - rtc::CritScope cs(&lock_); - RTC_DCHECK(encoder_sink_ == sink); - encoder_sink_ = nullptr; - } - source_->RemoveSink(this); + RTC_DCHECK(encoder_sink_ == sink); + encoder_sink_ = nullptr; + source_->RemoveSink(sink); } void WebRtcVideoChannel2::WebRtcVideoSendStream::AddOrUpdateSink( - VideoSinkInterface* sink, + rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) { if (worker_thread_ == rtc::Thread::Current()) { // AddOrUpdateSink is called on |worker_thread_| if this is the first // registration of |sink|. RTC_DCHECK_RUN_ON(&thread_checker_); - { - rtc::CritScope cs(&lock_); - encoder_sink_ = sink; - } - source_->AddOrUpdateSink(this, wants); + encoder_sink_ = sink; + source_->AddOrUpdateSink(encoder_sink_, wants); } else { // Subsequent calls to AddOrUpdateSink will happen on the encoder task // queue. - invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [this, wants] { - RTC_DCHECK_RUN_ON(&thread_checker_); - bool encoder_sink_valid = true; - { - rtc::CritScope cs(&lock_); - encoder_sink_valid = encoder_sink_ != nullptr; - } - // Since |source_| is still valid after a call to RemoveSink, check if - // |encoder_sink_| is still valid to check if this call should be - // cancelled. - if (source_ && encoder_sink_valid) { - source_->AddOrUpdateSink(this, wants); - } - }); + invoker_.AsyncInvoke( + RTC_FROM_HERE, worker_thread_, [this, sink, wants] { + RTC_DCHECK_RUN_ON(&thread_checker_); + // |sink| may be invalidated after this task was posted since + // RemoveSink is called on the worker thread. + bool encoder_sink_valid = (sink == encoder_sink_); + if (source_ && encoder_sink_valid) { + source_->AddOrUpdateSink(encoder_sink_, wants); + } + }); } } @@ -2135,12 +2065,10 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::RecreateWebRtcStream() { parameters_.encoder_config.encoder_specific_settings = NULL; if (source_) { - // TODO(perkj, nisse): Remove |source_| and directly call - // |stream_|->SetSource(source) once the video frame types have been - // merged and |stream_| internally reconfigure the encoder on frame - // resolution change. // Do not adapt resolution for screen content as this will likely result in // blurry and unreadable text. + // |this| acts like a VideoSource to make sure SinkWants are handled on the + // correct thread. stream_->SetSource( this, enable_cpu_overuse_detection_ && !parameters_.options.is_screencast.value_or(false) diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index 1fb794eece..b50c5a148e 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -231,11 +231,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { static std::string CodecSettingsVectorToString( const std::vector& codecs); - // Wrapper for the sender part, this is where the source is connected and - // frames are then converted from cricket frames to webrtc frames. + // Wrapper for the sender part. class WebRtcVideoSendStream - : public rtc::VideoSinkInterface, - public rtc::VideoSourceInterface { + : public rtc::VideoSourceInterface { public: WebRtcVideoSendStream( webrtc::Call* call, @@ -256,14 +254,12 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { // Implements rtc::VideoSourceInterface. // WebRtcVideoSendStream acts as a source to the webrtc::VideoSendStream - // in |stream_|. - // TODO(perkj, nisse): Refactor WebRtcVideoSendStream to directly connect - // the camera input |source_| - void AddOrUpdateSink(VideoSinkInterface* sink, + // in |stream_|. This is done to proxy VideoSinkWants from the encoder to + // the worker thread. + void AddOrUpdateSink(rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) override; - void RemoveSink(VideoSinkInterface* sink) override; + void RemoveSink(rtc::VideoSinkInterface* sink) override; - void OnFrame(const webrtc::VideoFrame& frame) override; bool SetVideoSend(bool mute, const VideoOptions* options, rtc::VideoSourceInterface* source); @@ -306,21 +302,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { bool external; }; - // TODO(perkj): VideoFrameInfo is currently used for sending a black frame - // when the video source is removed. Consider moving that logic to - // VieEncoder or remove it. - struct VideoFrameInfo { - VideoFrameInfo() - : width(0), - height(0), - rotation(webrtc::kVideoRotation_0), - is_texture(false) {} - int width; - int height; - webrtc::VideoRotation rotation; - bool is_texture; - }; - rtc::scoped_refptr ConfigureVideoEncoderSettings(const VideoCodec& codec); AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec); @@ -348,10 +329,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { WebRtcVideoEncoderFactory* const external_encoder_factory_ ACCESS_ON(&thread_checker_); - rtc::CriticalSection lock_; webrtc::VideoSendStream* stream_ ACCESS_ON(&thread_checker_); rtc::VideoSinkInterface* encoder_sink_ - GUARDED_BY(lock_); + ACCESS_ON(&thread_checker_); // Contains settings that are the same for all streams in the MediaChannel, // such as codecs, header extensions, and the global bitrate limit for the // entire channel. @@ -363,13 +343,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { // one stream per MediaChannel. webrtc::RtpParameters rtp_parameters_ ACCESS_ON(&thread_checker_); AllocatedEncoder allocated_encoder_ ACCESS_ON(&thread_checker_); - VideoFrameInfo last_frame_info_ GUARDED_BY(lock_); bool sending_ ACCESS_ON(&thread_checker_); - - // The timestamp of the last frame received - // Used to generate timestamp for the black frame when source is removed - int64_t last_frame_timestamp_us_ GUARDED_BY(lock_); }; // Wrapper for the receiver part, contains configs etc. that are needed to diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 8349469669..852eedd65c 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -1654,8 +1654,7 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) { << "Non-screenshare shouldn't use min-transmit bitrate."; EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr)); - // Removing a capturer triggers a black frame to be sent. - EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames()); + EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames()); VideoOptions screencast_options; screencast_options.is_screencast = rtc::Optional(true); EXPECT_TRUE( @@ -1663,7 +1662,7 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) { EXPECT_TRUE(capturer.CaptureFrame()); // Send stream not recreated after option change. ASSERT_EQ(send_stream, fake_call_->GetVideoSendStreams().front()); - EXPECT_EQ(3, send_stream->GetNumberOfSwappedFrames()); + EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames()); // Verify screencast settings. encoder_config = send_stream->GetEncoderConfig().Copy();