Add VideoProcessor interface.

remoting/client/rectangle_update_decoder.cc

Index: remoting/client/rectangle_update_decoder.cc
diff --git a/remoting/client/rectangle_update_decoder.cc b/remoting/client/rectangle_update_decoder.cc
index 3482527e90435fee611247f191f1d07e0a5f70dd..bae064545e96efdce303168ef75761b70f631359 100644
--- a/remoting/client/rectangle_update_decoder.cc
+++ b/remoting/client/rectangle_update_decoder.cc
@@ -4,6 +4,8 @@
 
 #include "remoting/client/rectangle_update_decoder.h"
 
+#include <list>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
@@ -74,7 +76,74 @@ class RgbToBgrVideoDecoderFilter : public VideoDecoder {
   scoped_ptr<VideoDecoder> parent_;
 };
 
-RectangleUpdateDecoder::RectangleUpdateDecoder(
+class RectangleUpdateDecoder::Core : public base::RefCountedThreadSafe<Core> {
+ public:
+  Core(scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+       scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner,
+       scoped_refptr<FrameConsumerProxy> consumer);
+
+  // VideoProcessor implementation.
+  void Initialize(const protocol::SessionConfig& config);
+  ChromotingStats* GetStats();
+  void ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
+                                  const base::Closure& done);
+
+  // FrameProducer implementation.  These methods may be called before we are
+  // Initialize()d, or we know the source screen size.
+  void DrawBuffer(webrtc::DesktopFrame* buffer);
+  void InvalidateRegion(const webrtc::DesktopRegion& region);
+  void RequestReturnBuffers(const base::Closure& done);
+  void SetOutputSizeAndClip(
+      const webrtc::DesktopSize& view_size,
+      const webrtc::DesktopRect& clip_area);
+  const webrtc::DesktopRegion* GetBufferShape();
+
+ private:
+  friend class base::RefCountedThreadSafe<Core>;
+  virtual ~Core();
+
+  // Paints the invalidated region to the next available buffer and returns it
+  // to the consumer.
+  void SchedulePaint();
+  void DoPaint();
+
+  // Decodes the contents of |packet|. DecodePacket may keep a reference to
+  // |packet| so the |packet| must remain alive and valid until |done| is
+  // executed.
+  void DecodePacket(scoped_ptr<VideoPacket> packet, const base::Closure& done);
+
+  // Callback method when a VideoPacket is processed. |decode_start| contains
+  // the timestamp when the packet will start to be processed.
+  void OnPacketDone(base::Time decode_start, const base::Closure& done);
+
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner_;
+  scoped_refptr<FrameConsumerProxy> consumer_;
+  scoped_ptr<VideoDecoder> decoder_;
+
+  // Remote screen size in pixels.
+  webrtc::DesktopSize source_size_;
+
+  // Vertical and horizontal DPI of the remote screen.
+  webrtc::DesktopVector source_dpi_;
+
+  // The current dimensions of the frame consumer view.
+  webrtc::DesktopSize view_size_;
+  webrtc::DesktopRect clip_area_;
+
+  // The drawing buffers supplied by the frame consumer.
+  std::list<webrtc::DesktopFrame*> buffers_;
+
+  // Flag used to coalesce runs of SchedulePaint()s into a single DoPaint().
+  bool paint_scheduled_;
+
+  ChromotingStats stats_;

[Wez, 2014/01/14 16:23:14]

If you move ProcessVideoPacket and OnDecodeDone back out to
RectangleUpdateDecoder, since they are called on the main thread, then you can
also move |stats_| out there and so avoid some boilerplate in Core.

[Sergey Ulanov, 2014/01/15 00:58:17]

Done.

+
+  // Keep track of the most recent sequence number bounced back from the host.
+  int64 latest_sequence_number_;
+};
+
+RectangleUpdateDecoder::Core::Core(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner,
     scoped_refptr<FrameConsumerProxy> consumer)
