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..1cf5a2e3c22d3e839bdc12b38c67ce08a9925ff0 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(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,19 +489,34 @@ 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;
+  uint32_t rtp_timestamp = 0;

[pbos, 2017/02/10 00:05:08]

Can you make this optional?

[emircan, 2017/02/10 17:14:55]

Done.

 
   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()) {

[pbos, 2017/02/10 00:05:08]

This should never be empty, right? If so there's a bug in the VEA?

[emircan, 2017/02/10 17:14:55]

Yes. However I want to loop until all the dropped frames' timestamps are cleared
and I just use this as the condition to parse until everything is finished.
Suppose HW VEA is somewhat broken and returns a different timestamp then given,
then we would empty all timestamps and not let this queue grow.

+      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 00:05:07]

Can you DCHECK after this brace that rtp_timestamp actually got set?

[emircan, 2017/02/10 17:14:55]

Again, suppose HW VEA is somewhat broken and returns a different timestamp then
given,
then we would empty all timestamps and not let this queue grow. rtp_timestamp()
wouldn't be set then and we would fall to l.515.

   } 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 == 0) {

[pbos, 2017/02/10 00:05:08]

0 is a valid RTP timestamp. Can you gate this on whether rtp_timestamp is set or
not instead?

[emircan, 2017/02/10 17:14:55]

Done.

+    // RTP timestamp can wrap around. Get the lower 32 bits.
+    rtp_timestamp = static_cast<uint32_t>(
+        capture_time_us * 90 / base::Time::kMicrosecondsPerMillisecond);
   }
-  // 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,
@@ -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);