Cleanup RectangleUpdateDecoder and VideoStub

Previously RectangleUpdateDecoder maintaned list of pending packets. That's not really necessary.
Simplified it by queue tasks on the decode thread. Also some minor cleanups for the VideoStub
interface - removed GetPendingVideoPackets() that wasn't used anywhere.

Review URL: https://chromiumcodereview.appspot.com/10879085

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@154272 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 18 insertions, 108 deletions

diff --git a/remoting/client/chromoting_client.cc b/remoting/client/chromoting_client.cc
index 3bdc7b6..388bed5 100644
--- a/remoting/client/chromoting_client.cc
+++ b/remoting/client/chromoting_client.cc
@@ -67,9 +67,6 @@ void ChromotingClient::Start(
 void ChromotingClient::Stop(const base::Closure& shutdown_task) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  // Drop all pending packets.
-  rectangle_decoder_->DropAllPackets();
-
   connection_->Disconnect(base::Bind(&ChromotingClient::OnDisconnected,
                                      weak_ptr_, shutdown_task));
 }
diff --git a/remoting/client/rectangle_update_decoder.cc b/remoting/client/rectangle_update_decoder.cc
index c7a1404..3a7fddc 100644
--- a/remoting/client/rectangle_update_decoder.cc
+++ b/remoting/client/rectangle_update_decoder.cc
@@ -6,9 +6,10 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/location.h"
 #include "base/logging.h"
-#include "base/message_loop_proxy.h"
+#include "base/single_thread_task_runner.h"
 #include "ppapi/cpp/image_data.h"
 #include "remoting/base/util.h"
 #include "remoting/codec/video_decoder.h"
@@ -23,16 +24,6 @@ using remoting::protocol::SessionConfig;
 
 namespace remoting {
 
-RectangleUpdateDecoder::QueuedVideoPacket::QueuedVideoPacket(
-    scoped_ptr<VideoPacket> packet,
-    const base::Closure& done)
-    : packet(packet.release()),
-      done(done) {
-}
-
-RectangleUpdateDecoder::QueuedVideoPacket::~QueuedVideoPacket() {
-}
-
 RectangleUpdateDecoder::RectangleUpdateDecoder(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner,
@@ -45,7 +36,6 @@ RectangleUpdateDecoder::RectangleUpdateDecoder(
       view_size_(SkISize::Make(0, 0)),
       clip_area_(SkIRect::MakeEmpty()),
       paint_scheduled_(false),
-      packet_being_processed_(false),
       latest_sequence_number_(0) {
 }
 
@@ -71,6 +61,7 @@ void RectangleUpdateDecoder::DecodePacket(scoped_ptr<VideoPacket> packet,
   DCHECK(decode_task_runner_->BelongsToCurrentThread());
 
   base::ScopedClosureRunner done_runner(done);
+
   bool decoder_needs_reset = false;
   bool notify_size_or_dpi_change = false;
 
@@ -262,61 +253,28 @@ void RectangleUpdateDecoder::ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
     stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds());
   }
 
-  received_packets_.push_back(QueuedVideoPacket(packet.Pass(), done));
-  if (!packet_being_processed_)
-    ProcessNextPacket();
-}
-
-int RectangleUpdateDecoder::GetPendingVideoPackets() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-  return received_packets_.size();
-}
-
-void RectangleUpdateDecoder::DropAllPackets() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-
-  while(!received_packets_.empty()) {
-    delete received_packets_.front().packet;
-    received_packets_.front().done.Run();
-    received_packets_.pop_front();
-  }
-}
-
-void RectangleUpdateDecoder::ProcessNextPacket() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-  CHECK(!packet_being_processed_);
-
-  if (received_packets_.empty()) {
-    // Nothing to do!
-    return;
-  }
-
-  scoped_ptr<VideoPacket> packet(received_packets_.front().packet);
-  received_packets_.front().packet = NULL;
-  packet_being_processed_ = true;
-
   // Measure the latency between the last packet being received and presented.
   bool last_packet = (packet->flags() & VideoPacket::LAST_PACKET) != 0;
   base::Time decode_start;
   if (last_packet)
     decode_start = base::Time::Now();
 
