Implement timing frames.

webrtc/video/vie_encoder.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.
  */
 
@@ skipping 546 matching lines @@
 
   stats_proxy_->SetAdaptationStats(GetActiveCounts(kCpu),
                                    GetActiveCounts(kQuality));
 }
 
 void ViEEncoder::OnFrame(const VideoFrame& video_frame) {
   RTC_DCHECK_RUNS_SERIALIZED(&incoming_frame_race_checker_);
   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;
   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_;

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

BTW, this delta_ntp_internal_ms may be the caching Erik mentioned in another
comment.

I don't quite like that either. On linux, the offset between rtc::TimeMicros and
utc time is constant over normal frequency adjustments by the ntp daemon (but
will obviously change if the system time is explicitly reset). But in general,
rtc::TimeMicros might have a frequency drift compared to the system wall clock.

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

Acknowledged.

   } else {
     capture_ntp_time_ms = current_time_ms + delta_ntp_internal_ms_;
   }
   incoming_frame.set_ntp_time_ms(capture_ntp_time_ms);
 
   // 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()));
 
@@ skipping 447 matching lines @@
 std::string ViEEncoder::AdaptCounter::ToString(
     const std::vector<int>& counters) const {
   std::stringstream ss;
   for (size_t reason = 0; reason < kScaleReasonSize; ++reason) {
     ss << (reason ? " cpu" : "quality") << ":" << counters[reason];
   }
   return ss.str();
 }
 
 }  // namespace webrtc