Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame on source remova…

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index e37bf94a651cc517676c484861ee063ebc1b1aca..f5e0420ce572246ba02a6951d249b3008bfc78f7 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -1564,8 +1564,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
       parameters_(std::move(config), options, max_bitrate_bps, codec_settings),
       rtp_parameters_(CreateRtpParametersWithOneEncoding()),
       allocated_encoder_(nullptr, webrtc::kVideoCodecUnknown, false),
-      sending_(false),
-      last_frame_timestamp_us_(0) {
+      sending_(false) {
   parameters_.config.rtp.max_packet_size = kVideoMtu;
   parameters_.conference_mode = send_params.conference_mode;
 
@@ -1591,43 +1590,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() {
   DestroyVideoEncoder(&allocated_encoder_);
 }
 
-void WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame(
-    const 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,
@@ -1637,7 +1599,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;
@@ -1647,29 +1608,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<webrtc::I420Buffer> black_buffer(
-          webrtc::I420Buffer::Create(last_frame_info_.width,
-                                     last_frame_info_.height));
-      black_buffer->SetToBlack();
-
-      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);
@@ -1680,11 +1618,11 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(
     // Do not adapt resolution for screen content as this will likely
     // result in blurry and unreadable text.
     stream_->SetSource(
-        this, enable_cpu_overuse_detection_ &&
-                      !parameters_.options.is_screencast.value_or(false)
-                  ? webrtc::VideoSendStream::DegradationPreference::kBalanced
-                  : webrtc::VideoSendStream::DegradationPreference::
-                        kMaintainResolution);
+        source, enable_cpu_overuse_detection_ &&

[perkj_webrtc, 2016/11/16 21:27:24]

ops, this should still be |this|

+                        !parameters_.options.is_screencast.value_or(false)
+                    ? webrtc::VideoSendStream::DegradationPreference::kBalanced
+                    : webrtc::VideoSendStream::DegradationPreference::
+                          kMaintainResolution);
   }
   return true;
 }
@@ -1945,45 +1883,35 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSend(bool send) {
 }
 
 void WebRtcVideoChannel2::WebRtcVideoSendStream::RemoveSink(
-    VideoSinkInterface<webrtc::VideoFrame>* sink) {
+    rtc::VideoSinkInterface<webrtc::VideoFrame>* 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<webrtc::VideoFrame>* sink,
+    rtc::VideoSinkInterface<webrtc::VideoFrame>* 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<void>(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<void>(
+        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_;

[nisse-webrtc, 2016/11/07 09:52:46]

Hmm, this also appears to be the only use of the encoder_sink_ member. What
would it take to delete it? If I understand this correctly, it is intended to
handle a
race between application calling RemoveSink, and encoder calling AddOrUpdateSink
(from cpu adaptation logic). Right?

But how will the encoder ever call AddOrUpdateSink on this object? It looks like
you are hooking it up directly to the source, replacing SetSource(this, ...) by
SetSource(source_)? Then this race must be handled elsewhere. If there's no
better way, maybe we have to introduce a VideoSourceInterface::UpdateSink
method, which behaves like AddOrUpdateSink except that it's a nop if the sink is
not registered. 

[nisse-webrtc, 2016/11/07 09:52:46]

Style: I'd prefer a pair of () around the comparison.

[perkj_webrtc, 2016/11/16 21:27:24]

Since we can not use rtc::Threads on lower levels I see no other way than to do
it here or as you way in all source implementations. But then I prefer to do it
here.

[nisse-webrtc, 2016/11/17 08:07:13]

Not sure what other use cases there are with AddOrUpdateSink called from
multiple threads. In general, that's a sign of missing coordination (threads
will overwrite eachother's settings). But in this case, if I understand it
correctly, there's a call from the worker thread when the sink is wired up
(essentially AddSink), and calls from the encoder thread to tweak resolution
(essentially UpdateSink). If separating those methods can be done safely and
that helps us to connect camera and encoder directly, I think we should do it.
But that's for a separate cl.

+          if (source_ && encoder_sink_valid) {
+            source_->AddOrUpdateSink(encoder_sink_, wants);
+          }
+        });
   }
 }
 
@@ -2104,18 +2032,14 @@ 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.
     stream_->SetSource(
-        this, enable_cpu_overuse_detection_ &&
-                      !parameters_.options.is_screencast.value_or(false)
-                  ? webrtc::VideoSendStream::DegradationPreference::kBalanced
-                  : webrtc::VideoSendStream::DegradationPreference::
-                        kMaintainResolution);
+        source_, enable_cpu_overuse_detection_ &&
+                         !parameters_.options.is_screencast.value_or(false)
+                     ? webrtc::VideoSendStream::DegradationPreference::kBalanced
+                     : webrtc::VideoSendStream::DegradationPreference::
+                           kMaintainResolution);
   }
 
   // Call stream_->Start() if necessary conditions are met.