Improve AlignRect() readability, and handle bad rectangles more gracefully (without crashing in gfx::Rect ctor).

Also fix cpplint nit.

BUG=74809
TEST=Follow repro steps of bug.

Review URL: http://codereview.chromium.org/6628051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@78541 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 60 insertions, 19 deletions

diff --git a/remoting/base/encoder_vp8.cc b/remoting/base/encoder_vp8.cc
index b0f35cb..6c10a2c 100644
--- a/remoting/base/encoder_vp8.cc
+++ b/remoting/base/encoder_vp8.cc
@@ -115,23 +115,20 @@ static int RoundToTwosMultiple(int x) {
   return x & (~1);
 }
 
-// Align the sides of the rectange to multiples of 2.
-static gfx::Rect AlignRect(const gfx::Rect& rect, int width, int height) {
-  CHECK(rect.width() > 0 && rect.height() > 0);
+// Align the sides of the rectangle to multiples of 2 (expanding outwards).
+static gfx::Rect AlignRect(const gfx::Rect& rect) {
   int x = RoundToTwosMultiple(rect.x());
   int y = RoundToTwosMultiple(rect.y());
-  int right = std::min(RoundToTwosMultiple(rect.right() + 1),
-                       RoundToTwosMultiple(width));
-  int bottom = std::min(RoundToTwosMultiple(rect.bottom() + 1),
-                        RoundToTwosMultiple(height));
-
-  // Do the final check to make sure the width and height are not negative.
-  gfx::Rect r(x, y, right - x, bottom - y);
-  if (r.width() <= 0 || r.height() <= 0) {
-    r.set_width(0);
-    r.set_height(0);
-  }
-  return r;
+  int right = RoundToTwosMultiple(rect.right() + 1);
+  int bottom = RoundToTwosMultiple(rect.bottom() + 1);
+  return gfx::Rect(x, y, right - x, bottom - y);
+}
+
+// static
+gfx::Rect EncoderVp8::AlignAndClipRect(const gfx::Rect& rect,
+                                       int width, int height) {
+  gfx::Rect screen(RoundToTwosMultiple(width), RoundToTwosMultiple(height));
+  return screen.Intersect(AlignRect(rect));
 }
 
 bool EncoderVp8::PrepareImage(scoped_refptr<CaptureData> capture_data,
@@ -155,9 +152,10 @@ bool EncoderVp8::PrepareImage(scoped_refptr<CaptureData> capture_data,
 
   DCHECK(updated_rects->empty());
   for (InvalidRects::const_iterator r = rects.begin(); r != rects.end(); ++r) {
-    // Align the rectangle report it as updated.
-    gfx::Rect rect = AlignRect(*r, image_->w, image_->h);
-    updated_rects->push_back(rect);
+    // Align the rectangle, report it as updated.
+    gfx::Rect rect = AlignAndClipRect(*r, image_->w, image_->h);
+    if (!rect.IsEmpty())
+      updated_rects->push_back(rect);
 
     ConvertRGB32ToYUVWithRect(in,
                               y_out,
@@ -223,7 +221,7 @@ void EncoderVp8::Encode(scoped_refptr<CaptureData> capture_data,
   act_map.rows = active_map_height_;
   act_map.cols = active_map_width_;
   act_map.active_map = active_map_.get();
-  if(vpx_codec_control(codec_.get(), VP8E_SET_ACTIVEMAP, &act_map)) {
+  if (vpx_codec_control(codec_.get(), VP8E_SET_ACTIVEMAP, &act_map)) {
     LOG(ERROR) << "Unable to apply active map";
   }
 
diff --git a/remoting/base/encoder_vp8.h b/remoting/base/encoder_vp8.h
index 8fdbbc2..6ded488 100644
--- a/remoting/base/encoder_vp8.h
+++ b/remoting/base/encoder_vp8.h
@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "remoting/base/encoder.h"
 #include "ui/gfx/rect.h"
 
@@ -26,6 +27,8 @@ class EncoderVp8 : public Encoder {
                       DataAvailableCallback* data_available_callback);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(EncoderVp8Test, AlignAndClipRect);
+
   // Initialize the encoder. Returns true if successful.
   bool Init(const gfx::Size& size);
 
@@ -38,6 +41,15 @@ class EncoderVp8 : public Encoder {
   // given to the encoder to speed up encoding.
   void PrepareActiveMap(const std::vector<gfx::Rect>& updated_rects);
 
+  // Align the sides of the rectangle to multiples of 2 (expanding outwards),
+  // but ensuring the result stays within the screen area (width, height).
+  // If |rect| falls outside the screen area, return an empty rect.
+  //
+  // TODO(lambroslambrou): Pull this out if it's useful for other things than
+  // VP8-encoding?
+  static gfx::Rect AlignAndClipRect(const gfx::Rect& rect,
+                                    int width, int height);
+
   // True if the encoder is initialized.
   bool initialized_;
 
diff --git a/remoting/base/encoder_vp8_unittest.cc b/remoting/base/encoder_vp8_unittest.cc
index 353b82b..370fd08 100644
--- a/remoting/base/encoder_vp8_unittest.cc
+++ b/remoting/base/encoder_vp8_unittest.cc
@@ -2,10 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <limits>
+
 #include "remoting/base/codec_test.h"
 #include "remoting/base/encoder_vp8.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+const int kIntMax = std::numeric_limits<int>::max();
+
+}  // namespace
+
 namespace remoting {
 
 TEST(EncoderVp8Test, TestEncoder) {
@@ -13,4 +21,27 @@ TEST(EncoderVp8Test, TestEncoder) {
   TestEncoder(&encoder, false);
 }
 
+TEST(EncoderVp8Test, AlignAndClipRect) {
+  // Simple test case (no clipping).
+  gfx::Rect r1(100, 200, 300, 400);
+  EXPECT_EQ(EncoderVp8::AlignAndClipRect(r1, kIntMax, kIntMax), r1);
+
+  // Should expand outward to r1.
+  gfx::Rect r2(101, 201, 298, 398);
+  EXPECT_EQ(EncoderVp8::AlignAndClipRect(r2, kIntMax, kIntMax), r1);
+
+  // Test clipping to screen size.
+  EXPECT_EQ(EncoderVp8::AlignAndClipRect(r1, 110, 220),
+            gfx::Rect(100, 200, 10, 20));
+
+  // Rectangle completely off-screen.
+  EXPECT_TRUE(EncoderVp8::AlignAndClipRect(r1, 50, 50).IsEmpty());
+
+  // Clipping to odd-sized screen.  An unlikely case, and we might not deal
+  // with it cleanly in the encoder (we possibly lose 1px at right & bottom
+  // of screen).
+  EXPECT_EQ(EncoderVp8::AlignAndClipRect(r1, 199, 299),
+            gfx::Rect(100, 200, 98, 98));
+}
+
 }  // namespace remoting