Switch over to using SkRegions to calculate dirty areas.

remoting/host/differ_unittest.cc

 // Copyright (c) 2011 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 "base/memory/scoped_ptr.h"
 #include "remoting/host/differ.h"
 #include "remoting/host/differ_block.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace remoting {
@@ skipping 26 matching lines @@
     memset(prev_.get(), 0, buffer_size_);
 
     curr_.reset(new uint8[buffer_size_]);
     memset(curr_.get(), 0, buffer_size_);
   }
 
   void ClearBuffer(uint8* buffer) {
     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) {
     // Offset from upper-left of buffer to upper-left of requested block.
     int block_offset = ((block_y * stride_) + (block_x * bytes_per_pixel_))
                         * kBlockSize;
     return BlockDifference(prev_.get() + block_offset,
                            curr_.get() + block_offset,
                            stride_);
   }
@@ skipping 59 matching lines @@
 
   // Mark the range of blocks specified.
   void MarkBlocks(int x_origin, int y_origin, int width, int height) {
     for (int y = 0; y < height; y++) {
       for (int x = 0; x < width; x++) {
         SetDiffInfo(x_origin + x, y_origin + y, 1);
       }
     }
   }
 
-  // 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.
   scoped_ptr<Differ> differ_;
 
   // Screen/buffer info.
   int width_;
   int height_;
   int bytes_per_pixel_;
   int stride_;
@@ skipping 30 matching lines @@
   InitDiffer(kScreenWidth, kScreenHeight);
   ClearDiffInfo();
 
   // Update a pixel in each block.
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
     for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
       WriteBlockPixel(curr_.get(), x, y, 10, 10, 0xff00ff);
     }
   }
 
-  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++) {
     for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
       EXPECT_EQ(1, GetDiffInfo(x, y))
           << "when x = " << x << ", and y = " << y;
     }
   }
 }
 
 TEST_F(DifferTest, MarkDirtyBlocks_Sampling) {
   InitDiffer(kScreenWidth, kScreenHeight);
   ClearDiffInfo();
 
   // Update some pixels in image.
   WriteBlockPixel(curr_.get(), 1, 0, 10, 10, 0xff00ff);
   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));
   EXPECT_EQ(0, GetDiffInfo(0, 1));
   EXPECT_EQ(1, GetDiffInfo(0, 2));
   EXPECT_EQ(1, GetDiffInfo(1, 0));
   EXPECT_EQ(0, GetDiffInfo(1, 1));
   EXPECT_EQ(0, GetDiffInfo(1, 2));
   EXPECT_EQ(0, GetDiffInfo(2, 0));
   EXPECT_EQ(1, GetDiffInfo(2, 1));
@@ skipping 47 matching lines @@
   InitDiffer(kPartialScreenWidth, kPartialScreenHeight);
   ClearDiffInfo();
 
   // Update the first pixel in each block.
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
     for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
       WriteBlockPixel(curr_.get(), x, y, 0, 0, 0xff00ff);
     }
   }
 
-  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++) {
     for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
       EXPECT_EQ(1, GetDiffInfo(x, y))
           << "when x = " << x << ", and y = " << y;
     }
   }
 }
 
 TEST_F(DifferTest, Partial_BorderPixel) {
   InitDiffer(kPartialScreenWidth, kPartialScreenHeight);
   ClearDiffInfo();
 
   // Update the right/bottom border pixels.
   for (int y = 0; y < height_; y++) {
     WritePixel(curr_.get(), width_ - 1, y, 0xff00ff);
   }
   for (int x = 0; x < width_; x++) {
     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;
   for (int y = 0; y < GetDiffInfoHeight() - 1; y++) {
     EXPECT_EQ(1, GetDiffInfo(x_last, y))
         << "when x = " << x_last << ", and y = " << y;
   }
   int y_last = GetDiffInfoHeight() - 2;
   for (int x = 0; x < GetDiffInfoWidth() - 1; x++) {
     EXPECT_EQ(1, GetDiffInfo(x, y_last))
@@ skipping 15 matching lines @@
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   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) {
   InitDiffer(kScreenWidth, kScreenHeight);
   // Mark a single block and make sure that there is a single merged
   // 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;
     }
   }
 }
 
 TEST_F(DifferTest, MergeBlocks_BlockRow) {
   InitDiffer(kScreenWidth, kScreenHeight);
 
   // +---+---+---+---+
   // | X | X |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 2, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 2, 1));
 
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 1, 3, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 1, 3, 1));
 
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 2, 2, 1);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 2, 2, 1));
 }
 
 TEST_F(DifferTest, MergeBlocks_BlockColumn) {
   InitDiffer(kScreenWidth, kScreenHeight);
 
   // +---+---+---+---+
   // | X |   |   | _ |
   // +---+---+---+---+
   // | X |   |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 1, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 1, 2));
 
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   | X |   | _ |
   // +---+---+---+---+
   // |   | X |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 1, 1, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 1, 1, 2));
 
   // +---+---+---+---+
   // |   |   | X | _ |
   // +---+---+---+---+
   // |   |   | X | _ |
   // +---+---+---+---+
   // |   |   | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(2, 0, 1, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(2, 0, 1, 3));
 }
 
 TEST_F(DifferTest, MergeBlocks_BlockRect) {
   InitDiffer(kScreenWidth, kScreenHeight);
 
   // +---+---+---+---+
   // | X | X |   | _ |
   // +---+---+---+---+
   // | X | X |   | _ |
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 2, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 2, 2));
 
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 1, 2, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 1, 2, 2));
 
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // |   | X | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(1, 0, 2, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(1, 0, 2, 3));
 
   // +---+---+---+---+
   // |   |   |   | _ |
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 1, 3, 2);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 1, 3, 2));
 
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // | X | X | X | _ |
   // +---+---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
-  MarkBlocksAndCheckMerge(0, 0, 3, 3);
+  ASSERT_TRUE(MarkBlocksAndCheckMerge(0, 0, 3, 3));
 }
 
 // This tests marked regions that require more than 1 single dirty rect.
 // The exact rects returned depend on the current implementation, so these
 // 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 |   |
   // +---+---+---+---+      +---+---+---+
   // | X |   |   | _ |      | 1 |   |   |
   // +---+---+---+---+  =>  +---+---+---+
   // |   |   | X | _ |      |   |   | 2 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   ClearDiffInfo();
   MarkBlocks(1, 0, 1, 1);
   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 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   ClearDiffInfo();
   MarkBlocks(0, 1, 1, 1);
   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 |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   ClearDiffInfo();
   MarkBlocks(0, 0, 3, 1);
   MarkBlocks(0, 1, 1, 1);
   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 |   |
   // +---+---+---+---+      +       +---+
   // | X | X |   | _ |      | 0   0 |   |
   // +---+---+---+---+  =>  +---+---+---+
   // |   | X |   | _ |      |   | 1 |   |
   // +---+---+---+---+      +---+---+---+
   // | _ | _ | _ | _ |
   // +---+---+---+---+
   ClearDiffInfo();
   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