Switch over to using SkRegions to calculate dirty areas.

remoting/host/differ_unittest.cc

Index: remoting/host/differ_unittest.cc
diff --git a/remoting/host/differ_unittest.cc b/remoting/host/differ_unittest.cc
index 23702a82b92c2bff052482ffe14125d6f357b12c..d700d1e0c44a0e90ea47517b4c685090a77c2eca 100644
--- a/remoting/host/differ_unittest.cc
+++ b/remoting/host/differ_unittest.cc
@@ -44,6 +44,24 @@ class DifferTest : public testing::Test {
     memset(buffer, 0, buffer_size_);
   }
 
+  // Here in DifferTest so that tests can access private methods of Differ.
+  void MarkDirtyBlocks(const void* prev_buffer, const void* curr_buffer) {
+    differ_->MarkDirtyBlocks(prev_buffer, curr_buffer);
+  }
+
+  void MergeBlocks(SkRegion* dirty) {
+    differ_->MergeBlocks(dirty);
+  }
+
+  // Convenience method to count rectangles in a region.
+  int RegionRectCount(const SkRegion& region) {

[Wez, 2011/08/08 20:49:34]

SkRegion really doesn't have a method to query the number of rectangles? :(

[dmac, 2011/08/10 20:30:36]

Nope

+    int count = 0;
+    for(SkRegion::Iterator iter(region); !iter.done(); iter.next()) {
+      ++count;
+    }
+    return count;
+  }
+
   // Convenience wrapper for Differ's DiffBlock that calculates the appropriate
   // offset to the start of the desired block.
   DiffInfo DiffBlock(int block_x, int block_y) {
@@ -123,29 +141,36 @@ class DifferTest : public testing::Test {
     }
   }
 
-  // Verify that the given dirty rect matches the expected |x|, |y|, |width|
+  // Verify that the given dirty region matches the expected |x|, |y|, |width|
   // and |height|.
   // |x|, |y|, |width| and |height| are specified in block (not pixel) units.

[Wez, 2011/08/08 20:49:34]

Update this comment, and the method name, to reflect what it actually does.  It
seems to check that the dirty region contains one rectangle matching the
dimensions given?

Given the usage below, it looks like it should instead be checking that the
SkRegion has exactly one rectangle, of the desired dimensions, though.

[dmac, 2011/08/10 20:30:36]

It is used in different ways below, but I did update name and comment.

-  void CheckDirtyRect(const InvalidRects& rects, int x, int y,
-                      int width, int height) {
-    gfx::Rect r(x * kBlockSize, y * kBlockSize,
-                width * kBlockSize, height * kBlockSize);
-    EXPECT_TRUE(rects.find(r) != rects.end());
+  bool CheckDirtyRegion(const SkRegion& region, int x, int y,
+                        int width, int height) {
+    SkIRect r = SkIRect::MakeXYWH(x * kBlockSize, y * kBlockSize,
+                                  width * kBlockSize, height * kBlockSize);
+    bool found = false;
+    for (SkRegion::Iterator i(region); !found && !i.done(); i.next()) {
+      found = (i.rect() == r);
+    }
+    return found;
   }
 
   // Mark the range of blocks specified and then verify that they are
   // merged correctly.
   // Only one rectangular region of blocks can be checked with this routine.
-  void MarkBlocksAndCheckMerge(int x_origin, int y_origin,
+  bool MarkBlocksAndCheckMerge(int x_origin, int y_origin,
                                int width, int height) {
     ClearDiffInfo();
     MarkBlocks(x_origin, y_origin, width, height);
 
-    scoped_ptr<InvalidRects> dirty(new InvalidRects());
-    differ_->MergeBlocks(dirty.get());
+    SkRegion dirty;
+    MergeBlocks(&dirty);
 
-    ASSERT_EQ(1UL, dirty->size());
-    CheckDirtyRect(*dirty.get(), x_origin, y_origin, width, height);
+    bool is_good = RegionRectCount(dirty) == 1;

[Wez, 2011/08/08 20:49:34]

Simpler to use isComplex() and get rid of RegionRectCount().

[dmac, 2011/08/10 20:30:36]

RegionRectCount is used elsewhere, but I did get rid of this case.

+    if (is_good) {
+     is_good = CheckDirtyRegion(dirty, x_origin, y_origin, width, height);
+    }
+    return is_good;
   }
 
   // The differ class we're testing.
@@ -196,7 +221,7 @@ TEST_F(DifferTest, MarkDirtyBlocks_All) {
     }
   }
 
-  differ_->MarkDirtyBlocks(prev_.get(), curr_.get());
+  MarkDirtyBlocks(prev_.get(), curr_.get());
 
   // Make sure each block is marked as dirty.
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
@@ -216,7 +241,7 @@ TEST_F(DifferTest, MarkDirtyBlocks_Sampling) {
   WriteBlockPixel(curr_.get(), 2, 1, 10, 10, 0xff00ff);
   WriteBlockPixel(curr_.get(), 0, 2, 10, 10, 0xff00ff);
 
-  differ_->MarkDirtyBlocks(prev_.get(), curr_.get());
+  MarkDirtyBlocks(prev_.get(), curr_.get());
 
   // Make sure corresponding blocks are updated.
   EXPECT_EQ(0, GetDiffInfo(0, 0));
@@ -284,7 +309,7 @@ TEST_F(DifferTest, Partial_FirstPixel) {
     }
   }
 
-  differ_->MarkDirtyBlocks(prev_.get(), curr_.get());
+  MarkDirtyBlocks(prev_.get(), curr_.get());
 
   // Make sure each block is marked as dirty.
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
@@ -307,7 +332,7 @@ TEST_F(DifferTest, Partial_BorderPixel) {
     WritePixel(curr_.get(), x, height_ - 1, 0xff00ff);
   }
 
-  differ_->MarkDirtyBlocks(prev_.get(), curr_.get());
+  MarkDirtyBlocks(prev_.get(), curr_.get());
 
   // Make sure last (partial) block in each row/column is marked as dirty.
   int x_last = GetDiffInfoWidth() - 2;
@@ -343,10 +368,10 @@ TEST_F(DifferTest, MergeBlocks_Empty) {
   // +---+---+---+---+
   ClearDiffInfo();
 
-  scoped_ptr<InvalidRects> dirty(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  SkRegion dirty;
+  MergeBlocks(&dirty);
 
-  EXPECT_EQ(0UL, dirty->size());
+  EXPECT_EQ(0, RegionRectCount(dirty));

[Wez, 2011/08/08 20:49:34]

EXPECT_TRUE(dirty.isEmpty());

[dmac, 2011/08/10 20:30:36]

Done.

 }
 
 TEST_F(DifferTest, MergeBlocks_SingleBlock) {
@@ -355,7 +380,8 @@ TEST_F(DifferTest, MergeBlocks_SingleBlock) {
   // rect with the correct bounds.
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
     for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
-      MarkBlocksAndCheckMerge(x, y, 1, 1);
+      ASSERT_TRUE(MarkBlocksAndCheckMerge(x, y, 1, 1)) << "x: " << x
+                                                       << "y: " << y;
     }
   }
 }
@@ -372,7 +398,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRow) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 2, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 2, 1));
 
   // +---+---+---+---+
   // |   |   |   | _ |
