Negate flag name for prerender smoothing and update comments.

Further, strictly require VideoReceiveStream::Config::rendererer
to be non-null when the VideoReceiveStream is started. This is
already true by construction in the production code.

Bug: None
Change-Id: Ia0a41cfafa44215efc195a9eb6204194930c3dde
Reviewed-on: https://webrtc-review.googlesource.com/c/115040
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26384}

call/video_receive_stream.h

@@ -193,17 +193,16 @@ class VideoReceiveStream {
 
     MediaTransportInterface* media_transport = nullptr;
 
-    // Must not be 'nullptr' when the stream is started.
+    // Must always be set.
     rtc::VideoSinkInterface<VideoFrame>* renderer = nullptr;
 
     // Expected delay needed by the renderer, i.e. the frame will be delivered
     // this many milliseconds, if possible, earlier than the ideal render time.
-    // Only valid if 'renderer' is set.
     int render_delay_ms = 10;
 
-    // If set, pass frames on to the renderer as soon as they are
+    // If false, pass frames on to the renderer as soon as they are
     // available.
-    bool disable_prerenderer_smoothing = false;
+    bool enable_prerenderer_smoothing = true;
 
     // Identifier for an A/V synchronization group. Empty string to disable.
     // TODO(pbos): Synchronize streams in a sync group, not just video streams

media/base/media_config.h

@@ -33,21 +33,15 @@ struct MediaConfig {
     // to VideoSendStream::Config::suspend_below_min_bitrate.
     bool suspend_below_min_bitrate = false;
 
-    // Set to true if the renderer has an algorithm of frame selection.
-    // If the value is true, then WebRTC will hand over a frame as soon as
-    // possible without delay, and rendering smoothness is completely the duty
-    // of the renderer;
-    // If the value is false, then WebRTC is responsible to delay frame release
-    // in order to increase rendering smoothness.
-    //
-    // This flag comes from PeerConnection's RtcConfiguration, but is
-    // currently only set by the command line flag
-    // 'disable-rtc-smoothness-algorithm'.
-    // WebRtcVideoChannel::AddRecvStream copies it to the created
-    // WebRtcVideoReceiveStream, where it is returned by the
-    // SmoothsRenderedFrames method. This method is used by the
-    // VideoReceiveStream, where the value is passed on to the
-    // IncomingVideoStream constructor.
+    // Enable buffering and playout timing smoothing of decoded frames.
+    // If set to true, then WebRTC will buffer and potentially drop decoded
+    // frames in order to keep a smooth rendering.
+    // If set to false, then WebRTC will hand over the frame from the decoder
+    // to the renderer as soon as possible, meaning that the renderer is
+    // responsible for smooth rendering.
+    // Note that even if this flag is set to false, dropping of frames can
+    // still happen pre-decode, e.g., dropping of higher temporal layers.
+    // This flag comes from the PeerConnection RtcConfiguration.
     bool enable_prerenderer_smoothing = true;
 
     // Enables periodic bandwidth probing in application-limited region.

media/engine/webrtc_video_engine.cc

@@ -1184,9 +1184,8 @@ bool WebRtcVideoChannel::AddRecvStream(const StreamParams& sp,
   ConfigureReceiverRtp(&config, &flexfec_config, sp);
 
   config.crypto_options = crypto_options_;
-  // TODO(nisse): Rename config variable to avoid negation.
-  config.disable_prerenderer_smoothing =
-      !video_config_.enable_prerenderer_smoothing;
+  config.enable_prerenderer_smoothing =
+      video_config_.enable_prerenderer_smoothing;
   if (!sp.stream_ids().empty()) {
     config.sync_group = sp.stream_ids()[0];
   }

video/end_to_end_tests/retransmission_tests.cc

@@ -316,6 +316,7 @@ TEST_F(RetransmissionEndToEndTest, ReceivesPliAndRecoversWithNack) {
 TEST_F(RetransmissionEndToEndTest, ReceivesPliAndRecoversWithoutNack) {
   ReceivesPliAndRecovers(0);
 }
+
 // This test drops second RTP packet with a marker bit set, makes sure it's
 // retransmitted and renders. Retransmission SSRCs are also checked.
 void RetransmissionEndToEndTest::DecodesRetransmittedFrame(bool enable_rtx,
@@ -399,7 +400,8 @@ void RetransmissionEndToEndTest::DecodesRetransmittedFrame(bool enable_rtx,
       RTC_DCHECK(!orig_renderer_);
       orig_renderer_ = (*receive_configs)[0].renderer;
       RTC_DCHECK(orig_renderer_);
-      (*receive_configs)[0].disable_prerenderer_smoothing = true;
+      // To avoid post-decode frame dropping, disable the prerender buffer.
+      (*receive_configs)[0].enable_prerenderer_smoothing = false;
       (*receive_configs)[0].renderer = this;
 
       (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;

video/video_receive_stream.cc

@@ -198,6 +198,7 @@ VideoReceiveStream::VideoReceiveStream(
       rtp_stream_sync_(this) {
   RTC_LOG(LS_INFO) << "VideoReceiveStream: " << config_.ToString();
 
+  RTC_DCHECK(config_.renderer);
   RTC_DCHECK(process_thread_);
   RTC_DCHECK(call_stats_);
 
@@ -269,11 +270,13 @@ void VideoReceiveStream::SetSync(Syncable* audio_syncable) {
 
 void VideoReceiveStream::Start() {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_);
-  if (decode_thread_.IsRunning())
-    return;
 
-  bool protected_by_fec = config_.rtp.protected_by_flexfec ||
-                          rtp_video_stream_receiver_.IsUlpfecEnabled();
+  if (decode_thread_.IsRunning()) {
+    return;
+  }
+
+  const bool protected_by_fec = config_.rtp.protected_by_flexfec ||
+                                rtp_video_stream_receiver_.IsUlpfecEnabled();
 
   frame_buffer_->Start();
 
@@ -284,16 +287,13 @@ void VideoReceiveStream::Start() {
 
   transport_adapter_.Enable();
   rtc::VideoSinkInterface<VideoFrame>* renderer = nullptr;
-  if (config_.renderer) {
-    if (config_.disable_prerenderer_smoothing) {
-      renderer = this;
-    } else {
-      incoming_video_stream_.reset(
-          new IncomingVideoStream(config_.render_delay_ms, this));
-      renderer = incoming_video_stream_.get();
-    }
+  if (config_.enable_prerenderer_smoothing) {
+    incoming_video_stream_.reset(
+        new IncomingVideoStream(config_.render_delay_ms, this));
+    renderer = incoming_video_stream_.get();
+  } else {
+    renderer = this;
   }
-  RTC_DCHECK(renderer != nullptr);
 
   for (const Decoder& decoder : config_.decoders) {
     std::unique_ptr<VideoDecoder> video_decoder =
@@ -329,6 +329,7 @@ void VideoReceiveStream::Start() {
                              &codec, num_cpu_cores_, false));
   }
 
+  RTC_DCHECK(renderer != nullptr);
   video_stream_decoder_.reset(new VideoStreamDecoder(
       &video_receiver_, &rtp_video_stream_receiver_,
       &rtp_video_stream_receiver_,
@@ -408,7 +409,6 @@ void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) {
     // TODO(tommi): OnSyncOffsetUpdated grabs a lock.
     stats_proxy_.OnSyncOffsetUpdated(sync_offset_ms, estimated_freq_khz);
   }
-  // config_.renderer must never be null if we're getting this callback.
   config_.renderer->OnFrame(video_frame);
 
   // TODO(tommi): OnRenderFrame grabs a lock too.