[remoting host] Track frame statistics per frame.

With the standard encoding pipeline, the scheduler may start capturing
a new frame before the current frame has been encoded and sent. This
means that storing the stats in WebrtcVideoStream::current_frame_stats_
is inadequate. This CL enables the frame stats to be attached to each
frame, before sending it to WebRTC's output sink. The stats will be
extracted from the frame by the new encoder, then re-attached to the
corresponding EncodedFrame after encoding.

In the current legacy code, the stats are attached manually in
WebrtcVideoStream::OnFrameEncoded(). In the new code, the stats
will be attached in WebrtcVideoStream::OnCapturedFrame().

Because the stats are attached via std::unique_ptr, this CL makes
EncodedFrame into a movable-but-not-copyable type, which makes
sense anyway since the frame data can be very large.

Bug: 1192865
Change-Id: Ie8eaa03e0912009d407eafca2f90de92a1f52845
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2911526
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#886533}

remoting/codec/webrtc_video_encoder.cc

@@ -9,8 +9,8 @@ namespace remoting {
 WebrtcVideoEncoder::EncodedFrame::EncodedFrame() = default;
 WebrtcVideoEncoder::EncodedFrame::~EncodedFrame() = default;
 WebrtcVideoEncoder::EncodedFrame::EncodedFrame(
-    const WebrtcVideoEncoder::EncodedFrame&) = default;
+    WebrtcVideoEncoder::EncodedFrame&&) = default;
 WebrtcVideoEncoder::EncodedFrame& WebrtcVideoEncoder::EncodedFrame::operator=(
-    const WebrtcVideoEncoder::EncodedFrame&) = default;
+    WebrtcVideoEncoder::EncodedFrame&&) = default;
 
 }  // namespace remoting

remoting/codec/webrtc_video_encoder.h

@@ -50,11 +50,26 @@ class WebrtcVideoEncoder {
     int vpx_max_quantizer = -1;
   };
 