-  base::Closure callback = base::Bind(&RectangleUpdateDecoder::OnPacketDone,
-                                      this,
-                                      last_packet,
-                                      decode_start);
+  base::Closure decode_done =
+      base::Bind(&RectangleUpdateDecoder::OnPacketDone, this,
+                 last_packet, decode_start, done);
 
   decode_task_runner_->PostTask(FROM_HERE, base::Bind(
       &RectangleUpdateDecoder::DecodePacket, this,
-      base::Passed(&packet), callback));
+      base::Passed(&packet), decode_done));
 }
 
 void RectangleUpdateDecoder::OnPacketDone(bool last_packet,
-                                          base::Time decode_start) {
+                                          base::Time decode_start,
+                                          const base::Closure& done) {
   if (!main_task_runner_->BelongsToCurrentThread()) {
     main_task_runner_->PostTask(FROM_HERE, base::Bind(
         &RectangleUpdateDecoder::OnPacketDone, this,
-        last_packet, decode_start));
+        last_packet, decode_start, done));
     return;
   }
 
@@ -327,13 +285,7 @@ void RectangleUpdateDecoder::OnPacketDone(bool last_packet,
         (base::Time::Now() - decode_start).InMilliseconds());
   }
 
-  received_packets_.front().done.Run();
-  received_packets_.pop_front();
-
-  packet_being_processed_ = false;
-
-  // Process the next video packet.
-  ProcessNextPacket();
+  done.Run();
 }
 
 ChromotingStats* RectangleUpdateDecoder::GetStats() {
diff --git a/remoting/client/rectangle_update_decoder.h b/remoting/client/rectangle_update_decoder.h
index af91267..5eefcad 100644
--- a/remoting/client/rectangle_update_decoder.h
+++ b/remoting/client/rectangle_update_decoder.h
@@ -7,7 +7,7 @@
 
 #include <list>
 
-#include "base/callback.h"
+#include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "remoting/codec/video_decoder.h"
@@ -22,12 +22,11 @@ class SingleThreadTaskRunner;
 
 namespace pp {
 class ImageData;
-};
+}  // namespace pp
 
 namespace remoting {
 
 class ChromotingStats;
-class VideoPacket;
 
 namespace protocol {
 class SessionConfig;
@@ -54,9 +53,6 @@ class RectangleUpdateDecoder
   // Initializes decoder with the information from the protocol config.
   void Initialize(const protocol::SessionConfig& config);
 
-  // Removes all video packets in the queue.
-  void DropAllPackets();
-
   // FrameProducer implementation.  These methods may be called before we are
   // Initialize()d, or we know the source screen size.
   virtual void DrawBuffer(pp::ImageData* buffer) OVERRIDE;
@@ -68,20 +64,11 @@ class RectangleUpdateDecoder
   // VideoStub implementation.
   virtual void ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
                                   const base::Closure& done) OVERRIDE;
-  virtual int GetPendingVideoPackets() OVERRIDE;
 
   // Return the stats recorded by this client.
   ChromotingStats* GetStats();
 
  private:
-  struct QueuedVideoPacket {
-    QueuedVideoPacket(scoped_ptr<VideoPacket> packet,
-                      const base::Closure& done);
-    ~QueuedVideoPacket();
-    VideoPacket* packet;
-    base::Closure done;
-  };
-
   friend class base::RefCountedThreadSafe<RectangleUpdateDecoder>;
   virtual ~RectangleUpdateDecoder();
 
@@ -90,10 +77,6 @@ class RectangleUpdateDecoder
   void SchedulePaint();
   void DoPaint();
 
-  // If a packet is not being processed, dispatches a single message from the
-  // |received_packets_| queue.
-  void ProcessNextPacket();
-
   // Decodes the contents of |packet|. DecodePacket may keep a reference to
   // |packet| so the |packet| must remain alive and valid until |done| is
   // executed.
@@ -102,7 +85,9 @@ class RectangleUpdateDecoder
   // Callback method when a VideoPacket is processed.
   // If |last_packet| is true then |decode_start| contains the timestamp when
   // the packet will start to be processed.
-  void OnPacketDone(bool last_packet, base::Time decode_start);
+  void OnPacketDone(bool last_packet,
+                    base::Time decode_start,
+                    const base::Closure& done);
 
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner_;
@@ -125,17 +110,6 @@ class RectangleUpdateDecoder
   // Flag used to coalesce runs of SchedulePaint()s into a single DoPaint().
   bool paint_scheduled_;
 
-  // Contains all video packets that have been received, but have not yet been
-  // processed.
-  //
-  // Used to serialize sending of messages to the client.
-  // TODO(sergeyu): Simplify this code and remove this list.
-  std::list<QueuedVideoPacket> received_packets_;
-
-  // True if a message is being processed. Can be used to determine if it is
-  // safe to dispatch another message.
-  bool packet_being_processed_;
-
   ChromotingStats stats_;
 
   // Keep track of the most recent sequence number bounced back from the host.
diff --git a/remoting/protocol/protobuf_video_writer.cc b/remoting/protocol/protobuf_video_writer.cc
index e1318c0..08e937b 100644
--- a/remoting/protocol/protobuf_video_writer.cc
+++ b/remoting/protocol/protobuf_video_writer.cc
@@ -66,9 +66,5 @@ void ProtobufVideoWriter::ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
   buffered_writer_.Write(SerializeAndFrameMessage(*packet), done);
 }
 
