VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc clock.

webrtc/video/video_capture_input.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
 #include "webrtc/video/video_capture_input.h"
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/trace_event.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/video_capture/video_capture_factory.h"
 #include "webrtc/modules/video_processing/include/video_processing.h"
 #include "webrtc/modules/video_render/video_render_defines.h"
-#include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/tick_util.h"
 #include "webrtc/video/overuse_frame_detector.h"
 #include "webrtc/video/send_statistics_proxy.h"
 #include "webrtc/video/vie_encoder.h"
 
 namespace webrtc {
 
 namespace internal {
 VideoCaptureInput::VideoCaptureInput(VideoCaptureCallback* frame_callback,
                                      VideoRenderer* local_renderer,
                                      SendStatisticsProxy* stats_proxy,
                                      OveruseFrameDetector* overuse_detector)
     : frame_callback_(frame_callback),
       local_renderer_(local_renderer),
       stats_proxy_(stats_proxy),
       encoder_thread_(EncoderThreadFunction, this, "EncoderThread"),
       capture_event_(false, false),
       stop_(0),
+      // TODO(danilchap): pass clock from outside to ensure it is same clock
+      // used for audio frame timestamping.

[stefan-webrtc, 2016/02/11 16:08:47]

Audio frames aren't timestamped using the clock, but are instead counting
samples, so it's always based on the sound card rate.

[danilchap, 2016/02/11 17:18:54]

I skipped several steps in reasoning why this clock should be
Clock::GetRealTimeClock(), comment corrected.

+      clock_(Clock::GetRealTimeClock()),
       last_captured_timestamp_(0),
-      delta_ntp_internal_ms_(
-          Clock::GetRealTimeClock()->CurrentNtpInMilliseconds() -
-          TickTime::MillisecondTimestamp()),
+      delta_ntp_internal_ms_(clock_->CurrentNtpInMilliseconds() -
+                             TickTime::MillisecondTimestamp()),
       overuse_detector_(overuse_detector) {
   encoder_thread_.Start();
   encoder_thread_.SetPriority(rtc::kHighPriority);
 }
 
 VideoCaptureInput::~VideoCaptureInput() {
   // Stop the thread.
   rtc::AtomicOps::ReleaseStore(&stop_, 1);
   capture_event_.Set();
   encoder_thread_.Stop();
 }
 
 void VideoCaptureInput::IncomingCapturedFrame(const VideoFrame& video_frame) {
   // TODO(pbos): Remove local rendering, it should be handled by the client code
   // if required.
   if (local_renderer_)
     local_renderer_->RenderFrame(video_frame, 0);
 
   stats_proxy_->OnIncomingFrame(video_frame.width(), video_frame.height());
 
   VideoFrame incoming_frame = video_frame;
 
-  if (incoming_frame.ntp_time_ms() != 0) {
-    // If a NTP time stamp is set, this is the time stamp we will use.
-    incoming_frame.set_render_time_ms(incoming_frame.ntp_time_ms() -
-                                      delta_ntp_internal_ms_);
-  } else {  // NTP time stamp not set.
-    int64_t render_time = incoming_frame.render_time_ms() != 0
-                              ? incoming_frame.render_time_ms()
-                              : TickTime::MillisecondTimestamp();
+  // Local time in webrtc time base.

[stefan-webrtc, 2016/02/11 16:08:46]

It's not clear to me if anything below actually has changed. Is this mostly
refactoring? Or maybe it's just too late for me to review this. 

[danilchap, 2016/02/11 17:18:54]

render_time is always taken from clock_ instead of incoming video_frame.

rest is refactoring to clarify how ntp_time is calculated. calculation itself is
unchanged for ntp_time

+  int64_t current_time = clock_->TimeInMilliseconds();
+  incoming_frame.set_render_time_ms(current_time);

[stefan-webrtc, 2016/02/11 16:08:46]

Have you figured out what the render time is used for in the sender? Is it only
used for rendering locally?

[danilchap, 2016/02/11 17:18:54]

render_time (later named capture_time) is used by rtcp SenderReport to calculate
how much time had pass since last frame was captured. Together with rtp
timestamp of last frame SenderReport estimate what rtp timestamp would be when
it is time to send SenderReport.
SenderReports are used for stream synchronization.

 
-    incoming_frame.set_render_time_ms(render_time);
-    incoming_frame.set_ntp_time_ms(render_time + delta_ntp_internal_ms_);
+  // Capture time may come from clock with an offset and drift from clock_.
+  int64_t capture_ntp_time_ms;
+  if (video_frame.ntp_time_ms() != 0) {
+    capture_ntp_time_ms = video_frame.ntp_time_ms();
+  } else if (video_frame.render_time_ms() != 0) {
+    capture_ntp_time_ms = video_frame.render_time_ms() + delta_ntp_internal_ms_;
+  } else {
+    capture_ntp_time_ms = current_time + delta_ntp_internal_ms_;
   }
+  incoming_frame.set_ntp_time_ms(capture_ntp_time_ms);

[stefan-webrtc, 2016/02/11 16:08:46]

We should probably comment here that the ntp time is later converted to the rtp
timestamp.

[danilchap, 2016/02/11 17:18:54]

there is such comment 2 lines below.

 
   // Convert NTP time, in ms, to RTP timestamp.
   const int kMsToRtpTimestamp = 90;
   incoming_frame.set_timestamp(
       kMsToRtpTimestamp * static_cast<uint32_t>(incoming_frame.ntp_time_ms()));
 
   rtc::CritScope lock(&crit_);
   if (incoming_frame.ntp_time_ms() <= last_captured_timestamp_) {
     // We don't allow the same capture time for two frames, drop this one.
     LOG(LS_WARNING) << "Same/old NTP timestamp ("
@@ skipping 34 matching lines @@
     }
     if (!deliver_frame.IsZeroSize()) {
       frame_callback_->DeliverFrame(deliver_frame);
     }
   }
   return true;
 }
 
 }  // namespace internal
 }  // namespace webrtc