@@ -383,7 +409,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRow) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 1, 3, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 1, 3, 1));
 
   // +---+---+---+---+
   // |   |   |   | _ |
@@ -394,7 +420,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRow) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 2, 2, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 2, 2, 1));
 }
 
 TEST_F(DifferTest, MergeBlocks_BlockColumn) {
@@ -409,7 +435,7 @@ TEST_F(DifferTest, MergeBlocks_BlockColumn) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 1, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 1, 2));
 
   // +---+---+---+---+
   // |   |   |   | _ |
@@ -420,7 +446,7 @@ TEST_F(DifferTest, MergeBlocks_BlockColumn) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 1, 1, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 1, 1, 2));
 
   // +---+---+---+---+
   // |   |   | X | _ |
@@ -431,7 +457,7 @@ TEST_F(DifferTest, MergeBlocks_BlockColumn) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(2, 0, 1, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(2, 0, 1, 3));
 }
 
 TEST_F(DifferTest, MergeBlocks_BlockRect) {
@@ -446,7 +472,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 2, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 2, 2));
 
   // +---+---+---+---+
   // |   |   |   | _ |
@@ -457,7 +483,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 1, 2, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 1, 2, 2));
 
   // +---+---+---+---+
   // |   | X | X | _ |
@@ -468,7 +494,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 0, 2, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 0, 2, 3));
 
   // +---+---+---+---+
   // |   |   |   | _ |
@@ -479,7 +505,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 1, 3, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 1, 3, 2));
 
   // +---+---+---+---+
   // | X | X | X | _ |
@@ -490,7 +516,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 3, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 3, 3));
 }
 
 // This tests marked regions that require more than 1 single dirty rect.
