Reland: Use webrtc::VideoFrame timestamp in RTCVideoEncoder

content/renderer/media/gpu/rtc_video_encoder.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/gpu/rtc_video_encoder.h"
 
 #include <string.h>
 
+#include <deque>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/scoped_vector.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #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"
 #include "media/base/video_util.h"
 #include "media/filters/h264_parser.h"
 #include "media/renderers/gpu_video_accelerator_factories.h"
 #include "media/video/video_encode_accelerator.h"
 #include "third_party/libyuv/include/libyuv.h"
 #include "third_party/webrtc/base/timeutils.h"
 
 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(
     webrtc::VideoCodecType type,
     const webrtc::VideoCodec* codec_settings) {
   DCHECK_EQ(type, codec_settings->codecType);
   switch (type) {
     case webrtc::kVideoCodecVP8:
       return media::VP8PROFILE_ANY;
     case webrtc::kVideoCodecH264:
@@ skipping 156 matching lines @@
   // webrtc::VideoEncoder expects InitEncode() and Encode() to be synchronous.
   // Do this by waiting on the |async_waiter_| and returning the return value in
   // |async_retval_| when initialization completes, encoding completes, or
   // an error occurs.
   base::WaitableEvent* async_waiter_;
   int32_t* async_retval_;
 
   // 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_;
 
   // Whether to encode a keyframe next.
   bool input_next_frame_keyframe_;
 
   // Frame sizes.
   gfx::Size input_frame_coded_size_;
   gfx::Size input_visible_size_;
@@ skipping 243 matching lines @@
   base::SharedMemory* output_buffer = output_buffers_[bitstream_buffer_id];
   if (payload_size > output_buffer->mapped_size()) {
     LogAndNotifyError(FROM_HERE, "invalid payload_size",
                       media::VideoEncodeAccelerator::kPlatformFailureError);
     return;
   }
   output_buffers_free_count_--;
 
   // 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();
   }
-  // RTP timestamp can wrap around. Get the lower 32 bits.
-  rtp_timestamp = static_cast<uint32_t>(
-      capture_time_us * 90 / base::Time::kMicrosecondsPerMillisecond);
+
+  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.

+  }
 
   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);
   image._completeFrame = true;
 
   ReturnEncodedImage(image, bitstream_buffer_id, picture_id_);
   // Picture ID must wrap after reaching the maximum.
   picture_id_ = (picture_id_ + 1) & 0x7FFF;
 }
 
 void RTCVideoEncoder::Impl::NotifyError(
@@ skipping 58 matching lines @@
         next_frame->video_frame_buffer()->native_handle());
     requires_copy = RequiresSizeChange(frame) ||
                     frame->storage_type() != media::VideoFrame::STORAGE_SHMEM;
   } else {
     requires_copy = true;
   }
 
   if (requires_copy) {
     const base::TimeDelta timestamp =
         frame ? frame->timestamp()
               : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());

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

Do you think it is still right to use ntp_time_ms here?

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

I see no docs on what epoch media::VideoFrame::timestamp_ is 
supposed to use, so it's not clear to me what's right.

But since nothing else in this file appears to use ntp time, it looks odd.

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

This here represents the case where WebRTC dropped the reference to the Chrome's
original media::VideoFrame. We need a strictly increasing presentation timestamp
to pass to the HW encoder and we make one from ntp_time_ms(). It also looked odd
to me, but I guess we should keep it as is in case of a regression.

Also, can you let me know what to do on l.527? Should |capture_time_ms_| carry
the value from |render_time_ms|?

     base::SharedMemory* input_buffer = input_buffers_[index];
     frame = media::VideoFrame::WrapExternalSharedMemory(
         media::PIXEL_FORMAT_I420, input_frame_coded_size_,
         gfx::Rect(input_visible_size_), input_visible_size_,
         reinterpret_cast<uint8_t*>(input_buffer->memory()),
         input_buffer->mapped_size(), input_buffer->handle(), 0, timestamp);
     if (!frame.get()) {
       LogAndNotifyError(FROM_HERE, "failed to create frame",
                         media::VideoEncodeAccelerator::kPlatformFailureError);
       return;
@@ skipping 19 matching lines @@
                           frame->visible_rect().width(),
                           frame->visible_rect().height(),
                           libyuv::kFilterBox)) {
       LogAndNotifyError(FROM_HERE, "Failed to copy buffer",
                         media::VideoEncodeAccelerator::kPlatformFailureError);
       return;
     }
   }
   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);
 }
 
 void RTCVideoEncoder::Impl::EncodeFrameFinished(int index) {
   DVLOG(3) << "Impl::EncodeFrameFinished(): index=" << index;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GE(index, 0);
   DCHECK_LT(index, static_cast<int>(input_buffers_.size()));
@@ skipping 257 matching lines @@
   UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoEncoderInitEncodeSuccess",
                         init_retval == WEBRTC_VIDEO_CODEC_OK);
   if (init_retval == WEBRTC_VIDEO_CODEC_OK) {
     UMA_HISTOGRAM_ENUMERATION("Media.RTCVideoEncoderProfile",
                               profile,
                               media::VIDEO_CODEC_PROFILE_MAX + 1);
   }
 }
 
 }  // namespace content