Implement timing frames.

webrtc/video/vie_encoder.cc

Index: webrtc/video/vie_encoder.cc
diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc
index b56bbf5153852af7c67e70fff1395ef52677639d..9fd977941171c2cc02b3d287d9c8c837d9653c7b 100644
--- a/webrtc/video/vie_encoder.cc
+++ b/webrtc/video/vie_encoder.cc
@@ -564,7 +564,13 @@ void ViEEncoder::OnFrame(const VideoFrame& video_frame) {
   VideoFrame incoming_frame = video_frame;
 
   // Local time in webrtc time base.
-  int64_t current_time_ms = clock_->TimeInMilliseconds();
+  int64_t current_time_us = clock_->TimeInMicroseconds();
+  int64_t current_time_ms = current_time_us / rtc::kNumMicrosecsPerMillisec;
+  // If the frame is not from the capturer but from decoder, render time may
+  // be set to the future. To avoid irregularities in statistics, where frame
+  // is captured after being encoded, we should reset capture time here.
+  if (incoming_frame.timestamp_us() > current_time_us)

[nisse-webrtc, 2017/06/02 12:04:55]

I don't quite understand the problem here, and I'd really prefer to not
reintroduce overwriting of the timestamps in this function. 

If frames are forwarded, i.e., generated by the decoder, which selects some
future "render time", and then reencoded, can't the responsibility for clipping
the timestamps should be moved somewhere before this function? Maybe you can use
the TimestampAligner class (same logic which is used to process camera
timestamps).

[ilnik, 2017/06/02 13:02:21]

Problem is that timestamps are sent over network as deltas from capture_time
(because we can't and don't want to send 40 bytes of timestamps). I am using
uint16_t for that (which gives us ability to send correctly timestamps upto 65
seconds in the future). So capture time after encode start time produces
negative delta, which we can't even send.

Removal of rewriting timestamps simply breaks everything.

Also, introducing additional layer between producer and consumer of frames just
to rewrite timestamps seems too much (especially because it will have to be done
in client code). Also, it will produce very hard to investigate errors if
someone forgets about that additional layer.

I agree, it may make sense to move all the timestamps processing code here
(L566-606) somewhere else, but it's completely separate refactoring to that I am
trying to do.

[nisse-webrtc, 2017/06/08 06:58:36]

I think I'm missing some context here, can you help me? This conditional checks
if the frame timestamp is in the future. When will that happen? Which code
produces webrtc::VideoFrame objects with future timestamps?

I'm mainly familiar with the capture pipeline, and there the TimestampAligner is
used which, among other things, ensures timestamps aren't in the future. And I
agree that's a reasonable requirement.

[ilnik, 2017/06/08 07:26:00]

It happens in PeerConnectionIntegrationTest.CanSendRemoteVideoTrack test. I
don't quite understand all the details, but output of one decoder is feed into
another encoder inside VideoBroadcaster. In exactly that situation timestamp in
the frame (interpreted as capture time in encoders and render time in decoders)
will be set by decoder to projected render time in the future.

[nisse-webrtc, 2017/06/08 07:38:36]

Ok, I see. Could you clarify the comment a bit? I think the reference to
"statistics" is confusing, if we have a general requirement downstream on the
encode and rtp send path that the timestamp must not be in the future.

I think the proper way of doing this would be to have the decode pipeline attach
its best guesstimate for original capture time to the frame (which ought to
always be in the past), and apply an appropriate playout delay closer to
rendering. Do you think that's doable? I understand it's going to be a much
larger refactoring.

[ilnik, 2017/06/08 08:01:43]

I clarified the comment. 

About the broad refactoring, I don't see much benefits here. The only
problematic case is when encoder is fed on decoder output. But amount of
refactoring is quite huge. Also, it's still possible in that case to wire things
mistakenly and still fed frames with incorrect timestamps to the decoder. So,
safety check here is still required.

+    incoming_frame.set_timestamp_us(current_time_us);
 
   // Capture time may come from clock with an offset and drift from clock_.
   int64_t capture_ntp_time_ms;