Use RunLoop to wait for readback on compositor unittest

ui/compositor/layer_unittest.cc

 // Copyright (c) 2012 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/basictypes.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/debug/trace_event.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ skipping 114 matching lines @@
     layer->SetBounds(bounds);
     return layer;
   }
 
   void DrawTree(Layer* root) {
     GetCompositor()->SetRootLayer(root);
     GetCompositor()->ScheduleDraw();
     WaitForSwap();
   }
 
-  bool ReadPixels(SkBitmap* bitmap) {
+  void ReadPixels(SkBitmap* bitmap) {
     return ReadPixels(bitmap, gfx::Rect(GetCompositor()->size()));

[danakj, 2014/09/19 20:52:26]

remove return

[weiliangc, 2014/09/19 21:02:46]

Done.

   }
 
-  bool ReadPixels(SkBitmap* bitmap, gfx::Rect source_rect) {
+  void ReadPixels(SkBitmap* bitmap, gfx::Rect source_rect) {
     scoped_refptr<ReadbackHolder> holder(new ReadbackHolder);
     scoped_ptr<cc::CopyOutputRequest> request =
         cc::CopyOutputRequest::CreateBitmapRequest(
             base::Bind(&ReadbackHolder::OutputRequestCallback, holder));
     request->set_area(source_rect);
 
     GetCompositor()->root_layer()->RequestCopyOfOutput(request.Pass());
 
     // Wait for copy response.  The copy output request will get committed
     // before the first draw, but may not be part of the first draw's frame.
     // The second draw will perform the async copy request, post the callback.
-    // The second loop finishes before the callback is run, so a third
-    // loop is needed.
-    for (int i = 0; i < 3; i++) {
+    for (int i = 0; i < 2; i++) {
       GetCompositor()->ScheduleFullRedraw();
       WaitForDraw();
     }
 
-    if (holder->completed()) {
-      *bitmap = holder->result();
-      return true;
-    }
+    // Waits for the callback to finish run and return result.
+    holder->WaitForReadback();
 
-    // Callback never called.
-    NOTREACHED();
-    return false;
+    *bitmap = holder->result();
   }
 
   void WaitForDraw() {
     ui::DrawWaiterForTest::WaitForCompositingStarted(GetCompositor());
   }
 
   void WaitForSwap() {
     DrawWaiterForTest::WaitForCompositingEnded(GetCompositor());
   }
 
   void WaitForCommit() {
     ui::DrawWaiterForTest::WaitForCommit(GetCompositor());
   }
 
   // Invalidates the entire contents of the layer.
   void SchedulePaintForLayer(Layer* layer) {
     layer->SchedulePaint(
         gfx::Rect(0, 0, layer->bounds().width(), layer->bounds().height()));
   }
 
   const base::FilePath& test_data_directory() const {
     return test_data_directory_;
   }
 
  private:
   class ReadbackHolder : public base::RefCountedThreadSafe<ReadbackHolder> {
    public:
-    ReadbackHolder() : completed_(false) {}
+    ReadbackHolder() : run_loop_(new base::RunLoop) {}
 
     void OutputRequestCallback(scoped_ptr<cc::CopyOutputResult> result) {
-      DCHECK(!completed_);
+      DCHECK(run_loop_->running());

[danakj, 2014/09/19 20:52:26]

what if the result comes before the wait starts, this looks like a race and it's
not important.

[weiliangc, 2014/09/19 21:02:46]

Make sense. Removed.

       result_ = result->TakeBitmap();
-      completed_ = true;
+      base::MessageLoop::current()->PostTask(FROM_HERE,
+                                             run_loop_->QuitClosure());
     }
-    bool completed() const {
-      return completed_;
-    };
+
+    void WaitForReadback() { run_loop_->Run(); }

[danakj, 2014/09/19 20:52:26]

can you verify that Run() will exit if you called the QuitClosure() before
calling Run() otherwise the race is bad, and you need to check if the callback
already happened and skip Run()

[weiliangc, 2014/09/19 21:02:46]

RunLoop's comment on its Quit and QuitClose function says:
- calling Quit before Run will just cause Run to exit immediately
- QuitClosure calls Quit safely, has no effect if RunLoop is gone

How much trust can I put into these comments?

+
     const SkBitmap& result() const { return *result_; }
 
    private:
     friend class base::RefCountedThreadSafe<ReadbackHolder>;
 
     virtual ~ReadbackHolder() {}
 
     scoped_ptr<SkBitmap> result_;
-    bool completed_;
+    scoped_ptr<base::RunLoop> run_loop_;
   };
 
   scoped_ptr<TestCompositorHost> compositor_host_;
 
   // The root directory for test files.
   base::FilePath test_data_directory_;
 
   DISALLOW_COPY_AND_ASSIGN(LayerWithRealCompositorTest);
 };
 
@@ skipping 665 matching lines @@
       CreateColorLayer(SK_ColorRED, gfx::Rect(viewport_size)));
   scoped_ptr<Layer> layer2(
       CreateColorLayer(SK_ColorBLUE,
                        gfx::Rect(0, 0, viewport_size.width(), blue_height)));
 
   layer->Add(layer2.get());
 
   DrawTree(layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
   for (int x = 0; x < viewport_size.width(); x++) {
     for (int y = 0; y < viewport_size.height(); y++) {
       SkColor actual_color = bitmap.getColor(x, y);
       SkColor expected_color = y < blue_height ? SK_ColorBLUE : SK_ColorRED;
       ExpectRgba(x, y, expected_color, actual_color);
     }
   }
@@ skipping 17 matching lines @@
   scoped_ptr<Layer> foreground_layer(
       CreateColorLayer(blue_with_alpha, gfx::Rect(viewport_size)));
 
   // This must be set to false for layers with alpha to be blended correctly.
   foreground_layer->SetFillsBoundsOpaquely(false);
 
   background_layer->Add(foreground_layer.get());
   DrawTree(background_layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
   for (int x = 0; x < test_size; x++) {
     for (int y = 0; y < test_size; y++) {
       SkColor actual_color = bitmap.getColor(x, y);
       ExpectRgba(x, y, blend_color, actual_color);
     }
   }
 }
@@ skipping 20 matching lines @@
   SkRegion shape;
   shape.setRect(0, 0, viewport_size.width(), blue_height);
   foreground_layer->SetAlphaShape(make_scoped_ptr(new SkRegion(shape)));
 
   foreground_layer->SetFillsBoundsOpaquely(false);
 
   background_layer->Add(foreground_layer.get());
   DrawTree(background_layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
   for (int x = 0; x < test_size; x++) {
     for (int y = 0; y < test_size; y++) {
       SkColor actual_color = bitmap.getColor(x, y);
       ExpectRgba(x, y, actual_color,
                  y < blue_height ? blend_color : SK_ColorRED);
     }
   }
@@ skipping 125 matching lines @@
   base::FilePath ref_img1 =
       test_data_directory().AppendASCII("ModifyHierarchy1.png");
   base::FilePath ref_img2 =
       test_data_directory().AppendASCII("ModifyHierarchy2.png");
   SkBitmap bitmap;
 
   l0->Add(l11.get());
   l11->Add(l21.get());
   l0->Add(l12.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img1);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   l0->StackAtTop(l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img2);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 
   // should restore to original configuration
   l0->StackAbove(l12.get(), l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   // l11 back to front
   l0->StackAtTop(l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 
   // should restore to original configuration
   l0->StackAbove(l12.get(), l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   // l11 back to front
   l0->StackAbove(l11.get(), l12.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 }
 
 // Opacity is rendered correctly.
 // Checks that modifying the hierarchy correctly affects final composite.
 TEST_F(LayerWithRealCompositorTest, Opacity) {
   GetCompositor()->SetScaleAndSize(1.0f, gfx::Size(50, 50));
 
   // l0
   //  +-l11
   scoped_ptr<Layer> l0(CreateColorLayer(SK_ColorRED,
                                         gfx::Rect(0, 0, 50, 50)));
   scoped_ptr<Layer> l11(CreateColorLayer(SK_ColorGREEN,
                                          gfx::Rect(0, 0, 25, 25)));
 
   base::FilePath ref_img = test_data_directory().AppendASCII("Opacity.png");
 
   l11->SetOpacity(0.75);
   l0->Add(l11.get());
   DrawTree(l0.get());
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img, cc::ExactPixelComparator(true)));
 }
 
 namespace {
 
 class SchedulePaintLayerDelegate : public LayerDelegate {
  public:
   SchedulePaintLayerDelegate() : paint_count_(0), layer_(NULL) {}
@@ skipping 540 matching lines @@
                                      MakeFrameData(gfx::Size(10, 10))));
   layer->SetShowDelegatedContent(frame_provider.get(), gfx::Size(10, 10));
 
   EXPECT_FALSE(delegate.delegated_frame_damage_called());
   layer->OnDelegatedFrameDamage(damage_rect);
   EXPECT_TRUE(delegate.delegated_frame_damage_called());
   EXPECT_EQ(damage_rect, delegate.delegated_frame_damage_rect());
 }
 
 }  // namespace ui