@@ -85,13 +154,13 @@ RectangleUpdateDecoder::RectangleUpdateDecoder(
       latest_sequence_number_(0) {
 }
 
-RectangleUpdateDecoder::~RectangleUpdateDecoder() {
+RectangleUpdateDecoder::Core::~Core() {
 }
 
-void RectangleUpdateDecoder::Initialize(const SessionConfig& config) {
+void RectangleUpdateDecoder::Core::Initialize(const SessionConfig& config) {
   if (!decode_task_runner_->BelongsToCurrentThread()) {

[Wez, 2014/01/14 16:23:14]

Since RUD runs on main thread and RUD::Core on decode thread, why not move the
PostTask out into RUD::Initialize(), to remove the need for the
BelongsToCurrentThread() check and make the thread flow easier to reason about?

[Sergey Ulanov, 2014/01/15 00:58:17]

Done.

     decode_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RectangleUpdateDecoder::Initialize, this,
+        FROM_HERE, base::Bind(&RectangleUpdateDecoder::Core::Initialize, this,
                               config));
     return;
   }
@@ -115,7 +184,7 @@ void RectangleUpdateDecoder::Initialize(const SessionConfig& config) {
   }
 }
 
-void RectangleUpdateDecoder::DecodePacket(scoped_ptr<VideoPacket> packet,
+void RectangleUpdateDecoder::Core::DecodePacket(scoped_ptr<VideoPacket> packet,
                                           const base::Closure& done) {
   DCHECK(decode_task_runner_->BelongsToCurrentThread());
 
@@ -160,15 +229,15 @@ void RectangleUpdateDecoder::DecodePacket(scoped_ptr<VideoPacket> packet,
   }
 }
 
-void RectangleUpdateDecoder::SchedulePaint() {
+void RectangleUpdateDecoder::Core::SchedulePaint() {
   if (paint_scheduled_)
     return;
   paint_scheduled_ = true;
   decode_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&RectangleUpdateDecoder::DoPaint, this));
+      FROM_HERE, base::Bind(&RectangleUpdateDecoder::Core::DoPaint, this));
 }
 
-void RectangleUpdateDecoder::DoPaint() {
+void RectangleUpdateDecoder::Core::DoPaint() {
   DCHECK(paint_scheduled_);
   paint_scheduled_ = false;
 
@@ -184,9 +253,7 @@ void RectangleUpdateDecoder::DoPaint() {
   webrtc::DesktopFrame* buffer = buffers_.front();
   webrtc::DesktopRegion output_region;
   decoder_->RenderFrame(view_size_, clip_area_,
-                        buffer->data(),
-                        buffer->stride(),
-                        &output_region);
+                        buffer->data(), buffer->stride(), &output_region);
 
   // Notify the consumer that painting is done.
   if (!output_region.is_empty()) {
@@ -195,11 +262,12 @@ void RectangleUpdateDecoder::DoPaint() {
   }
 }
 
-void RectangleUpdateDecoder::RequestReturnBuffers(const base::Closure& done) {
+void RectangleUpdateDecoder::Core::RequestReturnBuffers(
+    const base::Closure& done) {
   if (!decode_task_runner_->BelongsToCurrentThread()) {
     decode_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RectangleUpdateDecoder::RequestReturnBuffers,
-        this, done));
+        FROM_HERE, base::Bind(
+            &RectangleUpdateDecoder::Core::RequestReturnBuffers, this, done));
     return;
   }
 
@@ -212,10 +280,10 @@ void RectangleUpdateDecoder::RequestReturnBuffers(const base::Closure& done) {
     done.Run();
 }
 
-void RectangleUpdateDecoder::DrawBuffer(webrtc::DesktopFrame* buffer) {
+void RectangleUpdateDecoder::Core::DrawBuffer(webrtc::DesktopFrame* buffer) {
   if (!decode_task_runner_->BelongsToCurrentThread()) {
     decode_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RectangleUpdateDecoder::DrawBuffer,
+        FROM_HERE, base::Bind(&RectangleUpdateDecoder::Core::DrawBuffer,
                               this, buffer));
     return;
   }
@@ -227,11 +295,11 @@ void RectangleUpdateDecoder::DrawBuffer(webrtc::DesktopFrame* buffer) {
   SchedulePaint();
 }
 
-void RectangleUpdateDecoder::InvalidateRegion(
+void RectangleUpdateDecoder::Core::InvalidateRegion(
     const webrtc::DesktopRegion& region) {
   if (!decode_task_runner_->BelongsToCurrentThread()) {
     decode_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RectangleUpdateDecoder::InvalidateRegion,
+        FROM_HERE, base::Bind(&RectangleUpdateDecoder::Core::InvalidateRegion,
                               this, region));
     return;
   }
@@ -242,13 +310,14 @@ void RectangleUpdateDecoder::InvalidateRegion(
   }
 }
 
-void RectangleUpdateDecoder::SetOutputSizeAndClip(
+void RectangleUpdateDecoder::Core::SetOutputSizeAndClip(
     const webrtc::DesktopSize& view_size,
     const webrtc::DesktopRect& clip_area) {
   if (!decode_task_runner_->BelongsToCurrentThread()) {
     decode_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RectangleUpdateDecoder::SetOutputSizeAndClip,
-                              this, view_size, clip_area));
+        FROM_HERE,
+        base::Bind(&RectangleUpdateDecoder::Core::SetOutputSizeAndClip,
+                   this, view_size, clip_area));
     return;
   }
 
@@ -281,12 +350,13 @@ void RectangleUpdateDecoder::SetOutputSizeAndClip(
   }
 }
 
-const webrtc::DesktopRegion* RectangleUpdateDecoder::GetBufferShape() {
+const webrtc::DesktopRegion* RectangleUpdateDecoder::Core::GetBufferShape() {
   return decoder_->GetImageShape();
 }
 
-void RectangleUpdateDecoder::ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
-                                                const base::Closure& done) {
+void RectangleUpdateDecoder::Core::ProcessVideoPacket(
+    scoped_ptr<VideoPacket> packet,
+    const base::Closure& done) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   // If the video packet is empty then drop it. Empty packets are used to
@@ -318,18 +388,18 @@ void RectangleUpdateDecoder::ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
   base::Time decode_start = base::Time::Now();
 
   base::Closure decode_done = base::Bind(
-      &RectangleUpdateDecoder::OnPacketDone, this, decode_start, done);
+      &RectangleUpdateDecoder::Core::OnPacketDone, this, decode_start, done);

[Wez, 2014/01/14 16:23:14]

You can put all this code in RectangleUpdateDecoder::ProcessVideoPacket, since
it all runs on |main_task_runner_| (though stats will need moving out to
RectangleUpdateDecoder as well).

[Sergey Ulanov, 2014/01/15 00:58:17]

Done.

 
   decode_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &RectangleUpdateDecoder::DecodePacket, this,
+      &RectangleUpdateDecoder::Core::DecodePacket, this,
       base::Passed(&packet), decode_done));
 }
 
