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

webrtc/media/engine/webrtcvideoengine2.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1546 matching lines @@
       ssrc_groups_(sp.ssrc_groups),
       call_(call),
       enable_cpu_overuse_detection_(enable_cpu_overuse_detection),
       source_(nullptr),
       external_encoder_factory_(external_encoder_factory),
       stream_(nullptr),
       encoder_sink_(nullptr),
       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;
 
   sp.GetPrimarySsrcs(&parameters_.config.rtp.ssrcs);
   sp.GetFidSsrcs(parameters_.config.rtp.ssrcs,
                  &parameters_.config.rtp.rtx.ssrcs);
   parameters_.config.rtp.c_name = sp.cname;
   if (rtp_extensions) {
     parameters_.config.rtp.extensions = *rtp_extensions;
   }
   parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size
                                          ? webrtc::RtcpMode::kReducedSize
                                          : webrtc::RtcpMode::kCompound;
   if (codec_settings) {
     SetCodec(*codec_settings);
   }
 }
 
 WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() {
   if (stream_ != NULL) {
     call_->DestroyVideoSendStream(stream_);
   }
   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,
     rtc::VideoSourceInterface<cricket::VideoFrame>* source) {
   TRACE_EVENT0("webrtc", "WebRtcVideoSendStream::SetVideoSend");
   RTC_DCHECK_RUN_ON(&thread_checker_);
 
   // 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;
     parameters_.options.SetAll(*options);
     if (parameters_.options != old_options) {
       ReconfigureEncoder();
     }
   }
 
-  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);
   }
   // Switch to the new source.
   source_ = source;
   if (source && stream_) {
     // 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;
 }
 
 const std::vector<uint32_t>&
 WebRtcVideoChannel2::WebRtcVideoSendStream::GetSsrcs() const {
   return ssrcs_;
 }
 
 WebRtcVideoChannel2::WebRtcVideoSendStream::AllocatedEncoder
@@ skipping 240 matching lines @@
   parameters_.encoder_config = std::move(encoder_config);
 }
 
 void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSend(bool send) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   sending_ = send;
   UpdateSendState();
 }
 
 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);
+          }
+        });
   }
 }
 
 VideoSenderInfo WebRtcVideoChannel2::WebRtcVideoSendStream::GetVideoSenderInfo(
     bool log_stats) {
   VideoSenderInfo info;
   RTC_DCHECK_RUN_ON(&thread_checker_);
   for (uint32_t ssrc : parameters_.config.rtp.ssrcs)
     info.add_ssrc(ssrc);
 
@@ skipping 100 matching lines @@
     LOG(LS_WARNING) << "RTX SSRCs configured but there's no configured RTX "
                        "payload type the set codec. Ignoring RTX.";
     config.rtp.rtx.ssrcs.clear();
   }
   stream_ = call_->CreateVideoSendStream(std::move(config),
                                          parameters_.encoder_config.Copy());
 
   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.
   UpdateSendState();
 }
 
 WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream(
     webrtc::Call* call,
     const StreamParams& sp,
     webrtc::VideoReceiveStream::Config config,
@@ skipping 434 matching lines @@
         rtx_mapping[video_codecs[i].codec.id] !=
             ulpfec_config.red_payload_type) {
       video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id];
     }
   }
 
   return video_codecs;
 }
 
 }  // namespace cricket