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}

8 files changed, 97 insertions, 34 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