-void RectangleUpdateDecoder::OnPacketDone(base::Time decode_start,
-                                          const base::Closure& done) {
+void RectangleUpdateDecoder::Core::OnPacketDone(base::Time decode_start,
+                                                const base::Closure& done) {
   if (!main_task_runner_->BelongsToCurrentThread()) {

[Wez, 2014/01/14 16:23:14]

Since this method just thread-hops back to |main_task_runner_| to update the
stats and then runs |done| there, it would be cleaner to put this on
RectangleUpdateDecoder, to keep a clean separation between |decoder_task_runner|
and |main_task_runner| code. You'd need to PostTask() through a WeakPtr to the
RUD to do that, though.

[Sergey Ulanov, 2014/01/15 00:58:17]

Done.

     main_task_runner_->PostTask(FROM_HERE, base::Bind(
-        &RectangleUpdateDecoder::OnPacketDone, this,
+        &RectangleUpdateDecoder::Core::OnPacketDone, this,
         decode_start, done));
     return;
   }
@@ -341,9 +411,53 @@ void RectangleUpdateDecoder::OnPacketDone(base::Time decode_start,
   done.Run();
 }
 
-ChromotingStats* RectangleUpdateDecoder::GetStats() {
+ChromotingStats* RectangleUpdateDecoder::Core::GetStats() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   return &stats_;
 }
 
+RectangleUpdateDecoder::RectangleUpdateDecoder(
+    scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner,
+    scoped_refptr<FrameConsumerProxy> consumer)
+    : core_(new Core(main_task_runner, decode_task_runner, consumer)) {
+}
+
+void RectangleUpdateDecoder::Initialize(const protocol::SessionConfig& config) {
+  core_->Initialize(config);
+}
+
+ChromotingStats* RectangleUpdateDecoder::GetStats() {
+  return core_->GetStats();
+}
+
+void RectangleUpdateDecoder::ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
+                                                const base::Closure& done) {
+  core_->ProcessVideoPacket(packet.Pass(), done);

[Wez, 2014/01/14 16:23:14]

As with Initialize, where we're just proxying calls from the main thread into
the Core on the decode thread, why not just PostTask from the RUD::Foo method to
the RUD::Core::Foo implementation, and avoid the BelongsToCurrentThread check?
Only down-side is that we'll PostTask even if the threads are the same, but that
will never happen in practice, and I suspect the performance hit is negligible
compared to decode.

[Sergey Ulanov, 2014/01/15 00:58:17]

Done.

+}
+
+void RectangleUpdateDecoder::DrawBuffer(webrtc::DesktopFrame* buffer) {
+  core_->DrawBuffer(buffer);
+}
+
+void RectangleUpdateDecoder::InvalidateRegion(
+    const webrtc::DesktopRegion& region) {
+  core_->InvalidateRegion(region);
+}
+
+void RectangleUpdateDecoder::RequestReturnBuffers(const base::Closure& done) {
+  core_->RequestReturnBuffers(done);
+}
+
+void RectangleUpdateDecoder::SetOutputSizeAndClip(
+    const webrtc::DesktopSize& view_size,
+    const webrtc::DesktopRect& clip_area) {
+  core_->SetOutputSizeAndClip(view_size, clip_area);
+}
+
+const webrtc::DesktopRegion* RectangleUpdateDecoder::GetBufferShape() {
+  return core_->GetBufferShape();
+}
+
+
 }  // namespace remoting