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

webrtc/video/video_capture_input.cc

Index: webrtc/video/video_capture_input.cc
diff --git a/webrtc/video/video_capture_input.cc b/webrtc/video/video_capture_input.cc
index dfdf5ae4825501525d655733504ed1e7f5fa044f..1eba2c805b9190a11a0dddcc16a797a951a07bab 100644
--- a/webrtc/video/video_capture_input.cc
+++ b/webrtc/video/video_capture_input.cc
@@ -17,7 +17,6 @@
 #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"
@@ -36,10 +35,12 @@ VideoCaptureInput::VideoCaptureInput(VideoCaptureCallback* frame_callback,
       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);
@@ -62,18 +63,20 @@ void VideoCaptureInput::IncomingCapturedFrame(const VideoFrame& video_frame) {
 
   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();
-
-    incoming_frame.set_render_time_ms(render_time);
-    incoming_frame.set_ntp_time_ms(render_time + delta_ntp_internal_ms_);
+  // 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.

+
+  // 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;