-int ProtobufVideoWriter::GetPendingVideoPackets() {
-  return buffered_writer_.GetBufferChunks();
-}
-
 }  // namespace protocol
 }  // namespace remoting
diff --git a/remoting/protocol/protobuf_video_writer.h b/remoting/protocol/protobuf_video_writer.h
index 9c373170..961e821 100644
--- a/remoting/protocol/protobuf_video_writer.h
+++ b/remoting/protocol/protobuf_video_writer.h
@@ -37,7 +37,6 @@ class ProtobufVideoWriter : public VideoWriter {
   // VideoStub interface.
   virtual void ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
                                   const base::Closure& done) OVERRIDE;
-  virtual int GetPendingVideoPackets() OVERRIDE;
 
  private:
   void OnChannelReady(scoped_ptr<net::StreamSocket> socket);
diff --git a/remoting/protocol/protocol_mock_objects.h b/remoting/protocol/protocol_mock_objects.h
index 9e64bc2..5251287 100644
--- a/remoting/protocol/protocol_mock_objects.h
+++ b/remoting/protocol/protocol_mock_objects.h
@@ -139,8 +139,6 @@ class MockVideoStub : public VideoStub {
     ProcessVideoPacketPtr(video_packet.get(), done);
   }
 
-  MOCK_METHOD0(GetPendingVideoPackets, int());
-
  private:
   DISALLOW_COPY_AND_ASSIGN(MockVideoStub);
 };
diff --git a/remoting/protocol/video_stub.h b/remoting/protocol/video_stub.h
index 20ab8ed..461e6d7 100644
--- a/remoting/protocol/video_stub.h
+++ b/remoting/protocol/video_stub.h
@@ -16,19 +16,13 @@ namespace protocol {
 
 class VideoStub {
  public:
-  virtual ~VideoStub() { }
+  virtual ~VideoStub() {}
 
-  // TODO(sergeyu): VideoPacket is the protobuf message that is used to send
-  // video packets in protobuf stream. It should not be used here. Add another
-  // struct and use it to represent video packets internally.
   virtual void ProcessVideoPacket(scoped_ptr<VideoPacket> video_packet,
                                   const base::Closure& done) = 0;
 
-  // Returns number of packets currently pending in the buffer.
-  virtual int GetPendingVideoPackets() = 0;
-
  protected:
-  VideoStub() { }
+  VideoStub() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoStub);