Move common VideoPacket initialization into VideoEncoderHelper.

This avoids duplication of common VideoPacket initialization across
multiple encoders, making addition of new fields (e.g. the recently
added shape field) more robust.

Review URL: https://codereview.chromium.org/530243002

Cr-Commit-Position: refs/heads/master@{#293064}

10 files changed, 251 insertions, 52 deletions

diff --git a/remoting/codec/video_encoder_helper.cc b/remoting/codec/video_encoder_helper.cc
new file mode 100644
index 0000000..5310d01
--- /dev/null
+++ b/remoting/codec/video_encoder_helper.cc
@@ -0,0 +1,70 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/codec/video_encoder_helper.h"
+
+#include "remoting/proto/video.pb.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
+
+namespace remoting {
+
+VideoEncoderHelper::VideoEncoderHelper() {}
+
+scoped_ptr<VideoPacket> VideoEncoderHelper::CreateVideoPacket(
+    const webrtc::DesktopFrame& frame) {
+  return CreateVideoPacketWithUpdatedRegion(frame, frame.updated_region());
+}
+
+scoped_ptr<VideoPacket>
+VideoEncoderHelper::CreateVideoPacketWithUpdatedRegion(
+    const webrtc::DesktopFrame& frame,
+    const webrtc::DesktopRegion& updated_region) {
+  scoped_ptr<VideoPacket> packet(new VideoPacket());
+
+  // Set |screen_width| and |screen_height| iff they have changed.
+  if (!frame.size().equals(screen_size_)) {
+    screen_size_ = frame.size();
+
+    VideoPacketFormat* format = packet->mutable_format();
+    format->set_screen_width(screen_size_.width());
+    format->set_screen_height(screen_size_.height());
+  }
+
+  // Record the list of changed rectangles.
+  for (webrtc::DesktopRegion::Iterator iter(updated_region);
+       !iter.IsAtEnd(); iter.Advance()) {
+    const webrtc::DesktopRect& rect = iter.rect();
+    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());
+  }
+
+  // Record the shape of the frame, if specified.
+  if (frame.shape()) {
+    packet->set_use_desktop_shape(true);
+    for (webrtc::DesktopRegion::Iterator r(*frame.shape());
+         !r.IsAtEnd(); r.Advance()) {
+      Rect* rect = packet->add_desktop_shape_rects();
+      rect->set_x(r.rect().left());
+      rect->set_y(r.rect().top());
+      rect->set_width(r.rect().width());
+      rect->set_height(r.rect().height());
+    }
+  }
+
+  // Store the capture time and frame DPI.
+  packet->set_capture_time_ms(frame.capture_time_ms());
+  if (!frame.dpi().is_zero()) {
+    packet->mutable_format()->set_x_dpi(frame.dpi().x());
+    packet->mutable_format()->set_y_dpi(frame.dpi().y());
+  }
+
+  return packet.Pass();
+}
+
+}  // namespace remoting
diff --git a/remoting/codec/video_encoder_helper.h b/remoting/codec/video_encoder_helper.h
new file mode 100644
index 0000000..c33fdb9
--- /dev/null
+++ b/remoting/codec/video_encoder_helper.h
@@ -0,0 +1,46 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_CODEC_VIDEO_ENCODER_HELPER_H_
+#define REMOTING_CODEC_VIDEO_ENCODER_HELPER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
+
+namespace webrtc {
+class DesktopFrame;
+class DesktopRegion;
+}
+
+namespace remoting {
+
+class VideoPacket;
+
+class VideoEncoderHelper {
+ public:
+  VideoEncoderHelper();
+
+  // Returns a new VideoPacket with common fields (e.g. capture_time_ms, rects
+  // list, frame shape if any) initialized based on the supplied |frame|.
+  // Screen width and height will be set iff |frame|'s size differs from that
+  // of the previously-supplied frame.
+  scoped_ptr<VideoPacket> CreateVideoPacket(const webrtc::DesktopFrame& frame);
+
+  // Returns a new VideoPacket with the common fields populated from |frame|,
+  // but the updated rects overridden by |updated_region|. This is useful for
+  // encoders which alter the updated region e.g. by expanding it to macroblock
+  // boundaries.
+  scoped_ptr<VideoPacket> CreateVideoPacketWithUpdatedRegion(
+      const webrtc::DesktopFrame& frame,
+      const webrtc::DesktopRegion& updated_region);
+ private:
+  // The most recent screen size. Used to detect screen size changes.
+  webrtc::DesktopSize screen_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoEncoderHelper);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_CODEC_VIDEO_ENCODER_HELPER_H_
diff --git a/remoting/codec/video_encoder_helper_unittest.cc b/remoting/codec/video_encoder_helper_unittest.cc
new file mode 100644
index 0000000..e464802
--- /dev/null
+++ b/remoting/codec/video_encoder_helper_unittest.cc
@@ -0,0 +1,107 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/codec/video_encoder_helper.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "remoting/proto/video.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+
+using webrtc::BasicDesktopFrame;
+using webrtc::DesktopRect;
+using webrtc::DesktopRegion;
+using webrtc::DesktopSize;
+using webrtc::DesktopVector;
+
+namespace remoting {
+
+TEST(VideoEncoderHelperTest, PropagatesCommonFields) {
+  BasicDesktopFrame frame(DesktopSize(32, 32));
+  frame.set_dpi(DesktopVector(96, 97));
+  frame.set_capture_time_ms(20);
+  frame.mutable_updated_region()->SetRect(DesktopRect::MakeLTRB(0, 0, 16, 16));
+  scoped_ptr<DesktopRegion> shape(
+      new DesktopRegion(DesktopRect::MakeLTRB(16, 0, 32, 16)));
+  frame.set_shape(shape.release());
+
+  VideoEncoderHelper helper;
+  scoped_ptr<VideoPacket> packet(helper.CreateVideoPacket(frame));
+
+  ASSERT_TRUE(packet->has_format());
+  EXPECT_FALSE(packet->format().has_encoding());
+  EXPECT_TRUE(packet->format().has_screen_width());
+  EXPECT_TRUE(packet->format().has_screen_height());
+  EXPECT_TRUE(packet->format().has_x_dpi());
+  EXPECT_TRUE(packet->format().has_y_dpi());
+
+  EXPECT_TRUE(packet->has_capture_time_ms());
+  EXPECT_EQ(1, packet->dirty_rects().size());
+
+  ASSERT_TRUE(packet->has_use_desktop_shape());
+  EXPECT_TRUE(packet->use_desktop_shape());
+
+  EXPECT_EQ(1, packet->desktop_shape_rects().size());
+}
+
+TEST(VideoEncoderHelperTest, ZeroDpi) {
+  BasicDesktopFrame frame(DesktopSize(32, 32));
+  // DPI is zero unless explicitly set.
+
+  VideoEncoderHelper helper;
+  scoped_ptr<VideoPacket> packet(helper.CreateVideoPacket(frame));
+
+  // Packet should have a format containing the screen dimensions only.
+  ASSERT_TRUE(packet->has_format());
+  EXPECT_TRUE(packet->format().has_screen_width());
+  EXPECT_TRUE(packet->format().has_screen_height());
+  EXPECT_FALSE(packet->format().has_x_dpi());
+  EXPECT_FALSE(packet->format().has_y_dpi());
+}
+
+TEST(VideoEncoderHelperTest, NoShape) {
+  BasicDesktopFrame frame(DesktopSize(32, 32));
+
+  VideoEncoderHelper helper;
+  scoped_ptr<VideoPacket> packet(helper.CreateVideoPacket(frame));
+
+  EXPECT_FALSE(packet->use_desktop_shape());
+  EXPECT_EQ(0, packet->desktop_shape_rects().size());
+}
+
+TEST(VideoEncoderHelperTest, NoScreenSizeIfUnchanged) {
+  BasicDesktopFrame frame(DesktopSize(32, 32));
+  // Set DPI so that the packet will have a format, with DPI but no size.
+  frame.set_dpi(DesktopVector(96, 97));
+
+  VideoEncoderHelper helper;
+  scoped_ptr<VideoPacket> packet(helper.CreateVideoPacket(frame));
+  packet = helper.CreateVideoPacket(frame);
+
+  ASSERT_TRUE(packet->has_format());
+  EXPECT_FALSE(packet->format().has_screen_width());
+  EXPECT_FALSE(packet->format().has_screen_height());
+  EXPECT_TRUE(packet->format().has_x_dpi());
+  EXPECT_TRUE(packet->format().has_y_dpi());
+}
+
+TEST(VideoEncoderHelperTest, ScreenSizeWhenChanged) {
+  VideoEncoderHelper helper;
+
+  // Process the same frame twice, so the helper knows the current size, and
+  // to trigger suppression of the size field due to the size not changing.
+  BasicDesktopFrame frame1(DesktopSize(32, 32));
+  scoped_ptr<VideoPacket> packet(helper.CreateVideoPacket(frame1));
+  packet = helper.CreateVideoPacket(frame1);
+
+  // Process a different-sized frame to trigger size to be emitted.
+  BasicDesktopFrame frame2(DesktopSize(48, 48));
+  packet = helper.CreateVideoPacket(frame2);
+
+  ASSERT_TRUE(packet->has_format());
+  EXPECT_TRUE(packet->format().has_screen_width());
+  EXPECT_TRUE(packet->format().has_screen_height());
+}
+
+}  // namespace remoting
diff --git a/remoting/codec/video_encoder_verbatim.cc b/remoting/codec/video_encoder_verbatim.cc
index 0133b3f..c7550e1 100644
--- a/remoting/codec/video_encoder_verbatim.cc
+++ b/remoting/codec/video_encoder_verbatim.cc
@@ -10,9 +10,16 @@
 #include "remoting/base/util.h"
 #include "remoting/proto/video.pb.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
 
 namespace remoting {
 
+static uint8_t* GetPacketOutputBuffer(VideoPacket* packet, size_t size) {
+  packet->mutable_data()->resize(size);
+  return reinterpret_cast<uint8_t*>(string_as_array(packet->mutable_data()));
+}
+
 VideoEncoderVerbatim::VideoEncoderVerbatim() {}
 VideoEncoderVerbatim::~VideoEncoderVerbatim() {}
 
@@ -21,15 +28,10 @@ scoped_ptr<VideoPacket> VideoEncoderVerbatim::Encode(
   CHECK(frame.data());
 
   base::Time encode_start_time = base::Time::Now();
-  scoped_ptr<VideoPacket> packet(new VideoPacket());
 
-  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());
-  }
+  // Create a VideoPacket with common fields (e.g. DPI, rects, shape) set.
+  scoped_ptr<VideoPacket> packet(helper_.CreateVideoPacket(frame));
+  packet->mutable_format()->set_encoding(VideoPacketFormat::ENCODING_VERBATIM);
 
   // Calculate output size.
   size_t output_size = 0;
@@ -40,10 +42,10 @@ scoped_ptr<VideoPacket> VideoEncoderVerbatim::Encode(
         webrtc::DesktopFrame::kBytesPerPixel;
   }
 
-  uint8_t* out = GetOutputBuffer(packet.get(), output_size);
+  uint8_t* out = GetPacketOutputBuffer(packet.get(), output_size);
   const int in_stride = frame.stride();
 
-  // Store all changed rectangles in the packet.
+  // Encode pixel data for all changed rectangles into the packet.
   for (webrtc::DesktopRegion::Iterator iter(frame.updated_region());
        !iter.IsAtEnd(); iter.Advance()) {
     const webrtc::DesktopRect& rect = iter.rect();
@@ -55,29 +57,13 @@ scoped_ptr<VideoPacket> VideoEncoderVerbatim::Encode(
       out += row_size;
       in += in_stride;
     }
-
-    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());
   }
 
