Rework PerformanceTracker to make it usable with WebRTC

remoting/client/plugin/pepper_video_renderer_3d.cc

Index: remoting/client/plugin/pepper_video_renderer_3d.cc
diff --git a/remoting/client/plugin/pepper_video_renderer_3d.cc b/remoting/client/plugin/pepper_video_renderer_3d.cc
index fb00b23f38f86e7d9b2a33add13e02a02c030ff7..a1f1633316efb2e0864f158b80330fb49bcae792 100644
--- a/remoting/client/plugin/pepper_video_renderer_3d.cc
+++ b/remoting/client/plugin/pepper_video_renderer_3d.cc
@@ -6,10 +6,9 @@
 
 #include <math.h>
 
+#include <algorithm>
 #include <utility>
 
-#include "base/callback_helpers.h"
-#include "base/stl_util.h"
 #include "ppapi/c/pp_codecs.h"
 #include "ppapi/c/ppb_opengles2.h"
 #include "ppapi/c/ppb_video_decoder.h"
@@ -17,6 +16,7 @@
 #include "ppapi/lib/gl/include/GLES2/gl2.h"
 #include "ppapi/lib/gl/include/GLES2/gl2ext.h"
 #include "remoting/proto/video.pb.h"
+#include "remoting/protocol/frame_stats.h"
 #include "remoting/protocol/performance_tracker.h"
 #include "remoting/protocol/session_config.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
