Reland: Use webrtc::VideoFrame timestamp in RTCVideoEncoder

content/renderer/media/gpu/rtc_video_encoder.cc

Index: content/renderer/media/gpu/rtc_video_encoder.cc
diff --git a/content/renderer/media/gpu/rtc_video_encoder.cc b/content/renderer/media/gpu/rtc_video_encoder.cc
index 4c42938e2a4bef07048703ba1fb759c6d4448135..f55e271bc3301ef6a47bbd1883329c828b39631e 100644
--- a/content/renderer/media/gpu/rtc_video_encoder.cc
+++ b/content/renderer/media/gpu/rtc_video_encoder.cc
@@ -6,6 +6,8 @@
 
 #include <string.h>
 
+#include <deque>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -18,6 +20,7 @@
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/bitstream_buffer.h"
 #include "media/base/video_frame.h"
@@ -32,6 +35,17 @@ namespace content {
 
 namespace {
 
+struct RTCTimestamps {
+  RTCTimestamps(const base::TimeDelta& media_timestamp, int32_t rtp_timestamp)
+      : media_timestamp_in_microseconds(media_timestamp.InMicroseconds()),
+        rtp_timestamp(rtp_timestamp) {}
+  const int64_t media_timestamp_in_microseconds;
+  const int32_t rtp_timestamp;
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(RTCTimestamps);
+};
+
 // Translate from webrtc::VideoCodecType and webrtc::VideoCodec to
 // media::VideoCodecProfile.
 media::VideoCodecProfile WebRTCVideoCodecToVideoCodecProfile(
@@ -208,6 +222,10 @@ class RTCVideoEncoder::Impl
   // The underlying VEA to perform encoding on.
   std::unique_ptr<media::VideoEncodeAccelerator> video_encoder_;
 
+  // Used to match the encoded frame timestamp with WebRTC's given RTP
+  // timestamp.
+  std::deque<RTCTimestamps> pending_timestamps_;
+
   // Next input frame.  Since there is at most one next frame, a single-element
   // queue is sufficient.
   const webrtc::VideoFrame* input_next_frame_;
@@ -471,26 +489,41 @@ void RTCVideoEncoder::Impl::BitstreamBufferReady(int32_t bitstream_buffer_id,
 
   // Derive the capture time (in ms) and RTP timestamp (in 90KHz ticks).
   int64_t capture_time_us, capture_time_ms;
-  uint32_t rtp_timestamp;
+  base::Optional<uint32_t> rtp_timestamp;
 
   if (!timestamp.is_zero()) {
-    capture_time_us = timestamp.InMicroseconds();;
+    capture_time_us = timestamp.InMicroseconds();
     capture_time_ms = timestamp.InMilliseconds();
+    // Pop timestamps until we have a match.
+    while (!pending_timestamps_.empty()) {
+      const auto& front_timestamps = pending_timestamps_.front();
+      if (front_timestamps.media_timestamp_in_microseconds ==
+          timestamp.InMicroseconds()) {
+        rtp_timestamp = front_timestamps.rtp_timestamp;
+        pending_timestamps_.pop_front();
+        break;
+      }
+      pending_timestamps_.pop_front();
+    }

[pbos, 2017/02/10 17:24:43]

I'm worried we'll ever go from RTPTimestamps to non-RTP timestamps (some match
and some don't).

If some VEAs are broken aren't we breaking them completely now? Or is
rtp_timestamp approximately the generated one?

[emircan, 2017/02/10 22:48:37]

I am considering the cases where VEA is consistently giving the wrong
timestamps. Look at AndroidVEA for instance[0]. It returns the time when encode
is completed. We can't match that with anything. So, we should just empty the
queue and set rtp timestamp using l.515. This is same as what was happening
before.

Also, I added unittest to verify the match. 

[0]
https://cs.chromium.org/chromium/src/media/gpu/android_video_encode_accelerat...

[pbos, 2017/02/13 03:35:51]

I would like to never ever use pending_timestamps_ if they ever fail so that we
don't end up in a case where we flipflop back and forth adding jitter. What do
you think?

If so, can we add logging for how often that actually fails? E.g. something
HISTOGRAMS?

[emircan, 2017/02/13 20:23:19]

I see, you are considering the case where we can have one odd match using the
wrong value.I can add a boolean to track that. 

I actually looked closer to AndroidVEA. It looks like none of the clients make
use that given current time. We can just pass an empty timestamp there instead
and fall to the else case here. Then, I think I can add
DCHECK(rtp_timestamp.has_value()) as you first suggested.

I dont think histograms are necessary. There are VEA unittests to make sure that
it is contained in Mac/Win/CrOS. And Android always fails.

   } else {
     // Fallback to the current time if encoder does not provide timestamp.
     capture_time_us = rtc::TimeMicros();
     capture_time_ms = capture_time_us / base::Time::kMicrosecondsPerMillisecond;
+    pending_timestamps_.clear();
+  }
+
+  if (!rtp_timestamp.has_value()) {
+    // RTP timestamp can wrap around. Get the lower 32 bits.
+    rtp_timestamp = static_cast<uint32_t>(
+        capture_time_us * 90 / base::Time::kMicrosecondsPerMillisecond);

[nisse-chromium (ooo August 14), 2017/02/13 07:40:32]

Note that webrtc sets the rtp time differently, based on ntp time. See 

https://cs.chromium.org/chromium/src/third_party/webrtc/video/vie_encoder.cc?...

Not sure if that's a problem (not changed in this cl).

[emircan, 2017/02/13 20:23:19]

This is the case where HW encoder actually drops timestamp information and we do
not have any indication, i.e. Android. We do not have ntp_time either. We just
make up an rtp_timestamp and capture_time_ms from current time. I don't know how
else we can do it.

   }
-  // RTP timestamp can wrap around. Get the lower 32 bits.
-  rtp_timestamp = static_cast<uint32_t>(
-      capture_time_us * 90 / base::Time::kMicrosecondsPerMillisecond);
 
   webrtc::EncodedImage image(
       reinterpret_cast<uint8_t*>(output_buffer->memory()), payload_size,
       output_buffer->mapped_size());
   image._encodedWidth = input_visible_size_.width();
   image._encodedHeight = input_visible_size_.height();
-  image._timeStamp = rtp_timestamp;
+  image._timeStamp = rtp_timestamp.value();
   image.capture_time_ms_ = capture_time_ms;

[nisse-chromium (ooo August 14), 2017/02/14 07:45:04]

I think we must use a time consistent with rtc::TimeMicros(), since, iiuc, this
time is going to be compared to clock_->TimeInMicroseconds() down in the
rtp/rtcp code. Raw camera time (+ offset) won't work.

So either always use current time, or if you can get render_time_ms() (aka
timestamp_us() / 1000) from the corresponding uncompressed webrtc::VideoFrame.
Or -1, and then the rtp code should substitute current time later.

[emircan, 2017/02/14 20:07:33]

We can't guarantee returning render_time_ms() as some HW encoders actually don't
preserve timestamp info, i.e. Android.

I will make that change in a separate CL, please review 
https://codereview.chromium.org/2692853009/. This CL is addressing rtp_timestamp
which is related to stats, see
https://bugs.chromium.org/p/chromium/issues/detail?id=597087. We have concerns
that it might trigger some unwanted side effects as you can see in the bug, and
may be reverted like the attempts before.

   image._frameType =
       (key_frame ? webrtc::kVideoFrameKey : webrtc::kVideoFrameDelta);
@@ -610,6 +643,13 @@ void RTCVideoEncoder::Impl::EncodeOneFrame() {
   }
   frame->AddDestructionObserver(media::BindToCurrentLoop(
       base::Bind(&RTCVideoEncoder::Impl::EncodeFrameFinished, this, index)));
+  DCHECK(std::find_if(pending_timestamps_.begin(), pending_timestamps_.end(),
+                      [&frame](const RTCTimestamps& entry) {
+           return entry.media_timestamp_in_microseconds ==
+                  frame->timestamp().InMicroseconds();
+         }) == pending_timestamps_.end());
+  pending_timestamps_.emplace_back(frame->timestamp(), next_frame->timestamp());
+
   video_encoder_->Encode(frame, next_frame_keyframe);
   input_buffers_free_.pop_back();
   SignalAsyncWaiter(WEBRTC_VIDEO_CODEC_OK);