+  // Information needed for sending video statistics for each encoded frame.
+  // The destructor is virtual so that implementations can derive from this
+  // class to attach more data to the frame.
+  struct FrameStats {
+    FrameStats() = default;
+    FrameStats(const FrameStats&) = default;
+    FrameStats& operator=(const FrameStats&) = default;
+    virtual ~FrameStats() = default;
+
+    base::TimeTicks encode_started_time;
+    base::TimeTicks encode_ended_time;
+  };
+
   struct EncodedFrame {
     EncodedFrame();
+    EncodedFrame(const EncodedFrame&) = delete;
+    EncodedFrame(EncodedFrame&&);
+    EncodedFrame& operator=(const EncodedFrame&) = delete;
+    EncodedFrame& operator=(EncodedFrame&&);
     ~EncodedFrame();
-    EncodedFrame(const EncodedFrame&);
-    EncodedFrame& operator=(const EncodedFrame&);
 
     webrtc::DesktopSize size;
     std::string data;
@@ -62,8 +77,12 @@ class WebrtcVideoEncoder {
     int quantizer;
     webrtc::VideoCodecType codec;
 
-    // Information provided to WebRTC when sending the frame via the
-    // webrtc::EncodedImageCallback.
+    std::unique_ptr<FrameStats> stats;
+
+    // These fields are needed by
+    // WebrtcDummyVideoEncoderFactory::SendEncodedFrame().
+    // TODO(crbug.com/1192865): Remove them when standard encoding pipeline is
+    // implemented.
     base::TimeTicks capture_time;
     base::TimeTicks encode_start;
     base::TimeTicks encode_finish;

remoting/protocol/input_event_timestamps.h

@@ -20,7 +20,7 @@ struct InputEventTimestamps {
   // Time when the event was processed by the host.
   base::TimeTicks host_timestamp;
 
-  bool is_null() { return client_timestamp.is_null(); }
+  bool is_null() const { return client_timestamp.is_null(); }
 };
 
 // InputEventTimestampsSource is used by VideoStream implementations to get

remoting/protocol/webrtc_video_encoder_wrapper.cc

@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <functional>
 #include <string>
 #include <vector>
 
@@ -160,9 +161,19 @@ int32_t WebrtcVideoEncoderWrapper::Encode(
   auto* video_frame_adapter =
       static_cast<WebrtcVideoFrameAdapter*>(frame.video_frame_buffer().get());
 
-  // Store timestamp so it can be added to the EncodedImage when encoding is
-  // complete.
+  // Store RTP timestamp and FrameStats so they can be added to the
+  // EncodedImage and EncodedFrame when encoding is complete.
   rtp_timestamp_ = frame.timestamp();
+  frame_stats_ = video_frame_adapter->TakeFrameStats();
+  if (!frame_stats_) {
+    // This could happen if WebRTC tried to encode the same frame twice.
+    // Taking the frame-stats twice from the same frame-adapter would return
+    // nullptr the second time.
+    LOG(ERROR) << "Frame provided with missing frame-stats.";
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
+  frame_stats_->encode_started_time = base::TimeTicks::Now();
 
   // TODO(crbug.com/1192865): Implement large-frame detection for VP8, and
   // ensure VP9 is configured to do this automatically. If the frame has a
@@ -313,6 +324,16 @@ void WebrtcVideoEncoderWrapper::OnFrameEncoded(
   DCHECK(encode_pending_);
   encode_pending_ = false;
 
+  if (frame) {
+    // This is non-null because the |encode_pending_| flag ensures that
+    // frame-encodings are serialized. So there cannot be 2 consecutive calls to
+    // this method without an intervening call to Encode() which sets
+    // |frame_stats_| to non-null.
+    DCHECK(frame_stats_);
+    frame_stats_->encode_ended_time = base::TimeTicks::Now();
+    frame->stats = std::move(frame_stats_);
+  }
+
   main_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&VideoChannelStateObserver::OnFrameEncoded,
                                 video_channel_state_observer_, encode_result,
@@ -341,10 +362,15 @@ void WebrtcVideoEncoderWrapper::OnFrameEncoded(
   DCHECK(encoded_callback_);
 
   webrtc::EncodedImageCallback::Result send_result = ReturnEncodedFrame(*frame);
+
+  // std::ref() is used here because base::BindOnce() would otherwise try to
+  // copy the referenced frame object, which is move-only. This is safe because
+  // base::OnTaskRunnerDeleter posts the frame-deleter task to run after this
+  // task has executed.
   main_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&VideoChannelStateObserver::OnEncodedFrameSent,
-                     video_channel_state_observer_, send_result, *frame));
+      FROM_HERE, base::BindOnce(&VideoChannelStateObserver::OnEncodedFrameSent,
+                                video_channel_state_observer_, send_result,
+                                std::ref(*frame)));
 }
 
 void WebrtcVideoEncoderWrapper::NotifyFrameDropped() {

remoting/protocol/webrtc_video_encoder_wrapper.h

@@ -76,6 +76,10 @@ class WebrtcVideoEncoderWrapper : public webrtc::VideoEncoder {
   // passes to Encode().
   uint32_t rtp_timestamp_ GUARDED_BY_CONTEXT(sequence_checker_);
 
+  // FrameStats taken from the input VideoFrameAdapter, then added to the
+  // EncodedFrame when encoding is complete.
+  std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats_;
+
   // Bandwidth estimate from SetRates(), which is expected to be called before
   // Encode().
   int bitrate_kbps_ GUARDED_BY_CONTEXT(sequence_checker_) = 0;

remoting/protocol/webrtc_video_encoder_wrapper_unittest.cc

@@ -79,8 +79,10 @@ VideoCodec GetVp9Codec() {
 VideoFrame MakeVideoFrame() {
   DesktopSize size(kInputFrameWidth, kInputFrameHeight);
   auto frame = std::make_unique<BasicDesktopFrame>(size);
+  auto stats = std::make_unique<WebrtcVideoEncoder::FrameStats>();
   frame->mutable_updated_region()->SetRect(webrtc::DesktopRect::MakeSize(size));
-  return WebrtcVideoFrameAdapter::CreateVideoFrame(std::move(frame));
+  return WebrtcVideoFrameAdapter::CreateVideoFrame(std::move(frame),
+                                                   std::move(stats));
 }
 
 class MockVideoChannelStateObserver : public VideoChannelStateObserver {

remoting/protocol/webrtc_video_frame_adapter.cc

@@ -13,17 +13,21 @@ namespace remoting {
 namespace protocol {
 
 WebrtcVideoFrameAdapter::WebrtcVideoFrameAdapter(
-    std::unique_ptr<webrtc::DesktopFrame> frame)
-    : frame_(std::move(frame)), frame_size_(frame_->size()) {}
+    std::unique_ptr<webrtc::DesktopFrame> frame,
+    std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats)
+    : frame_(std::move(frame)),
+      frame_size_(frame_->size()),
+      frame_stats_(std::move(frame_stats)) {}
 
 WebrtcVideoFrameAdapter::~WebrtcVideoFrameAdapter() = default;
 
 // static
 webrtc::VideoFrame WebrtcVideoFrameAdapter::CreateVideoFrame(
-    std::unique_ptr<webrtc::DesktopFrame> desktop_frame) {
+    std::unique_ptr<webrtc::DesktopFrame> desktop_frame,
+    std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats) {
   rtc::scoped_refptr<WebrtcVideoFrameAdapter> adapter =
       new rtc::RefCountedObject<WebrtcVideoFrameAdapter>(
-          std::move(desktop_frame));
+          std::move(desktop_frame), std::move(frame_stats));
   return webrtc::VideoFrame::Builder().set_video_frame_buffer(adapter).build();
 }
 
@@ -32,6 +36,11 @@ WebrtcVideoFrameAdapter::TakeDesktopFrame() {
   return std::move(frame_);
 }
 
+std::unique_ptr<WebrtcVideoEncoder::FrameStats>
+WebrtcVideoFrameAdapter::TakeFrameStats() {
+  return std::move(frame_stats_);
+}
+
 webrtc::VideoFrameBuffer::Type WebrtcVideoFrameAdapter::type() const {
   return Type::kNative;
 }

remoting/protocol/webrtc_video_frame_adapter.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "remoting/codec/webrtc_video_encoder.h"
 #include "third_party/webrtc/api/video/video_frame.h"
 #include "third_party/webrtc/api/video/video_frame_buffer.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
@@ -19,19 +20,27 @@ namespace protocol {
 // captured DesktopFrame from VideoFrame::video_frame_buffer().
 class WebrtcVideoFrameAdapter : public webrtc::VideoFrameBuffer {
  public:
-  explicit WebrtcVideoFrameAdapter(std::unique_ptr<webrtc::DesktopFrame> frame);
+  WebrtcVideoFrameAdapter(
+      std::unique_ptr<webrtc::DesktopFrame> frame,
+      std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats);
   ~WebrtcVideoFrameAdapter() override;
   WebrtcVideoFrameAdapter(const WebrtcVideoFrameAdapter&) = delete;
   WebrtcVideoFrameAdapter& operator=(const WebrtcVideoFrameAdapter&) = delete;
 
   // Returns a VideoFrame that wraps the provided DesktopFrame.
   static webrtc::VideoFrame CreateVideoFrame(
-      std::unique_ptr<webrtc::DesktopFrame> desktop_frame);
+      std::unique_ptr<webrtc::DesktopFrame> desktop_frame,
+      std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats);
 
   // Used by the encoder. After this returns, the adapter no longer wraps a
   // DesktopFrame.
   std::unique_ptr<webrtc::DesktopFrame> TakeDesktopFrame();
 
+  // Called by the encoder to transfer the frame stats out of this adapter
+  // into the EncodedFrame. The encoder will also set the encode start/end
+  // times.
+  std::unique_ptr<WebrtcVideoEncoder::FrameStats> TakeFrameStats();
+
   // webrtc::VideoFrameBuffer overrides.
   Type type() const override;
   int width() const override;
@@ -41,6 +50,7 @@ class WebrtcVideoFrameAdapter : public webrtc::VideoFrameBuffer {
  private:
   std::unique_ptr<webrtc::DesktopFrame> frame_;
   webrtc::DesktopSize frame_size_;
+  std::unique_ptr<WebrtcVideoEncoder::FrameStats> frame_stats_;
 };
 
 }  // namespace protocol

remoting/protocol/webrtc_video_frame_adapter_unittest.cc

@@ -27,10 +27,11 @@ namespace protocol {
 
 TEST(WebrtcVideoFrameAdapter, CreateVideoFrameWrapsDesktopFrame) {
   auto desktop_frame = MakeDesktopFrame(100, 200);
+  auto frame_stats = std::make_unique<WebrtcVideoEncoder::FrameStats>();
   DesktopFrame* desktop_frame_ptr = desktop_frame.get();
 
-  webrtc::VideoFrame video_frame =
-      WebrtcVideoFrameAdapter::CreateVideoFrame(std::move(desktop_frame));
+  webrtc::VideoFrame video_frame = WebrtcVideoFrameAdapter::CreateVideoFrame(
+      std::move(desktop_frame), std::move(frame_stats));
 
   auto* adapter = static_cast<WebrtcVideoFrameAdapter*>(
       video_frame.video_frame_buffer().get());
@@ -40,9 +41,10 @@ TEST(WebrtcVideoFrameAdapter, CreateVideoFrameWrapsDesktopFrame) {
 
 TEST(WebrtcVideoFrameAdapter, AdapterHasCorrectSize) {
   auto desktop_frame = MakeDesktopFrame(100, 200);
+  auto frame_stats = std::make_unique<WebrtcVideoEncoder::FrameStats>();
   rtc::scoped_refptr<WebrtcVideoFrameAdapter> adapter =
       new rtc::RefCountedObject<WebrtcVideoFrameAdapter>(
-          std::move(desktop_frame));
+          std::move(desktop_frame), std::move(frame_stats));
 
   EXPECT_EQ(100, adapter->width());
   EXPECT_EQ(200, adapter->height());

remoting/protocol/webrtc_video_stream.cc

@@ -52,16 +52,19 @@ std::string EncodeResultToString(WebrtcVideoEncoder::EncodeResult result) {
 
 }  // namespace
 
-struct WebrtcVideoStream::FrameStats {
-  // The following fields are non-null only for one frame after each incoming
-  // input event.
+struct WebrtcVideoStream::FrameStats : public WebrtcVideoEncoder::FrameStats {
+  FrameStats() = default;
+  FrameStats(const FrameStats&) = default;
+  FrameStats& operator=(const FrameStats&) = default;
+  ~FrameStats() override = default;
+
+  // The input-event fields are non-null only for one frame after each
+  // incoming input event.
   InputEventTimestamps input_event_timestamps;
 
   base::TimeTicks capture_started_time;
   base::TimeTicks capture_ended_time;
   base::TimeDelta capture_delay;
-  base::TimeTicks encode_started_time;
-  base::TimeTicks encode_ended_time;
 
   uint32_t capturer_id = 0;
 };
@@ -308,9 +311,16 @@ void WebrtcVideoStream::OnFrameEncoded(
     return;
   }
 
+  // These next 3 lines are needed by
+  // WebrtcDummyVideoEncoderFactory::SendEncodedFrame(), which adds the
+  // timestamps to the frame sent to WebRTC.
+  // TODO(crbug.com/1192865): Remove them when standard encoding pipeline is
+  // implemented, as they will no longer be required by WebRTC.
   frame->capture_time = current_frame_stats_->capture_started_time;
   frame->encode_start = current_frame_stats_->encode_started_time;
   frame->encode_finish = current_frame_stats_->encode_ended_time;
+
+  frame->stats = std::move(current_frame_stats_);
   webrtc::EncodedImageCallback::Result result =
       webrtc_transport_->video_encoder_factory()->SendEncodedFrame(*frame);
 
@@ -331,6 +341,12 @@ void WebrtcVideoStream::OnEncodedFrameSent(
 
   // Send FrameStats message.
   if (video_stats_dispatcher_.is_connected()) {
+    // The down-cast is safe, because the |stats| object was originally created
+    // by this class and attached to the frame.
+    const auto* current_frame_stats =
+        static_cast<const FrameStats*>(frame.stats.get());
+    DCHECK(current_frame_stats);
+
     HostFrameStats stats;
 
     // Get bandwidth, RTT and send_pending_delay into |stats|.
@@ -338,29 +354,28 @@ void WebrtcVideoStream::OnEncodedFrameSent(
 
     stats.frame_size = frame.data.size();
 
-    if (!current_frame_stats_->input_event_timestamps.is_null()) {
+    if (!current_frame_stats->input_event_timestamps.is_null()) {
       stats.capture_pending_delay =
-          current_frame_stats_->capture_started_time -
-          current_frame_stats_->input_event_timestamps.host_timestamp;
+          current_frame_stats->capture_started_time -
+          current_frame_stats->input_event_timestamps.host_timestamp;
       stats.latest_event_timestamp =
-          current_frame_stats_->input_event_timestamps.client_timestamp;
+          current_frame_stats->input_event_timestamps.client_timestamp;
     }
 
-    stats.capture_delay = current_frame_stats_->capture_delay;
+    stats.capture_delay = current_frame_stats->capture_delay;
 
     // Total overhead time for IPC and threading when capturing frames.
-    stats.capture_overhead_delay =
-        (current_frame_stats_->capture_ended_time -
-         current_frame_stats_->capture_started_time) -
-        stats.capture_delay;
+    stats.capture_overhead_delay = (current_frame_stats->capture_ended_time -
+                                    current_frame_stats->capture_started_time) -
+                                   stats.capture_delay;
 
-    stats.encode_pending_delay = current_frame_stats_->encode_started_time -
-                                 current_frame_stats_->capture_ended_time;
+    stats.encode_pending_delay = current_frame_stats->encode_started_time -
+                                 current_frame_stats->capture_ended_time;
 
-    stats.encode_delay = current_frame_stats_->encode_ended_time -
-                         current_frame_stats_->encode_started_time;
+    stats.encode_delay = current_frame_stats->encode_ended_time -
+                         current_frame_stats->encode_started_time;
 
-    stats.capturer_id = current_frame_stats_->capturer_id;
+    stats.capturer_id = current_frame_stats->capturer_id;
 
     // Convert the frame quantizer to a measure of frame quality between 0 and
     // 100, for a simple visualization of quality over time. The quantizer from