@@ -28,26 +28,35 @@ namespace remoting {
 // 1.1, so we have to pass 0 for backwards compatibility with older versions of
 // the browser. Currently all API implementations allocate more than 3 buffers
 // by default.
-//
-// TODO(sergeyu): Change this to 3 once PPB_VideoDecoder v1.1 is enabled on
-// stable channel (crbug.com/520323).
-const uint32_t kMinimumPictureCount = 0;  // 3
+const uint32_t kMinimumPictureCount = 3;
 
-class PepperVideoRenderer3D::PendingPacket {
+class PepperVideoRenderer3D::FrameTracker {
  public:
-  PendingPacket(std::unique_ptr<VideoPacket> packet, const base::Closure& done)
-      : packet_(std::move(packet)), done_runner_(done) {}
+  FrameTracker(std::unique_ptr<VideoPacket> packet,
+               protocol::PerformanceTracker* perf_tracker,
+               const base::Closure& done)
+      : packet_(std::move(packet)), perf_tracker_(perf_tracker), done_(done) {
+    stats_ = protocol::FrameStats::GetForVideoPacket(*packet_);
+  }
 
-  ~PendingPacket() {}
+  ~FrameTracker() {
+    stats_.time_rendered = base::TimeTicks::Now();

[Jamie, 2016/06/24 23:53:21]

Unless you feel strongly otherwise, I think this would be better in an explicit
OnRendered method. This sort of thing is okay in a dtor if the class is defined
for use within a scope, but it seems a bit hacky here.

Failing that, at least add a comment when the list is emptied so it's clear that
something non-trivial is happening as a result.

[Sergey Ulanov, 2016/06/25 05:34:03]

Done.

+    if (perf_tracker_)
+      perf_tracker_->RecordVideoFrameStats(stats_);
+    if (!done_.is_null())
+      done_.Run();
+  }
 
-  const VideoPacket* packet() const { return packet_.get(); }
+  void OnDecoded() { stats_.time_decoded = base::TimeTicks::Now(); }
+  VideoPacket* packet() { return packet_.get(); }
 
  private:
   std::unique_ptr<VideoPacket> packet_;
-  base::ScopedClosureRunner done_runner_;
+  protocol::PerformanceTracker* perf_tracker_;
+  protocol::FrameStats stats_;
+  base::Closure done_;
 };
 
-
 class PepperVideoRenderer3D::Picture {
  public:
   Picture(pp::VideoDecoder* decoder, PP_VideoPicture picture)
@@ -66,8 +75,6 @@ PepperVideoRenderer3D::PepperVideoRenderer3D() : callback_factory_(this) {}
 PepperVideoRenderer3D::~PepperVideoRenderer3D() {
   if (shader_program_)
     gles2_if_->DeleteProgram(graphics_.pp_resource(), shader_program_);
-
-  STLDeleteElements(&pending_packets_);
 }
 
 bool PepperVideoRenderer3D::Initialize(
@@ -189,13 +196,13 @@ protocol::FrameConsumer* PepperVideoRenderer3D::GetFrameConsumer() {
 void PepperVideoRenderer3D::ProcessVideoPacket(
     std::unique_ptr<VideoPacket> packet,
     const base::Closure& done) {
-  base::ScopedClosureRunner done_runner(done);
-
-  perf_tracker_->RecordVideoPacketStats(*packet);
+  VideoPacket* packet_ptr = packet.get();
+  std::unique_ptr<FrameTracker> frame_tracker(
+      new FrameTracker(std::move(packet), perf_tracker_, done));
 
   // Don't need to do anything if the packet is empty. Host sends empty video
   // packets when the screen is not changing.
-  if (!packet->data().size())
+  if (!packet_ptr->data().size())
     return;
 
   if (!frame_received_) {
@@ -203,26 +210,25 @@ void PepperVideoRenderer3D::ProcessVideoPacket(
     frame_received_ = true;
   }
 
-  if (packet->format().has_screen_width() &&
-      packet->format().has_screen_height()) {
-    frame_size_.set(packet->format().screen_width(),
-                    packet->format().screen_height());
+  if (packet_ptr->format().has_screen_width() &&
+      packet_ptr->format().has_screen_height()) {
+    frame_size_.set(packet_ptr->format().screen_width(),
+                    packet_ptr->format().screen_height());
   }
 
   // Report the dirty region, for debugging, if requested.
   if (debug_dirty_region_) {
     webrtc::DesktopRegion dirty_region;
-    for (int i = 0; i < packet->dirty_rects_size(); ++i) {
-      Rect remoting_rect = packet->dirty_rects(i);
+    for (int i = 0; i < packet_ptr->dirty_rects_size(); ++i) {
+      Rect remoting_rect = packet_ptr->dirty_rects(i);
       dirty_region.AddRect(webrtc::DesktopRect::MakeXYWH(
-          remoting_rect.x(), remoting_rect.y(),
-          remoting_rect.width(), remoting_rect.height()));
+          remoting_rect.x(), remoting_rect.y(), remoting_rect.width(),
+          remoting_rect.height()));
     }
     event_handler_->OnVideoFrameDirtyRegion(dirty_region);
   }
 
-  pending_packets_.push_back(
-      new PendingPacket(std::move(packet), done_runner.Release()));
+  pending_frames_.push_back(std::move(frame_tracker));
   DecodeNextPacket();
 }
 
@@ -236,10 +242,10 @@ void PepperVideoRenderer3D::OnInitialized(int32_t result) {
 }
 
 void PepperVideoRenderer3D::DecodeNextPacket() {
-  if (!initialization_finished_ || decode_pending_ || pending_packets_.empty())
+  if (!initialization_finished_ || decode_pending_ || pending_frames_.empty())
     return;
 
-  const VideoPacket* packet = pending_packets_.front()->packet();
+  const VideoPacket* packet = pending_frames_.front()->packet();
 
   int32_t result = video_decoder_.Decode(
       packet->frame_id(), packet->data().size(), packet->data().data(),
@@ -258,8 +264,10 @@ void PepperVideoRenderer3D::OnDecodeDone(int32_t result) {
     return;
   }
 
-  delete pending_packets_.front();
-  pending_packets_.pop_front();
+  pending_frames_.front()->OnDecoded();
+  // Move the frame from |pending_frames_| to |decoded_frames_|.
+  decoded_frames_.splice(decoded_frames_.end(), pending_frames_,
+                         pending_frames_.begin());
 
   DecodeNextPacket();
   GetNextPicture();
@@ -287,8 +295,6 @@ void PepperVideoRenderer3D::OnPictureReady(int32_t result,
     return;
   }
 
-  perf_tracker_->OnFrameDecoded(picture.decode_id);
-
   // Workaround crbug.com/542945 by filling in visible_rect if it isn't set.
   if (picture.visible_rect.size.width == 0 ||
       picture.visible_rect.size.height == 0) {
@@ -304,6 +310,13 @@ void PepperVideoRenderer3D::OnPictureReady(int32_t result,
         std::min(frame_size_.height(), picture.texture_size.height);
   }
 
+  DCHECK_EQ(static_cast<int32_t>(picture.decode_id),
+            decoded_frames_.front()->packet()->frame_id());
+
+  // Move the frame from |decoded_frames_| to |next_picture_frames_|.
+  next_picture_frames_.splice(next_picture_frames_.end(), decoded_frames_,
+                              decoded_frames_.begin());
+
   next_picture_.reset(new Picture(&video_decoder_, picture));
 
   PaintIfNeeded();
@@ -315,8 +328,11 @@ void PepperVideoRenderer3D::PaintIfNeeded() {
   if (paint_pending_ || !need_repaint)
     return;
 
-  if (next_picture_)
+  if (next_picture_) {
     current_picture_ = std::move(next_picture_);
+    current_picture_frames_.splice(current_picture_frames_.end(),
+                                   next_picture_frames_);
+  }
 
   force_repaint_ = false;
 
@@ -382,7 +398,7 @@ void PepperVideoRenderer3D::OnPaintDone(int32_t result) {
   CHECK_EQ(result, PP_OK) << "Graphics3D::SwapBuffers() failed";
 
   paint_pending_ = false;
-  perf_tracker_->OnFramePainted(current_picture_->picture().decode_id);
+  current_picture_frames_.clear();
   PaintIfNeeded();
 }