Changing UpdateStreamPacket protobuf definition for chromoting

This code also changes the API for encoder and ClientConnection to eliminate
one less copy.

Review URL: http://codereview.chromium.org/2963003

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

6 files changed, 141 insertions, 36 deletions

diff --git a/remoting/client/decoder.h b/remoting/client/decoder.h
index ac2fe4d..4d083bd 100644
--- a/remoting/client/decoder.h
+++ b/remoting/client/decoder.h
@@ -57,9 +57,20 @@ class Decoder {
   // Give a HostMessage that contains the update stream packet that contains
   // the encoded data to the decoder.
   // The decoder will own |message| and is responsible for deleting it.
+  //
   // If the decoder has written something into |frame|,
   // |partial_decode_done_| is called with |frame| and updated regions.
   // Return true if the decoder can accept |message| and decode it.
+  //
+  // HostMessage returned by this method will contain a
+  // UpdateStreamPacketMessage.
+  // This message will contain either:
+  // 1. UpdateStreamBeginRect
+  // 2. UpdateStreamRectData
+  // 3. UpdateStreamEndRect
+  //
+  // See remoting/base/protocol/chromotocol.proto for more information about
+  // these messages.
   virtual bool PartialDecode(HostMessage* message) = 0;
 
   // Notify the decoder that we have received the last update stream packet.
@@ -68,6 +79,34 @@ class Decoder {
   // If the update stream is not received fully and this method is called the
   // decoder should also call |decode_done_| as soon as possible.
   virtual void EndDecode() = 0;
+
+ protected:
+  // Every decoder will have two internal states because there are three
+  // kinds of messages send to PartialDecode().
+  //
+  // Here's a state diagram:
+  //
+  //                UpdateStreamBeginRect       UpdateStreamRectData
+  //                    ..............              ............
+  //                   .              .            .            .
+  //                  .                v          .              .
+  // kWaitingForBeginRect         kWaitingForRectData            .
+  //                  ^                .          ^              .
+  //                   .              .            .            .
+  //                    ..............              ............
+  //                    UpdateStreaEndRect
+  enum State {
+    // In this state the decoder is waiting for UpdateStreamBeginRect.
+    // After receiving UpdateStreaBeginRect, the encoder will transit to
+    // to kWaitingForRectData state.
+    kWaitingForBeginRect,
+
+    // In this state the decoder is waiting for UpdateStreamRectData.
+    // The decode remains in this state if UpdateStreamRectData is received.
+    // The decoder will transit to kWaitingForBeginRect if UpdateStreamEndRect
+    // is received.
+    kWaitingForRectData,
+  };
 };
 
 }  // namespace remoting