-  packet->set_capture_time_ms(frame.capture_time_ms());
+  // Note the time taken to encode the pixel data.
   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());
-  }
 
   return packet.Pass();
 }
 
-uint8_t* VideoEncoderVerbatim::GetOutputBuffer(VideoPacket* packet,
-                                               size_t size) {
-  packet->mutable_data()->resize(size);
-  return reinterpret_cast<uint8_t*>(string_as_array(packet->mutable_data()));
-}
-
 }  // namespace remoting
diff --git a/remoting/codec/video_encoder_verbatim.h b/remoting/codec/video_encoder_verbatim.h
index 54a8a03..0005a4a 100644
--- a/remoting/codec/video_encoder_verbatim.h
+++ b/remoting/codec/video_encoder_verbatim.h
@@ -6,8 +6,7 @@
 #define REMOTING_CODEC_VIDEO_ENCODER_VERBATIM_H_
 
 #include "remoting/codec/video_encoder.h"
-#include "remoting/proto/video.pb.h"
-#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
+#include "remoting/codec/video_encoder_helper.h"
 
 namespace remoting {
 
@@ -23,12 +22,9 @@ class VideoEncoderVerbatim : public VideoEncoder {
       const webrtc::DesktopFrame& frame) OVERRIDE;
 
  private:
-  // Allocates a buffer of the specified |size| inside |packet| and returns the
-  // pointer to it.
-  uint8* GetOutputBuffer(VideoPacket* packet, size_t size);
+  VideoEncoderHelper helper_;
 
-  // The most recent screen size. Used to detect screen size changes.
-  webrtc::DesktopSize screen_size_;
+  DISALLOW_COPY_AND_ASSIGN(VideoEncoderVerbatim);
 };
 
 }  // namespace remoting
