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}

remoting/codec/video_encoder_helper.cc

+// 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

remoting/codec/video_encoder_helper.h

+// 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_

remoting/codec/video_encoder_helper_unittest.cc

+// 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

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

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

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();
 }

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);
 };
 

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"

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',

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',