@@ -498,7 +524,7 @@ TEST_F(DifferTest, MergeBlocks_BlockRect) {
 // may need to be updated if we modify how we merge blocks.
 TEST_F(DifferTest, MergeBlocks_MultiRect) {
   InitDiffer(kScreenWidth, kScreenHeight);
-  scoped_ptr<InvalidRects> dirty;
+  SkRegion dirty;
 
   // +---+---+---+---+      +---+---+---+
   // |   | X |   | _ |      |   | 0 |   |
@@ -514,40 +540,40 @@ TEST_F(DifferTest, MergeBlocks_MultiRect) {
   MarkBlocks(0, 1, 1, 1);
   MarkBlocks(2, 2, 1, 1);
 
-  dirty.reset(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  dirty.setEmpty();
+  MergeBlocks(&dirty);
 
-  ASSERT_EQ(3UL, dirty->size());
-  CheckDirtyRect(*dirty.get(), 1, 0, 1, 1);
-  CheckDirtyRect(*dirty.get(), 0, 1, 1, 1);
-  CheckDirtyRect(*dirty.get(), 2, 2, 1, 1);
+  ASSERT_EQ(3, RegionRectCount(dirty));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 1, 0, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 1, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 2, 1, 1));

[Wez, 2011/08/08 20:49:34]

Clearer to restructure this to iterate over dirty three times, checking each
rect, then assert empty?

If you don't want to make assumptions about the order the SkRegion will have the
rects in, then for each rect check that it is completely contained by the
region, then subtract it from it, for an equivalent effect.

[dmac, 2011/08/10 20:30:36]

I'm going to leave as is for right now. It keeps all the test cases looking very
similar so you don't need to "learn" a new pattern.

 
   // +---+---+---+---+      +---+---+---+
   // |   |   | X | _ |      |   |   | 0 |
-  // +---+---+---+---+      +---+---+   +
-  // | X | X | X | _ |      | 1   1 | 0 |
-  // +---+---+---+---+  =>  +       +   +
-  // | X | X | X | _ |      | 1   1 | 0 |
+  // +---+---+---+---+      +---+---+---+
+  // | X | X | X | _ |      | 1   1   1 |
+  // +---+---+---+---+  =>  +           +
+  // | X | X | X | _ |      | 1   1   1 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   ClearDiffInfo();
-  MarkBlocks(2, 0, 1, 3);
-  MarkBlocks(0, 1, 2, 2);
+  MarkBlocks(2, 0, 1, 1);
+  MarkBlocks(0, 1, 3, 2);
 
-  dirty.reset(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  dirty.setEmpty();
+  MergeBlocks(&dirty);
 
-  ASSERT_EQ(2UL, dirty->size());
-  CheckDirtyRect(*dirty.get(), 2, 0, 1, 3);
-  CheckDirtyRect(*dirty.get(), 0, 1, 2, 2);
+  ASSERT_EQ(2, RegionRectCount(dirty));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 0, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 1, 3, 2));
 
   // +---+---+---+---+      +---+---+---+
   // |   |   |   | _ |      |   |   |   |
   // +---+---+---+---+      +---+---+---+
   // | X |   | X | _ |      | 0 |   | 1 |
   // +---+---+---+---+  =>  +   +---+   +
-  // | X | X | X | _ |      | 0 | 2 | 1 |
+  // | X | X | X | _ |      | 2 | 2 | 2 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
@@ -556,20 +582,20 @@ TEST_F(DifferTest, MergeBlocks_MultiRect) {
   MarkBlocks(2, 1, 1, 1);
   MarkBlocks(0, 2, 3, 1);
 
-  dirty.reset(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  dirty.setEmpty();
+  MergeBlocks(&dirty);
 
-  ASSERT_EQ(3UL, dirty->size());
-  CheckDirtyRect(*dirty.get(), 0, 1, 1, 2);
-  CheckDirtyRect(*dirty.get(), 2, 1, 1, 2);
-  CheckDirtyRect(*dirty.get(), 1, 2, 1, 1);
+  ASSERT_EQ(3, RegionRectCount(dirty));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 1, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 1, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 2, 3, 1));
 
   // +---+---+---+---+      +---+---+---+
   // | X | X | X | _ |      | 0   0   0 |
   // +---+---+---+---+      +---+---+---+
   // | X |   | X | _ |      | 1 |   | 2 |
   // +---+---+---+---+  =>  +   +---+   +
-  // | X | X | X | _ |      | 1 | 3 | 2 |
+  // | X | X | X | _ |      | 3 | 3 | 3 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
@@ -579,14 +605,14 @@ TEST_F(DifferTest, MergeBlocks_MultiRect) {
   MarkBlocks(2, 1, 1, 1);
   MarkBlocks(0, 2, 3, 1);
 
-  dirty.reset(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  dirty.setEmpty();
+  MergeBlocks(&dirty);
 
-  ASSERT_EQ(4UL, dirty->size());
-  CheckDirtyRect(*dirty.get(), 0, 0, 3, 1);
-  CheckDirtyRect(*dirty.get(), 0, 1, 1, 2);
-  CheckDirtyRect(*dirty.get(), 2, 1, 1, 2);
-  CheckDirtyRect(*dirty.get(), 1, 2, 1, 1);
+  ASSERT_EQ(4, RegionRectCount(dirty));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 0, 3, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 1, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 1, 1, 1));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 2, 3, 1));
 
   // +---+---+---+---+      +---+---+---+
   // | X | X |   | _ |      | 0   0 |   |
@@ -601,12 +627,12 @@ TEST_F(DifferTest, MergeBlocks_MultiRect) {
   MarkBlocks(0, 0, 2, 2);
   MarkBlocks(1, 2, 1, 1);
 
-  dirty.reset(new InvalidRects());
-  differ_->MergeBlocks(dirty.get());
+  dirty.setEmpty();
+  MergeBlocks(&dirty);
 
-  ASSERT_EQ(2UL, dirty->size());
-  CheckDirtyRect(*dirty.get(), 0, 0, 2, 2);
-  CheckDirtyRect(*dirty.get(), 1, 2, 1, 1);
+  ASSERT_EQ(2, RegionRectCount(dirty));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 0, 0, 2, 2));
+  ASSERT_TRUE(CheckDirtyRegion(dirty, 1, 2, 1, 1));
 }
 
 }  // namespace remoting