Split IncomingVideoStream into two implementations, with smoothing and without.

webrtc/video/video_receive_stream.cc

Index: webrtc/video/video_receive_stream.cc
diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
index 3ad97f5ccd93041cc16b8e2fa0e4ee63c8d31fdb..a481cb52cb844bac5cf856320318a7de4315fd1d 100644
--- a/webrtc/video/video_receive_stream.cc
+++ b/webrtc/video/video_receive_stream.cc
@@ -144,6 +144,32 @@ VideoCodec CreateDecoderVideoCodec(const VideoReceiveStream::Decoder& decoder) {
 }  // namespace
 
 namespace internal {
+
+// Since an IncomingVideoStream instance will create a thread/queue, we don't
+// instantiate one unless we know we're going to be delivering the frames
+// to a renderer.
+//
+// TODO(tommi): Consider making the functionality provided by the
+// IncomingVideoStream classes, tied to the Config class instead or even higher
+// level.  That will make it an optional feature that will be up to the
+// application to use or not use.  Right now, we have two classes available,
+// they both have threads involved which uses WebRTC's threading classes and
+// that might not suit what the application wants to do.  If one of them or
+// neither, suits, the app can get some code size back as well as more control.
+std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>
+MaybeCreateIncomingVideoStream(const VideoReceiveStream::Config& config,
+                               rtc::VideoSinkInterface<VideoFrame>* callback) {
+  std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> ret;
+  if (config.renderer) {
+    if (config.disable_prerenderer_smoothing) {
+      ret.reset(new IncomingVideoStreamNoSmoothing(callback));
+    } else {
+      ret.reset(new IncomingVideoStream(config.render_delay_ms, callback));
+    }
+  }
+  return ret;
+}
+
 VideoReceiveStream::VideoReceiveStream(
     int num_cpu_cores,
     CongestionController* congestion_controller,
@@ -161,7 +187,6 @@ VideoReceiveStream::VideoReceiveStream(
       congestion_controller_(congestion_controller),
       call_stats_(call_stats),
       video_receiver_(clock_, nullptr, this, this, this),
-      incoming_video_stream_(config_.disable_prerenderer_smoothing),
       stats_proxy_(&config_, clock_),
       rtp_stream_receiver_(&video_receiver_,
                            congestion_controller_->GetRemoteBitrateEstimator(
@@ -174,14 +199,6 @@ VideoReceiveStream::VideoReceiveStream(
                            &config_,
                            &stats_proxy_,
                            process_thread_),
-      video_stream_decoder_(&video_receiver_,
-                            &rtp_stream_receiver_,
-                            &rtp_stream_receiver_,
-                            rtp_stream_receiver_.IsRetransmissionsEnabled(),
-                            rtp_stream_receiver_.IsFecEnabled(),
-                            &stats_proxy_,
-                            &incoming_video_stream_,
-                            config.pre_render_callback),
       vie_sync_(&video_receiver_) {
   LOG(LS_INFO) << "VideoReceiveStream: " << config_.ToString();
 
@@ -189,9 +206,6 @@ VideoReceiveStream::VideoReceiveStream(
   RTC_DCHECK(congestion_controller_);
   RTC_DCHECK(call_stats_);
 
-  // Register the channel to receive stats updates.
-  call_stats_->RegisterStatsObserver(&video_stream_decoder_);
-
   RTC_DCHECK(!config_.decoders.empty());
   std::set<int> decoder_payload_types;
   for (const Decoder& decoder : config_.decoders) {
@@ -211,8 +225,6 @@ VideoReceiveStream::VideoReceiveStream(
   }
 
   video_receiver_.SetRenderDelay(config.render_delay_ms);
-  incoming_video_stream_.SetExpectedRenderDelay(config.render_delay_ms);
-  incoming_video_stream_.SetExternalCallback(this);
 
   process_thread_->RegisterModule(&video_receiver_);
   process_thread_->RegisterModule(&vie_sync_);
@@ -232,8 +244,6 @@ VideoReceiveStream::~VideoReceiveStream() {
   for (const Decoder& decoder : config_.decoders)
     video_receiver_.RegisterExternalDecoder(nullptr, decoder.payload_type);
 
-  call_stats_->DeregisterStatsObserver(&video_stream_decoder_);
-
   congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config_))
       ->RemoveStream(rtp_stream_receiver_.GetRemoteSsrc());
 }
@@ -257,7 +267,14 @@ void VideoReceiveStream::Start() {
   if (decode_thread_.IsRunning())
     return;
   transport_adapter_.Enable();
-  incoming_video_stream_.Start();
+  incoming_video_stream_ = MaybeCreateIncomingVideoStream(config_, this);
+  video_stream_decoder_.reset(new VideoStreamDecoder(
+      &video_receiver_, &rtp_stream_receiver_, &rtp_stream_receiver_,
+      rtp_stream_receiver_.IsRetransmissionsEnabled(),
+      rtp_stream_receiver_.IsFecEnabled(), &stats_proxy_,
+      incoming_video_stream_.get(), config_.pre_render_callback));
+  // Register the channel to receive stats updates.
+  call_stats_->RegisterStatsObserver(video_stream_decoder_.get());
   // Start the decode thread
   decode_thread_.Start();
   decode_thread_.SetPriority(rtc::kHighestPriority);
@@ -265,10 +282,12 @@ void VideoReceiveStream::Start() {
 }
 
 void VideoReceiveStream::Stop() {
-  incoming_video_stream_.Stop();
   rtp_stream_receiver_.StopReceive();
   video_receiver_.TriggerDecoderShutdown();
   decode_thread_.Stop();
+  call_stats_->DeregisterStatsObserver(video_stream_decoder_.get());
+  video_stream_decoder_.reset();
+  incoming_video_stream_.reset();
   transport_adapter_.Disable();
 }
 
@@ -290,16 +309,26 @@ VideoReceiveStream::Stats VideoReceiveStream::GetStats() const {
   return stats_proxy_.GetStats();
 }
 
+// TODO(tommi): This method grabs a lock 6 times.
 void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) {
+  // TODO(tommi): OnDecodedFrame grabs a lock, incidentally the same lock
+  // that OnSyncOffsetUpdated() and OnRenderedFrame() below grab.
   stats_proxy_.OnDecodedFrame();

[mflodman, 2016/06/13 18:39:24]

This is not the right place to log this stat, since we might want to log decode
events even without a renderer. I'll take a note about this and follow up.

[tommi, 2016/06/14 08:11:28]

OK.  Move into the decoder somehow?

[mflodman, 2016/06/14 08:59:26]

Yes, to VideoStremDecoder ot VideoReciecveStream. There's no obvious place
currently, but I'll figure something out in another CL.

 
   int64_t sync_offset_ms;
-  if (vie_sync_.GetStreamSyncOffsetInMs(video_frame, &sync_offset_ms))
+  // TODO(tommi): GetStreamSyncOffsetInMs grabs three locks.  One inside the
+  // function itself, another in GetChannel() and a third in
+  // GetPlayoutTimestamp.  Seems excessive.  Anyhow, I'm assuming the function
+  // succeeds most of the time, which leads to grabbing a fourth lock.
+  if (vie_sync_.GetStreamSyncOffsetInMs(video_frame, &sync_offset_ms)) {
+    // TODO(tommi): OnSyncOffsetUpdated grabs a lock.
     stats_proxy_.OnSyncOffsetUpdated(sync_offset_ms);
+  }
 
-  if (config_.renderer)
-    config_.renderer->OnFrame(video_frame);
+  // config_.renderer must never be null if we're getting this callback.
+  config_.renderer->OnFrame(video_frame);
 
+  // TODO(tommi): OnRenderFrame grabs a lock too.

[mflodman, 2016/06/13 18:39:24]

Any thoughts around how to get rid of that lock, and other, in the stats proxy?

[tommi, 2016/06/14 08:11:28]

I'd start by seeing if we can update the stats in one call.

A clock is being used by some of these methods to calculate an offset from 'now'
vs something else.  That could be done once at the top (and possibly once more
at the end of this function if necessary), and the calculations done on this
thread. Once all the stats have been calculated, they can be pushed to the stats
proxy and we'd grab a lock once and check the clock fewer times.

   stats_proxy_.OnRenderedFrame(video_frame.width(), video_frame.height());
 }