Simplify VideoEncoder interface.

remoting/codec/video_encoder_verbatim.cc

Index: remoting/codec/video_encoder_verbatim.cc
diff --git a/remoting/codec/video_encoder_verbatim.cc b/remoting/codec/video_encoder_verbatim.cc
index 4680362a02d20eb75094e2ab7b91ca90d3f6a2a5..656192c9d1fea0eb93ff64f43a8becb093fd97d0 100644
--- a/remoting/codec/video_encoder_verbatim.cc
+++ b/remoting/codec/video_encoder_verbatim.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/time/time.h"
 #include "remoting/base/util.h"
 #include "remoting/proto/video.pb.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
@@ -14,119 +15,73 @@ namespace remoting {
 
 static const int kPacketSize = 1024 * 1024;
 
-VideoEncoderVerbatim::VideoEncoderVerbatim()
-    : max_packet_size_(kPacketSize) {
-}
+VideoEncoderVerbatim::VideoEncoderVerbatim() {}
+VideoEncoderVerbatim::~VideoEncoderVerbatim() {}
 
-void VideoEncoderVerbatim::SetMaxPacketSize(int size) {
-  max_packet_size_ = size;
-}
+scoped_ptr<VideoPacket> VideoEncoderVerbatim::Encode(
+    const webrtc::DesktopFrame& frame) {
+  CHECK(frame.data());
 
-VideoEncoderVerbatim::~VideoEncoderVerbatim() {
-}
+  base::Time encode_start_time = base::Time::Now();
+  scoped_ptr<VideoPacket> packet(new VideoPacket());
 
-void VideoEncoderVerbatim::Encode(
-    const webrtc::DesktopFrame* frame,
-    const DataAvailableCallback& data_available_callback) {
-  callback_ = data_available_callback;
-  encode_start_time_ = base::Time::Now();
+  VideoPacketFormat* format = packet->mutable_format();
+  format->set_encoding(VideoPacketFormat::ENCODING_VERBATIM);
+  if (!frame.size().equals(screen_size_)) {
+    screen_size_ = frame.size();
+    format->set_screen_width(screen_size_.width());
+    format->set_screen_height(screen_size_.height());
+  }
 
-  webrtc::DesktopRegion::Iterator iter(frame->updated_region());
-  while (!iter.IsAtEnd()) {
+  // Calculate output size.
+  size_t output_size = 0;
+  for (webrtc::DesktopRegion::Iterator iter(frame.updated_region());
+       !iter.IsAtEnd(); iter.Advance()) {
     const webrtc::DesktopRect& rect = iter.rect();
-    iter.Advance();
-    EncodeRect(frame, rect, iter.IsAtEnd());
+    output_size += rect.width() * rect.height() *
+        webrtc::DesktopFrame::kBytesPerPixel;
   }
 
-  callback_.Reset();
-}
-
-void VideoEncoderVerbatim::EncodeRect(const webrtc::DesktopFrame* frame,
-                                      const webrtc::DesktopRect& rect,
-                                      bool last) {
-  CHECK(frame->data());
-  const int stride = frame->stride();
-  const int bytes_per_pixel = 4;
-  const int row_size = bytes_per_pixel * rect.width();
+  uint8_t* out = GetOutputBuffer(packet.get(), output_size);
+  const int stride = frame.stride();

[Wez, 2013/09/12 14:20:15]

nit: in_stride?

[Sergey Ulanov, 2013/09/12 19:18:45]

Done.

 
-  scoped_ptr<VideoPacket> packet(new VideoPacket());
-  PrepareUpdateStart(frame, rect, packet.get());
-  const uint8* in = frame->data() +
-      rect.top() * stride + rect.left() * bytes_per_pixel;
-  // TODO(hclam): Fill in the sequence number.
-  uint8* out = GetOutputBuffer(packet.get(), max_packet_size_);
-  int filled = 0;
-  int row_pos = 0;  // Position in the current row in bytes.
-  int row_y = 0;  // Current row.
-  while (row_y < rect.height()) {
-    // Prepare a message for sending out.
-    if (!packet.get()) {
-      packet.reset(new VideoPacket());
-      out = GetOutputBuffer(packet.get(), max_packet_size_);
-      filled = 0;
-    }
-
-    if (row_y < rect.height()) {
-      int bytes_to_copy =
-          std::min(row_size - row_pos, max_packet_size_ - filled);
-      memcpy(out + filled, in + row_pos, bytes_to_copy);
-      row_pos += bytes_to_copy;
-      filled += bytes_to_copy;
-
-      // Jump to the next row when we've reached the end of the current row.
-      if (row_pos == row_size) {
-        row_pos = 0;
-        in += stride;
-        ++row_y;
-      }
-    }
-
-    if (row_y == rect.height()) {
-      DCHECK_EQ(row_pos, 0);
-
-      packet->mutable_data()->resize(filled);
-      packet->set_flags(packet->flags() | VideoPacket::LAST_PACKET);
-
-      packet->set_capture_time_ms(frame->capture_time_ms());
-      packet->set_encode_time_ms(
-          (base::Time::Now() - encode_start_time_).InMillisecondsRoundedUp());
-      if (!frame->dpi().is_zero()) {
-        packet->mutable_format()->set_x_dpi(frame->dpi().x());
-        packet->mutable_format()->set_y_dpi(frame->dpi().y());
-      }
-      if (last)
-        packet->set_flags(packet->flags() | VideoPacket::LAST_PARTITION);
+  // Store all changed rectangles in the packet.
+  for (webrtc::DesktopRegion::Iterator iter(frame.updated_region());
+       !iter.IsAtEnd(); iter.Advance()) {
+    const webrtc::DesktopRect& rect = iter.rect();
+    const int row_size = webrtc::DesktopFrame::kBytesPerPixel * rect.width();

[Wez, 2013/09/12 14:20:15]

nit: row_bytes or out_stride?

[Sergey Ulanov, 2013/09/12 19:18:45]

there is no out_stride, because it's not a single image - instead it's a
sequence of rows of updated rectangles. This is is size of a single row for in
the current rectangle.

[Wez, 2013/09/13 11:12:21]

Strictly no, but while processing a given rect |out| has a particular stride, so
I think the name would be reasonable.

+    const uint8_t* in = frame.data() + rect.top() * stride +
+                        rect.left() * webrtc::DesktopFrame::kBytesPerPixel;
+    for (int y = rect.top(); y < rect.top() + rect.height(); ++y) {
+      memcpy(out, in, row_size);
+      out += row_size;
+      in += stride;
     }
 
-    // If we have filled the current packet, then send it.
-    if (filled == max_packet_size_ || row_y == rect.height()) {
-      packet->mutable_data()->resize(filled);
-      callback_.Run(packet.Pass());
-    }
+    Rect* dirty_rect = packet->add_dirty_rects();
+    dirty_rect->set_x(rect.left());
+    dirty_rect->set_y(rect.top());
+    dirty_rect->set_width(rect.width());
+    dirty_rect->set_height(rect.height());
   }
-}
 
-void VideoEncoderVerbatim::PrepareUpdateStart(const webrtc::DesktopFrame* frame,
-                                              const webrtc::DesktopRect& rect,
-                                              VideoPacket* packet) {
-  packet->set_flags(packet->flags() | VideoPacket::FIRST_PACKET);
-
-  VideoPacketFormat* format = packet->mutable_format();
-  format->set_x(rect.left());
-  format->set_y(rect.top());
-  format->set_width(rect.width());
-  format->set_height(rect.height());
-  format->set_encoding(VideoPacketFormat::ENCODING_VERBATIM);
-  if (frame->size().equals(screen_size_)) {
-    screen_size_ = frame->size();
-    format->set_screen_width(screen_size_.width());
-    format->set_screen_height(screen_size_.height());
+  packet->set_flags(VideoPacket::FIRST_PACKET | VideoPacket::LAST_PACKET);
+  packet->set_capture_time_ms(frame.capture_time_ms());
+  packet->set_encode_time_ms(
+      (base::Time::Now() - encode_start_time).InMillisecondsRoundedUp());
+  if (!frame.dpi().is_zero()) {
+    packet->mutable_format()->set_x_dpi(frame.dpi().x());
+    packet->mutable_format()->set_y_dpi(frame.dpi().y());
   }
+  packet->set_flags(packet->flags() | VideoPacket::LAST_PARTITION);

[Wez, 2013/09/12 14:20:15]

nit: Merge this into the earlier set_flags() call.

[Sergey Ulanov, 2013/09/12 19:18:45]

Done.

+
+  return packet.Pass();
 }
 
-uint8* VideoEncoderVerbatim::GetOutputBuffer(VideoPacket* packet, size_t size) {
+uint8_t* VideoEncoderVerbatim::GetOutputBuffer(VideoPacket* packet,
+                                               size_t size) {
   packet->mutable_data()->resize(size);
-  return reinterpret_cast<uint8*>(string_as_array(packet->mutable_data()));
+  return reinterpret_cast<uint8_t*>(string_as_array(packet->mutable_data()));
 }
 
 }  // namespace remoting