diff --git a/remoting/codec/video_encoder_vpx.cc b/remoting/codec/video_encoder_vpx.cc
index 75b6d4e..61d94c9 100644
--- a/remoting/codec/video_encoder_vpx.cc
+++ b/remoting/codec/video_encoder_vpx.cc
@@ -288,7 +288,9 @@ scoped_ptr<VideoPacket> VideoEncoderVpx::Encode(
 
   // TODO(hclam): Make sure we get exactly one frame from the packet.
   // TODO(hclam): We should provide the output buffer to avoid one copy.
-  scoped_ptr<VideoPacket> packet(new VideoPacket());
+  scoped_ptr<VideoPacket> packet(
+      helper_.CreateVideoPacketWithUpdatedRegion(frame, updated_region));
+  packet->mutable_format()->set_encoding(VideoPacketFormat::ENCODING_VP8);
 
   while (!got_data) {
     const vpx_codec_cx_pkt_t* vpx_packet =
@@ -306,25 +308,9 @@ scoped_ptr<VideoPacket> VideoEncoderVpx::Encode(
     }
   }
 
-  // Construct the VideoPacket message.
-  packet->mutable_format()->set_encoding(VideoPacketFormat::ENCODING_VP8);
-  packet->mutable_format()->set_screen_width(frame.size().width());
-  packet->mutable_format()->set_screen_height(frame.size().height());
-  packet->set_capture_time_ms(frame.capture_time_ms());
+  // Note the time taken to encode the pixel data.
   packet->set_encode_time_ms(
       (base::TimeTicks::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());
-  }
-  for (webrtc::DesktopRegion::Iterator r(updated_region); !r.IsAtEnd();
-       r.Advance()) {
-    Rect* rect = packet->add_dirty_rects();
-    rect->set_x(r.rect().left());
-    rect->set_y(r.rect().top());
-    rect->set_width(r.rect().width());
-    rect->set_height(r.rect().height());
-  }
 
   return packet.Pass();
 }