diff --git a/remoting/client/decoder_verbatim.cc b/remoting/client/decoder_verbatim.cc
index e9b88ef..d27d265 100644
--- a/remoting/client/decoder_verbatim.cc
+++ b/remoting/client/decoder_verbatim.cc
@@ -9,7 +9,13 @@
 namespace remoting {
 
 DecoderVerbatim::DecoderVerbatim()
-    : updated_rects_(NULL),
+    : state_(kWaitingForBeginRect),
+      rect_x_(0),
+      rect_y_(0),
+      rect_width_(0),
+      rect_height_(0),
+      bytes_per_pixel_(0),
+      updated_rects_(NULL),
       reverse_rows_(true) {
 }
 
@@ -20,6 +26,7 @@ bool DecoderVerbatim::BeginDecode(scoped_refptr<media::VideoFrame> frame,
   DCHECK(!partial_decode_done_.get());
   DCHECK(!decode_done_.get());
   DCHECK(!updated_rects_);
+  DCHECK_EQ(kWaitingForBeginRect, state_);
 
   partial_decode_done_.reset(partial_decode_done);
   decode_done_.reset(decode_done);
@@ -33,54 +40,88 @@ bool DecoderVerbatim::BeginDecode(scoped_refptr<media::VideoFrame> frame,
 
 bool DecoderVerbatim::PartialDecode(HostMessage* message) {
   scoped_ptr<HostMessage> msg_deleter(message);
+  DCHECK(message->has_update_stream_packet());
+
+  bool ret = true;
+  if (message->update_stream_packet().has_begin_rect())
+    ret = HandleBeginRect(message);
+  if (ret && message->update_stream_packet().has_rect_data())
+    ret = HandleRectData(message);
+  if (ret && message->update_stream_packet().has_end_rect())
+    ret = HandleEndRect(message);
+  return ret;
+}
+
+void DecoderVerbatim::EndDecode() {
+  DCHECK_EQ(kWaitingForBeginRect, state_);
+  decode_done_->Run();
+
+  partial_decode_done_.reset();
+  decode_done_.reset();
+  frame_ = NULL;
+  updated_rects_ = NULL;
+}
+
+bool DecoderVerbatim::HandleBeginRect(HostMessage* message) {
+  DCHECK_EQ(kWaitingForBeginRect, state_);
+  state_ = kWaitingForRectData;
+
+  rect_width_ = message->update_stream_packet().begin_rect().width();
+  rect_height_ = message->update_stream_packet().begin_rect().height();
+  rect_x_ = message->update_stream_packet().begin_rect().x();
+  rect_y_ = message->update_stream_packet().begin_rect().y();
 
-  int width = message->update_stream_packet().header().width();
-  int height = message->update_stream_packet().header().height();
-  int x = message->update_stream_packet().header().x();
-  int y = message->update_stream_packet().header().y();
   PixelFormat pixel_format =
-      message->update_stream_packet().header().pixel_format();
+      message->update_stream_packet().begin_rect().pixel_format();
 
   if (static_cast<PixelFormat>(frame_->format()) != pixel_format) {
     NOTREACHED() << "Pixel format of message doesn't match the video frame. "
                     "Expected vs received = "
                  << frame_->format() << " vs " << pixel_format
                  << " Color space conversion required.";
+    return false;
   }
 
-  int bytes_per_pixel = GetBytesPerPixel(pixel_format);
+  bytes_per_pixel_ = GetBytesPerPixel(pixel_format);
+  return true;
+}
+
+bool DecoderVerbatim::HandleRectData(HostMessage* message) {
+  DCHECK_EQ(kWaitingForRectData, state_);
+  DCHECK_EQ(0,
+            message->update_stream_packet().rect_data().sequence_number());
+
   // Copy the data line by line.
-  const int src_stride = bytes_per_pixel * width;
-  const char* src = message->update_stream_packet().data().c_str();
+  const int src_stride = bytes_per_pixel_ * rect_width_;
+  const char* src =
+      message->update_stream_packet().rect_data().data().c_str();
   int src_stride_dir = src_stride;
   if (reverse_rows_) {
     // Copy rows from bottom to top to flip the image vertically.
-    src += (height - 1) * src_stride;
+    src += (rect_height_ - 1) * src_stride;
     // Change the direction of the stride to work bottom to top.
     src_stride_dir *= -1;
   }
   const int dest_stride = frame_->stride(media::VideoFrame::kRGBPlane);
   uint8* dest = frame_->data(media::VideoFrame::kRGBPlane) +
-      dest_stride * y + bytes_per_pixel * x;
-  for (int i = 0; i < height; ++i) {
+      dest_stride * rect_y_ + bytes_per_pixel_ * rect_x_;
+  for (int i = 0; i < rect_height_; ++i) {
     memcpy(dest, src, src_stride);
     dest += dest_stride;
     src += src_stride_dir;
   }
 
   updated_rects_->clear();
-  updated_rects_->push_back(gfx::Rect(x, y, width, height));
+  updated_rects_->push_back(gfx::Rect(rect_x_, rect_y_,
+                                      rect_width_, rect_height_));
   partial_decode_done_->Run();
   return true;
 }
 
-void DecoderVerbatim::EndDecode() {
-  decode_done_->Run();
-
-  partial_decode_done_.reset();
-  decode_done_.reset();
-  frame_ = NULL;
-  updated_rects_ = NULL;
+bool DecoderVerbatim::HandleEndRect(HostMessage* message) {
+  DCHECK_EQ(kWaitingForRectData, state_);
+  state_ = kWaitingForBeginRect;
+  return true;
 }
 
 }  // namespace remoting
diff --git a/remoting/client/decoder_verbatim.h b/remoting/client/decoder_verbatim.h
index 85b49c6..6efc732 100644
--- a/remoting/client/decoder_verbatim.h
+++ b/remoting/client/decoder_verbatim.h
@@ -22,6 +22,20 @@ class DecoderVerbatim : public Decoder {
   virtual void EndDecode();
 
  private:
+  bool HandleBeginRect(HostMessage* message);
+  bool HandleRectData(HostMessage* message);
+  bool HandleEndRect(HostMessage* message);
+
+  // The internal state of the decoder.
+  State state_;
+
+  // Keeps track of the updating rect.
+  int rect_x_;
+  int rect_y_;
+  int rect_width_;
+  int rect_height_;
+  int bytes_per_pixel_;
+
   // Tasks to call when decode is done.
   scoped_ptr<Task> partial_decode_done_;
   scoped_ptr<Task> decode_done_;
diff --git a/remoting/client/decoder_verbatim_unittest.cc b/remoting/client/decoder_verbatim_unittest.cc
index ca2c6f7..d541d22 100644
--- a/remoting/client/decoder_verbatim_unittest.cc
+++ b/remoting/client/decoder_verbatim_unittest.cc
@@ -20,13 +20,22 @@ TEST(DecoderVerbatimTest, SimpleDecode) {
   const size_t kHeight = 1;
   const char kData[] = "ABCDEFGHIJ";
   scoped_ptr<HostMessage> msg(new HostMessage());
-  msg->mutable_update_stream_packet()->mutable_header()->set_width(kWidth);
-  msg->mutable_update_stream_packet()->mutable_header()->set_height(kHeight);
-  msg->mutable_update_stream_packet()->mutable_header()->set_x(0);
-  msg->mutable_update_stream_packet()->mutable_header()->set_y(0);
-  msg->mutable_update_stream_packet()->mutable_header()->set_pixel_format(
-      PixelFormatAscii);
-  msg->mutable_update_stream_packet()->set_data(kData, sizeof(kData));
+
+  // Prepare the begin rect message.
+  UpdateStreamBeginRect* begin_rect =
+      msg->mutable_update_stream_packet()->mutable_begin_rect();
+  begin_rect->set_width(kWidth);
+  begin_rect->set_height(kHeight);
+  begin_rect->set_x(0);
+  begin_rect->set_y(0);
+  begin_rect->set_pixel_format(PixelFormatAscii);
+
+  // Prepare the rect data.
+  msg->mutable_update_stream_packet()->mutable_rect_data()->set_data(
+      kData, sizeof(kData));
+
+  // Prepare the end rect.
+  msg->mutable_update_stream_packet()->mutable_end_rect();
 
   scoped_refptr<media::VideoFrame> frame;
   media::VideoFrame::CreateFrame(media::VideoFrame::ASCII, kWidth, kHeight,
diff --git a/remoting/client/simple_client.cc b/remoting/client/simple_client.cc
index 98074bf..d1dd6e2 100644
--- a/remoting/client/simple_client.cc
+++ b/remoting/client/simple_client.cc
@@ -86,9 +86,13 @@ class SimpleHostEventCallback : public HostConnection::HostEventCallback {
 
   void HandleUpdateStreamPacketMessage(HostMessage* host_msg) {
     const UpdateStreamPacketMessage& msg = host_msg->update_stream_packet();
-    std::cout << "UpdateStreamPacket (" << msg.header().x()
-              << ", " << msg.header().y() << ") ["
-              << msg.header().width() << " x " << msg.header().height() << "]"
+    if (!msg.has_begin_rect())
+      return;
+
+    std::cout << "UpdateStreamPacket (" << msg.begin_rect().x()
+              << ", " << msg.begin_rect().y() << ") ["
+              << msg.begin_rect().width() << " x "
+              << msg.begin_rect().height() << "]"
               << std::endl;
   }
 
diff --git a/remoting/client/x11_view.cc b/remoting/client/x11_view.cc
index d380cd2..c5f9a88 100644
--- a/remoting/client/x11_view.cc
+++ b/remoting/client/x11_view.cc
@@ -20,6 +20,9 @@ X11View::X11View(Display* display, XID window, int width, int height)
       width_(width),
       height_(height),
       picture_(0) {
+  media::VideoFrame::CreateFrame(media::VideoFrame::RGB32, width_, height_,
+                                 base::TimeDelta(), base::TimeDelta(), &frame_);
+  DCHECK(frame_);
 }
 
 X11View::~X11View() {
@@ -30,7 +33,7 @@ void X11View::Paint() {
   all_update_rects_.clear();
 
   // If we have not initialized the render target then do it now.
-  if (!frame_)
+  if (!picture_)
     InitPaintTarget();
 
   // Upload the image to a pixmap. And then create a picture from the pixmap
@@ -112,11 +115,6 @@ void X11View::InitPaintTarget() {
 
   picture_ = XRenderCreatePicture(display_, window_, pictformat, 0, NULL);
   CHECK(picture_) << "Backing picture not created";
-
-  // Create the video frame to carry the decoded image.
-  media::VideoFrame::CreateFrame(media::VideoFrame::RGB32, width_, height_,
-                                 base::TimeDelta(), base::TimeDelta(), &frame_);
-  DCHECK(frame_);
 }
 
 void X11View::HandleBeginUpdateStream(HostMessage* msg) {