Cleanup RectangleUpdateDecoder and VideoStub

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 c7a14046c39cc057ea4d29ce37484e9dac189c1f..00b9520b85544c786b57eab833030084a87a4b40 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,7 @@ RectangleUpdateDecoder::RectangleUpdateDecoder(
       view_size_(SkISize::Make(0, 0)),
       clip_area_(SkIRect::MakeEmpty()),
       paint_scheduled_(false),
-      packet_being_processed_(false),
+      stopped_(false),
       latest_sequence_number_(0) {
 }
 
@@ -66,11 +57,23 @@ void RectangleUpdateDecoder::Initialize(const SessionConfig& config) {
   }
 }
 
+void RectangleUpdateDecoder::Stop() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());

[Wez, 2012/08/28 16:42:27]

nit: Blank line after DCHECK.

[Sergey Ulanov, 2012/08/28 18:30:27]

Done.

+  // |stopped_| is used on the decode thread, but changed only here on the main
+  // thread. Don't need any synchronization here because it's not an issue
+  // if the decode thread sees false instead of true.

[Wez, 2012/08/28 16:42:27]

Why not just re-post the Stop() to set this on the decode thread, so you know
that it'll be seen as soon as that gets processed?

[Sergey Ulanov, 2012/08/28 18:30:27]

Posting a task on the decoder thread will not do what we want here. We want to
stop decoding of the frames that are already in the task queue for the decode
thread. If I post a task to the decode thread it will only be executed after all
pending decoder tasks.

[Wez, 2012/08/29 23:18:05]

OK, but you've stated that we don't need to lock here because the it's not an
issue if the thread sees false instead of true; if the optimization of stopping
processing on the other thread is valuable then it's worth putting a lock around
this to create a memory barrier and ensure that the other thread sees the
change.

[Sergey Ulanov, 2012/08/29 23:31:10]

Actually after thinking more about it I think we don't need this method at all.
Normally MessageReader class doesn't read from the socket if there are any
pending messages. The only case when it may allow several messages to be
processed concurrently is when they were read in a single Read() call - in that
case they are likely to be empty packets, so we don't win much by canceling them
anyway.
Removed this method.

+  stopped_ = true;
+}
+
 void RectangleUpdateDecoder::DecodePacket(scoped_ptr<VideoPacket> packet,
                                           const base::Closure& done) {
   DCHECK(decode_task_runner_->BelongsToCurrentThread());
 
   base::ScopedClosureRunner done_runner(done);
+
+  if (stopped_)
+    return;
+
   bool decoder_needs_reset = false;
   bool notify_size_or_dpi_change = false;
 
@@ -262,49 +265,15 @@ 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 callback =

[Wez, 2012/08/28 16:42:27]

nit: callback -> decode_done? (More consistent with |done|).

[Sergey Ulanov, 2012/08/28 18:30:27]

Done.

+      base::Bind(&RectangleUpdateDecoder::OnPacketDone, this,
+                 last_packet, decode_start, done);
 
   decode_task_runner_->PostTask(FROM_HERE, base::Bind(
       &RectangleUpdateDecoder::DecodePacket, this,
@@ -312,11 +281,12 @@ void RectangleUpdateDecoder::ProcessNextPacket() {
 }
 
 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 +297,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() {