diff --git a/remoting/codec/video_encoder_vpx.h b/remoting/codec/video_encoder_vpx.h
index 133b754..80e69db0 100644
--- a/remoting/codec/video_encoder_vpx.h
+++ b/remoting/codec/video_encoder_vpx.h
@@ -9,6 +9,7 @@
 #include "base/time/time.h"
 #include "remoting/codec/scoped_vpx_codec.h"
 #include "remoting/codec/video_encoder.h"
+#include "remoting/codec/video_encoder_helper.h"
 
 typedef struct vpx_image vpx_image_t;
 
@@ -68,6 +69,9 @@ class VideoEncoderVpx : public VideoEncoder {
   int active_map_width_;
   int active_map_height_;
 
+  // Used to help initialize VideoPackets from DesktopFrames.
+  VideoEncoderHelper helper_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoEncoderVpx);
 };
 
diff --git a/remoting/host/video_frame_recorder_unittest.cc b/remoting/host/video_frame_recorder_unittest.cc
index 9e4139b..e288135 100644
--- a/remoting/host/video_frame_recorder_unittest.cc
+++ b/remoting/host/video_frame_recorder_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "remoting/codec/video_encoder_verbatim.h"
+#include "remoting/proto/video.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
diff --git a/remoting/remoting_srcs.gypi b/remoting/remoting_srcs.gypi
index f91868c..e9440ad 100644
--- a/remoting/remoting_srcs.gypi
+++ b/remoting/remoting_srcs.gypi
@@ -62,6 +62,8 @@
       'codec/video_decoder_vpx.cc',
       'codec/video_decoder_vpx.h',
       'codec/video_encoder.h',
+      'codec/video_encoder_helper.cc',
+      'codec/video_encoder_helper.h',
       'codec/video_encoder_verbatim.cc',
       'codec/video_encoder_verbatim.h',
       'codec/video_encoder_vpx.cc',
diff --git a/remoting/remoting_test.gypi b/remoting/remoting_test.gypi
index 0044e16..0b90f47 100644
--- a/remoting/remoting_test.gypi
+++ b/remoting/remoting_test.gypi
@@ -124,6 +124,7 @@
         'codec/codec_test.cc',
         'codec/codec_test.h',
         'codec/video_decoder_vpx_unittest.cc',
+        'codec/video_encoder_helper_unittest.cc',
         'codec/video_encoder_verbatim_unittest.cc',
         'codec/video_encoder_vpx_unittest.cc',
         'host/audio_silence_detector_unittest.cc',