Update VideoFramePump to pass un-changed frames to encoders.

This allows VP9/lossy mode to "top-off" previously encoded imagery to
improve visual quality at the client, by continuing to deliver data even
when nothing has actually changed on the host desktop.

This CL makes several other minor changes:
- Fix the host's --enable-i444 flag, which was broken by refactoring.
- Tweak VP9/lossy mode to more conservative quantization settings.
- Tweak VP9/lossy mode to encode up to two unchanged frames for top-off.
- Update VideoEncoderVerbatim for the new VideoFramePump semantics.
- Update the VideoFrameRecorder & FakeDesktopCapturer.
- Adds simple codec & VideoFramePump unit-tests for the new behaviour.

BUG=134202

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

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

15 files changed, 223 insertions, 47 deletions

diff --git a/remoting/codec/codec_test.cc b/remoting/codec/codec_test.cc
index 8b9511c..51b52f0 100644
--- a/remoting/codec/codec_test.cc
+++ b/remoting/codec/codec_test.cc
@@ -247,7 +247,7 @@ scoped_ptr<DesktopFrame> PrepareFrame(const DesktopSize& size) {
 
 static void TestEncodingRects(VideoEncoder* encoder,
                               VideoEncoderTester* tester,
-                              webrtc::DesktopFrame* frame,
+                              DesktopFrame* frame,
                               const DesktopRect* rects,
                               int count) {
   frame->mutable_updated_region()->Clear();
@@ -267,7 +267,7 @@ void TestVideoEncoder(VideoEncoder* encoder, bool strict) {
   for (size_t xi = 0; xi < arraysize(kSizes); ++xi) {
     for (size_t yi = 0; yi < arraysize(kSizes); ++yi) {
       DesktopSize size = DesktopSize(kSizes[xi], kSizes[yi]);
-      scoped_ptr<webrtc::DesktopFrame> frame = PrepareFrame(size);
+      scoped_ptr<DesktopFrame> frame = PrepareFrame(size);
       std::vector<std::vector<DesktopRect> > test_rect_lists =
           MakeTestRectLists(size);
       for (size_t i = 0; i < test_rect_lists.size(); ++i) {
@@ -275,10 +275,31 @@ void TestVideoEncoder(VideoEncoder* encoder, bool strict) {
         TestEncodingRects(encoder, &tester, frame.get(),
                           &test_rects[0], test_rects.size());
       }
+
+      // Pass some empty frames through the encoder.
+      for (int i = 0; i < 10; ++i) {
+        TestEncodingRects(encoder, &tester, frame.get(), nullptr, 0);
+      }
     }
   }
 }
 
+void TestVideoEncoderEmptyFrames(VideoEncoder* encoder, int topoff_frames) {
+  const DesktopSize kSize(640, 480);
+  scoped_ptr<DesktopFrame> frame(PrepareFrame(kSize));
+
+  frame->mutable_updated_region()->SetRect(
+      webrtc::DesktopRect::MakeSize(kSize));
+  EXPECT_TRUE(encoder->Encode(*frame));
+
+  frame->mutable_updated_region()->Clear();
+  for (int i=0; i < topoff_frames; ++i) {
+    EXPECT_TRUE(encoder->Encode(*frame));
+  }
+
+  EXPECT_FALSE(encoder->Encode(*frame));
+}
+
 static void TestEncodeDecodeRects(VideoEncoder* encoder,
                                   VideoEncoderTester* encoder_tester,
                                   VideoDecoderTester* decoder_tester,
diff --git a/remoting/codec/codec_test.h b/remoting/codec/codec_test.h
index 9c9ec7b..0c143b7 100644
--- a/remoting/codec/codec_test.h
+++ b/remoting/codec/codec_test.h
@@ -26,6 +26,12 @@ class VideoEncoder;
 // rects match dirty rects.
 void TestVideoEncoder(VideoEncoder* encoder, bool strict);
 
+// Generate test data and test the encoder for a sequence of one "changed"
+// frame followed by one or more "unchanged" frames, and verify that the
+// encoder sends exactly |topoff_frames| of non-empty data for unchanged
+// frames, after which it returns null frames.
+void TestVideoEncoderEmptyFrames(VideoEncoder* encoder, int topoff_frames);
+
 // Generate test data and test the encoder and decoder pair.
 //
 // If |strict| is set to true, this routine will make sure the updated rects
diff --git a/remoting/codec/video_encoder.h b/remoting/codec/video_encoder.h
index f497eb3..9da447c 100644
--- a/remoting/codec/video_encoder.h
+++ b/remoting/codec/video_encoder.h
@@ -25,7 +25,10 @@ class VideoEncoder {
   virtual void SetLosslessEncode(bool want_lossless) {}
   virtual void SetLosslessColor(bool want_lossless) {}
 
-  // Encode an image stored in |frame|.
+  // Encode an image stored in |frame|. If |frame.updated_region()| is empty
+  // then the encoder may return a packet (e.g. to top-off previously-encoded
+  // portions of the frame to higher quality) or return nullptr to indicate that
+  // there is no work to do.
   virtual scoped_ptr<VideoPacket> Encode(const webrtc::DesktopFrame& frame) = 0;
 };
 
diff --git a/remoting/codec/video_encoder_verbatim.cc b/remoting/codec/video_encoder_verbatim.cc
index c7550e1..2948e3e 100644
--- a/remoting/codec/video_encoder_verbatim.cc
+++ b/remoting/codec/video_encoder_verbatim.cc
@@ -25,7 +25,12 @@ VideoEncoderVerbatim::~VideoEncoderVerbatim() {}
 
 scoped_ptr<VideoPacket> VideoEncoderVerbatim::Encode(
     const webrtc::DesktopFrame& frame) {
-  CHECK(frame.data());
+  DCHECK(frame.data());
+
+  // If nothing has changed in the frame then return NULL to indicate that
+  // we don't need to actually send anything (e.g. nothing to top-off).
+  if (frame.updated_region().is_empty())
+    return nullptr;
 
   base::Time encode_start_time = base::Time::Now();
 
diff --git a/remoting/codec/video_encoder_verbatim_unittest.cc b/remoting/codec/video_encoder_verbatim_unittest.cc
index b9e72ac..a294d7b 100644
--- a/remoting/codec/video_encoder_verbatim_unittest.cc
+++ b/remoting/codec/video_encoder_verbatim_unittest.cc
@@ -21,4 +21,9 @@ TEST(VideoEncoderVerbatimTest, EncodeAndDecode) {
   TestVideoEncoderDecoder(encoder.get(), decoder.get(), true);
 }
 
+TEST(VideoEncoderVerbatimTest, EncodeUnchangedFrame) {
+  scoped_ptr<VideoEncoderVerbatim> encoder(new VideoEncoderVerbatim());
+  TestVideoEncoderEmptyFrames(encoder.get(), 0);
+}
+
 }  // namespace remoting
diff --git a/remoting/codec/video_encoder_vpx.cc b/remoting/codec/video_encoder_vpx.cc
index c74f0d8..adc6bf9 100644
--- a/remoting/codec/video_encoder_vpx.cc
+++ b/remoting/codec/video_encoder_vpx.cc
@@ -5,7 +5,6 @@
 #include "remoting/codec/video_encoder_vpx.h"
 
 #include "base/bind.h"
-#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
 #include "remoting/base/util.h"
@@ -25,9 +24,6 @@ namespace remoting {
 
 namespace {
 
-// Name of command-line flag to enable VP9 to use I444 by default.
-const char kEnableI444SwitchName[] = "enable-i444";
-
 // Number of bytes in an RGBx pixel.
 const int kBytesPerRgbPixel = 4;
 
@@ -103,7 +99,9 @@ void SetVp9CodecParameters(vpx_codec_enc_cfg_t* config,
     config->rc_max_quantizer = 0;
     config->rc_end_usage = VPX_VBR;
   } else {
-    config->rc_min_quantizer = 4;
+    // TODO(wez): Set quantization range to 4-40, once the libvpx encoder is
+    // updated not to output any bits if nothing needs topping-off.
+    config->rc_min_quantizer = 20;
     config->rc_max_quantizer = 30;
     config->rc_end_usage = VPX_CBR;
     // In the absence of a good bandwidth estimator set the target bitrate to a
@@ -268,6 +266,22 @@ scoped_ptr<VideoPacket> VideoEncoderVpx::Encode(
   DCHECK_LE(32, frame.size().width());
   DCHECK_LE(32, frame.size().height());
 
+  if (!use_vp9_ || lossless_encode_) {
+    // Neither VP8 nor VP9-lossless support top-off, so ignore unchanged frames.
+    if (frame.updated_region().is_empty())
+      return nullptr;
+  } else {
+    // Let VP9-lossy mode top-off, by continuing to pass it unchanged frames
+    // for a short while.
+    if (frame.updated_region().is_empty()) {
+      if (topoff_frame_count_ == 0)
+        return nullptr;
+      topoff_frame_count_--;
+    } else {
+      topoff_frame_count_ = 2;
+    }
+  }
+
   base::TimeTicks encode_start_time = base::TimeTicks::Now();
 
   // Create or reconfigure the codec to match the size of |frame|.
@@ -343,19 +357,7 @@ scoped_ptr<VideoPacket> VideoEncoderVpx::Encode(
   return packet.Pass();
 }
 
-VideoEncoderVpx::VideoEncoderVpx(bool use_vp9)
-    : use_vp9_(use_vp9),
-      lossless_encode_(false),
-      lossless_color_(false),
-      active_map_width_(0),
-      active_map_height_(0) {
-  if (use_vp9_) {
-    // Use I444 colour space, by default, if specified on the command-line.
-    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-            kEnableI444SwitchName)) {
-      SetLosslessColor(true);
-    }
-  }
+VideoEncoderVpx::VideoEncoderVpx(bool use_vp9) : use_vp9_(use_vp9) {
 }
 
 void VideoEncoderVpx::Configure(const webrtc::DesktopSize& size) {
diff --git a/remoting/codec/video_encoder_vpx.h b/remoting/codec/video_encoder_vpx.h
index d8d39eb..dbfefeb 100644
--- a/remoting/codec/video_encoder_vpx.h
+++ b/remoting/codec/video_encoder_vpx.h
@@ -58,8 +58,8 @@ class VideoEncoderVpx : public VideoEncoder {
 
   // Options controlling VP9 encode quantization and color space.
   // These are always off (false) for VP8.
-  bool lossless_encode_;
-  bool lossless_color_;
+  bool lossless_encode_ = false;
+  bool lossless_color_ = false;
 
   // Holds the initialized & configured codec.
   ScopedVpxCodec codec_;
@@ -73,8 +73,12 @@ class VideoEncoderVpx : public VideoEncoder {
 
   // Active map used to optimize out processing of un-changed macroblocks.
   scoped_ptr<uint8[]> active_map_;
-  int active_map_width_;
-  int active_map_height_;
+  int active_map_width_ = 0;
+  int active_map_height_ = 0;
+
+  // Number of "top-off" frames we've encoded since we were last passed a
+  // frame containing an actual change. Used only when encoding with top-off.
+  int topoff_frame_count_ = 0;
 
   // Used to help initialize VideoPackets from DesktopFrames.
   VideoEncoderHelper helper_;
diff --git a/remoting/codec/video_encoder_vpx_unittest.cc b/remoting/codec/video_encoder_vpx_unittest.cc
index 98b3991..efe07d2 100644
--- a/remoting/codec/video_encoder_vpx_unittest.cc
+++ b/remoting/codec/video_encoder_vpx_unittest.cc
@@ -39,19 +39,19 @@ static scoped_ptr<webrtc::DesktopFrame> CreateTestFrame(
   return frame.Pass();
 }
 
-TEST(VideoEncoderVpxTest, TestVp8VideoEncoder) {
+TEST(VideoEncoderVpxTest, Vp8) {
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP8());
   TestVideoEncoder(encoder.get(), false);
 }
 
-TEST(VideoEncoderVpxTest, TestVp9VideoEncoder) {
+TEST(VideoEncoderVpxTest, Vp9) {
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
   // VP9 encoder defaults to lossless encode and lossy (I420) color.
   TestVideoEncoder(encoder.get(), false);
 }
 
 // Test that the VP9 encoder can switch between lossy & lossless encode.
-TEST(VideoEncoderVpxTest, TestVp9VideoEncoderLossyEncode) {
+TEST(VideoEncoderVpxTest, Vp9LossyEncodeSwitching) {
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
 
   webrtc::DesktopSize frame_size(1024, 768);
@@ -77,7 +77,7 @@ TEST(VideoEncoderVpxTest, TestVp9VideoEncoderLossyEncode) {
 }
 
 // Test that the VP9 encoder can switch between lossy & lossless color.
-TEST(VideoEncoderVpxTest, TestVp9VideoEncoderLossyColor) {
+TEST(VideoEncoderVpxTest, Vp9LossyColorSwitching) {
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
 
   webrtc::DesktopSize frame_size(1024, 768);
@@ -97,7 +97,7 @@ TEST(VideoEncoderVpxTest, TestVp9VideoEncoderLossyColor) {
 }
 
 // Test that the VP8 encoder ignores lossless modes without crashing.
-TEST(VideoEncoderVpxTest, TestVp8VideoEncoderIgnoreLossy) {
+TEST(VideoEncoderVpxTest, Vp8IgnoreLossy) {
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP8());
 
   webrtc::DesktopSize frame_size(1024, 768);
@@ -112,7 +112,7 @@ TEST(VideoEncoderVpxTest, TestVp8VideoEncoderIgnoreLossy) {
 
 // Test that calling Encode with a larger frame size than the initial one
 // does not cause VP8 to crash.
-TEST(VideoEncoderVpxTest, TestVp8SizeChangeNoCrash) {
+TEST(VideoEncoderVpxTest, Vp8SizeChangeNoCrash) {
   webrtc::DesktopSize frame_size(1000, 1000);
 
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP8());
@@ -131,7 +131,7 @@ TEST(VideoEncoderVpxTest, TestVp8SizeChangeNoCrash) {
 
 // Test that calling Encode with a larger frame size than the initial one
 // does not cause VP9 to crash.
-TEST(VideoEncoderVpxTest, TestVp9SizeChangeNoCrash) {
+TEST(VideoEncoderVpxTest, Vp9SizeChangeNoCrash) {
   webrtc::DesktopSize frame_size(1000, 1000);
 
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
@@ -150,7 +150,7 @@ TEST(VideoEncoderVpxTest, TestVp9SizeChangeNoCrash) {
 
 // Test that the DPI information is correctly propagated from the
 // media::ScreenCaptureData to the VideoPacket.
-TEST(VideoEncoderVpxTest, TestDpiPropagation) {
+TEST(VideoEncoderVpxTest, DpiPropagation) {
   webrtc::DesktopSize frame_size(32, 32);
 
   scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP8());
@@ -162,4 +162,21 @@ TEST(VideoEncoderVpxTest, TestDpiPropagation) {
   EXPECT_EQ(packet->format().y_dpi(), 97);
 }
 
+TEST(VideoEncoderVerbatimTest, Vp8EncodeUnchangedFrame) {
+  scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP8());
+  TestVideoEncoderEmptyFrames(encoder.get(), 0);
+}
+
+TEST(VideoEncoderVerbatimTest, Vp9LosslessUnchangedFrame) {
+  scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
+  encoder->SetLosslessEncode(true);
+  TestVideoEncoderEmptyFrames(encoder.get(), 0);
+}
+
+TEST(VideoEncoderVerbatimTest, Vp9LossyUnchangedFrame) {
+  scoped_ptr<VideoEncoderVpx> encoder(VideoEncoderVpx::CreateForVP9());
+  encoder->SetLosslessEncode(false);
+  TestVideoEncoderEmptyFrames(encoder.get(), 2);
+}
+
 }  // namespace remoting
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index d6a02fa..2b72eca 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/command_line.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "remoting/base/capabilities.h"
@@ -41,6 +42,9 @@ namespace remoting {
 
 namespace {
 
+// Name of command-line flag to enable VP9 to use I444 by default.
+const char kEnableI444SwitchName[] = "enable-i444";
+
 scoped_ptr<VideoEncoder> CreateVideoEncoder(
     const protocol::SessionConfig& config) {
   const protocol::ChannelConfig& video_config = config.video_config();
@@ -107,7 +111,8 @@ ClientSession::ClientSession(
       is_authenticated_(false),
       pause_video_(false),
       lossless_video_encode_(false),
-      lossless_video_color_(false),
+      lossless_video_color_(base::CommandLine::ForCurrentProcess()->HasSwitch(
+          kEnableI444SwitchName)),
       weak_factory_(this) {
   connection_->SetEventHandler(this);
 
diff --git a/remoting/host/fake_desktop_capturer.cc b/remoting/host/fake_desktop_capturer.cc
index 56a39cf..520d396 100644
--- a/remoting/host/fake_desktop_capturer.cc
+++ b/remoting/host/fake_desktop_capturer.cc
@@ -143,8 +143,10 @@ void FakeDesktopCapturer::Start(Callback* callback) {
 void FakeDesktopCapturer::Capture(const webrtc::DesktopRegion& region) {
   base::Time capture_start_time = base::Time::Now();
   scoped_ptr<webrtc::DesktopFrame> frame = frame_generator_.Run(callback_);
-  frame->set_capture_time_ms(
-      (base::Time::Now() - capture_start_time).InMillisecondsRoundedUp());
+  if (frame) {
+    frame->set_capture_time_ms(
+        (base::Time::Now() - capture_start_time).InMillisecondsRoundedUp());
+  }
   callback_->OnCaptureCompleted(frame.release());
 }
 
diff --git a/remoting/host/host_mock_objects.cc b/remoting/host/host_mock_objects.cc
index c48517f..8fdc11e 100644
--- a/remoting/host/host_mock_objects.cc
+++ b/remoting/host/host_mock_objects.cc
@@ -13,8 +13,10 @@
 #include "remoting/host/audio_capturer.h"
 #include "remoting/host/input_injector.h"
 #include "remoting/proto/event.pb.h"
+#include "remoting/proto/video.pb.h"
 #include "remoting/protocol/transport.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 namespace remoting {
 
@@ -88,4 +90,15 @@ MockMouseCursorMonitor::MockMouseCursorMonitor() {}
 
 MockMouseCursorMonitor::~MockMouseCursorMonitor() {}
 
+MockVideoEncoder::MockVideoEncoder() {
+}
+
+MockVideoEncoder::~MockVideoEncoder() {
+}
+
+scoped_ptr<VideoPacket> MockVideoEncoder::Encode(
+    const webrtc::DesktopFrame& frame) {
+  return make_scoped_ptr(EncodePtr(frame));
+}
+
 }  // namespace remoting
diff --git a/remoting/host/host_mock_objects.h b/remoting/host/host_mock_objects.h
index 52d0170..d4462c8 100644
--- a/remoting/host/host_mock_objects.h
+++ b/remoting/host/host_mock_objects.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "net/base/ip_endpoint.h"
+#include "remoting/codec/video_encoder.h"
 #include "remoting/host/chromoting_host_context.h"
 #include "remoting/host/client_session.h"
 #include "remoting/host/client_session_control.h"
@@ -19,6 +20,7 @@
 #include "remoting/host/screen_resolution.h"
 #include "remoting/proto/control.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 #include "third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor.h"
 
 namespace base {
@@ -164,6 +166,18 @@ class MockMouseCursorMonitor : public webrtc::MouseCursorMonitor {
   DISALLOW_COPY_AND_ASSIGN(MockMouseCursorMonitor);
 };
 
+class MockVideoEncoder : public VideoEncoder {
+ public:
+  MockVideoEncoder();
+  ~MockVideoEncoder() override;
+
+  MOCK_METHOD1(SetLosslessEncode, void(bool));
+  MOCK_METHOD1(SetLosslessColor, void(bool));
+  MOCK_METHOD1(EncodePtr, VideoPacket*(const webrtc::DesktopFrame&));
+
+  scoped_ptr<VideoPacket> Encode(const webrtc::DesktopFrame& frame) override;
+};
+
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_HOST_MOCK_OBJECTS_H_
diff --git a/remoting/host/video_frame_pump.cc b/remoting/host/video_frame_pump.cc
index 526615c..37d9da7 100644
--- a/remoting/host/video_frame_pump.cc
+++ b/remoting/host/video_frame_pump.cc
@@ -22,18 +22,22 @@ namespace remoting {
 
 namespace {
 
-// Helper used to encode frames on the encode thread.
-//
-// TODO(sergeyu): This functions doesn't do much beside calling
-// VideoEncoder::Encode(). It's only needed to handle empty frames properly and
-// that logic can be moved to VideoEncoder implementations.
 scoped_ptr<VideoPacket> EncodeFrame(VideoEncoder* encoder,
                                     scoped_ptr<webrtc::DesktopFrame> frame) {
-  // If there is nothing to encode then send an empty packet.
-  if (!frame || frame->updated_region().is_empty())
-    return make_scoped_ptr(new VideoPacket());
+  scoped_ptr<VideoPacket> packet;
 
-  return encoder->Encode(*frame);
+  // If |frame| is non-NULL then let the encoder process it.
+  if (frame) {
+    packet = encoder->Encode(*frame);
+  }
+
+  // If |frame| is NULL, or the encoder returned nothing, return an empty
+  // packet.
+  if (!packet) {
+    packet.reset(new VideoPacket());
+  }
+
+  return packet.Pass();
 }
 
 }  // namespace
diff --git a/remoting/host/video_frame_pump_unittest.cc b/remoting/host/video_frame_pump_unittest.cc
index cd5cd01..f72644b 100644
--- a/remoting/host/video_frame_pump_unittest.cc
+++ b/remoting/host/video_frame_pump_unittest.cc
@@ -26,9 +26,11 @@
 using ::remoting::protocol::MockVideoStub;
 
 using ::testing::_;
+using ::testing::AtLeast;
 using ::testing::DoAll;
 using ::testing::Expectation;
 using ::testing::InvokeWithoutArgs;
+using ::testing::Return;
 
 namespace remoting {
 
@@ -38,6 +40,18 @@ ACTION(FinishSend) {
   arg1.Run();
 }
 
+scoped_ptr<webrtc::DesktopFrame> CreateNullFrame(
+    webrtc::DesktopCapturer::Callback*) {
+  return nullptr;
+}
+
+scoped_ptr<webrtc::DesktopFrame> CreateUnchangedFrame(
+    webrtc::DesktopCapturer::Callback*) {
+  const webrtc::DesktopSize kSize(800, 640);
+  // updated_region() is already empty by default in new BasicDesktopFrames.
+  return make_scoped_ptr(new webrtc::BasicDesktopFrame(kSize));
+}
+
 }  // namespace
 
 static const int kWidth = 640;
@@ -159,6 +173,65 @@ TEST_F(VideoFramePumpTest, StartAndStop) {
                                      capture_task_runner_, capturer.Pass())),
                                  encoder.Pass(), &video_stub_));
 
+  // Run MessageLoop until the first frame is received.
+  run_loop.Run();
+}
+
+// Tests that the pump handles null frames returned by the capturer.
+TEST_F(VideoFramePumpTest, NullFrame) {
+  scoped_ptr<FakeDesktopCapturer> capturer(new FakeDesktopCapturer);
+  scoped_ptr<MockVideoEncoder> encoder(new MockVideoEncoder);
+
+  base::RunLoop run_loop;
+
+  // Set up the capturer to return null frames.
+  capturer->set_frame_generator(base::Bind(&CreateNullFrame));
+
+  // Expect that the VideoEncoder::Encode() method is never called.
+  EXPECT_CALL(*encoder, EncodePtr(_)).Times(0);
+
+  // When the first ProcessVideoPacket is received we stop the VideoFramePump.
+  EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
+      .WillOnce(DoAll(FinishSend(),
+                      InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit)))
+      .RetiresOnSaturation();
+
+  // Start video frame capture.
+  pump_.reset(new VideoFramePump(encode_task_runner_,
+                                 make_scoped_ptr(new DesktopCapturerProxy(
+                                     capture_task_runner_, capturer.Pass())),
+                                 encoder.Pass(), &video_stub_));
+
+  // Run MessageLoop until the first frame is received..
+  run_loop.Run();
+}
+
+// Tests how the pump handles unchanged frames returned by the capturer.
+TEST_F(VideoFramePumpTest, UnchangedFrame) {
+  scoped_ptr<FakeDesktopCapturer> capturer(new FakeDesktopCapturer);
+  scoped_ptr<MockVideoEncoder> encoder(new MockVideoEncoder);
+
+  base::RunLoop run_loop;
+
+  // Set up the capturer to return unchanged frames.
+  capturer->set_frame_generator(base::Bind(&CreateUnchangedFrame));
+
+  // Expect that the VideoEncoder::Encode() method is called.
+  EXPECT_CALL(*encoder, EncodePtr(_)).WillRepeatedly(Return(nullptr));
+
+  // When the first ProcessVideoPacket is received we stop the VideoFramePump.
+  // TODO(wez): Verify that the generated packet has no content here.
+  EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
+      .WillOnce(DoAll(FinishSend(),
+                      InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit)))
+      .RetiresOnSaturation();
+
+  // Start video frame capture.
+  pump_.reset(new VideoFramePump(encode_task_runner_,
+                                 make_scoped_ptr(new DesktopCapturerProxy(
+                                     capture_task_runner_, capturer.Pass())),
+                                 encoder.Pass(), &video_stub_));
+
   // Run MessageLoop until the first frame is received..
   run_loop.Run();
 }
diff --git a/remoting/host/video_frame_recorder_unittest.cc b/remoting/host/video_frame_recorder_unittest.cc
index ac00e45..3192012 100644
--- a/remoting/host/video_frame_recorder_unittest.cc
+++ b/remoting/host/video_frame_recorder_unittest.cc
@@ -132,11 +132,11 @@ scoped_ptr<webrtc::DesktopFrame> VideoFrameRecorderTest::CreateNextFrame() {
 
   // Fill content, DPI and updated-region based on |frame_count_| so that each
   // generated frame is different.
+  ++frame_count_;
   memset(frame->data(), frame_count_, frame->stride() * kFrameHeight);
   frame->set_dpi(webrtc::DesktopVector(frame_count_, frame_count_));
   frame->mutable_updated_region()->SetRect(
       webrtc::DesktopRect::MakeWH(frame_count_, frame_count_));
-  ++frame_count_;
 
   return frame.Pass();
 }
@@ -160,6 +160,8 @@ void VideoFrameRecorderTest::EncodeTestFrames() {
 void VideoFrameRecorderTest::EncodeDummyFrame() {
   webrtc::BasicDesktopFrame dummy_frame(
       webrtc::DesktopSize(kFrameWidth, kFrameHeight));
+  dummy_frame.mutable_updated_region()->SetRect(
+      webrtc::DesktopRect::MakeWH(kFrameWidth, kFrameHeight));
   ASSERT_TRUE(encoder_->Encode(dummy_frame));
   base::RunLoop().RunUntilIdle();
 }