Aura: Resize locks with --ui-enable-threaded-compositing

ui/compositor/compositor.cc

Index: ui/compositor/compositor.cc
diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index 65a7a75e5acde6202027f27db6c8be408ba949fc..788747d74713f8b4043db95b225324b4f052d1a8 100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -6,7 +6,9 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/command_line.h"
+#include "base/message_loop.h"
 #include "base/threading/thread_restrictions.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/images/SkImageEncoder.h"
@@ -40,6 +42,8 @@ bool test_compositor_enabled = false;
 
 ui::ContextFactory* g_context_factory = NULL;
 
+const int kCompositorLockTimeoutMs = 67;
+
 }  // namespace
 
 namespace ui {
@@ -129,6 +133,25 @@ Texture::Texture(bool flipped, const gfx::Size& size)
 Texture::~Texture() {
 }
 
+CompositorLock::CompositorLock(Compositor* compositor)
+    : compositor_(compositor) {
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CompositorLock::CancelLock, AsWeakPtr()),
+      base::TimeDelta::FromMilliseconds(kCompositorLockTimeoutMs));
+}
+
+CompositorLock::~CompositorLock() {
+  CancelLock();
+}
+
+void CompositorLock::CancelLock() {
+  if (!compositor_)
+    return;
+  compositor_->Unlock();
+  compositor_ = NULL;
+}
+
 Compositor::Compositor(CompositorDelegate* delegate,
                        gfx::AcceleratedWidget widget)
     : delegate_(delegate),
@@ -139,7 +162,9 @@ Compositor::Compositor(CompositorDelegate* delegate,
       device_scale_factor_(0.0f),
       last_started_frame_(0),
       last_ended_frame_(0),
-      disable_schedule_composite_(false) {
+      disable_schedule_composite_(false),
+      commits_allowed_(ENABLED_NEEDS_COMMIT),
+      compositor_lock_(NULL) {
   WebKit::WebLayerTreeView::Settings settings;
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   settings.showFPSCounter =
@@ -162,6 +187,11 @@ Compositor::Compositor(CompositorDelegate* delegate,
 }
 
 Compositor::~Compositor() {
+  if (compositor_lock_) {
+    compositor_lock_->CancelLock();
+    DCHECK(!compositor_lock_);
+  }
+
   // Don't call |CompositorDelegate::ScheduleDraw| from this point.
   delegate_ = NULL;
   // There's a cycle between |root_web_layer_| and |host_|, which results in
@@ -230,15 +260,24 @@ void Compositor::Draw(bool force_clear) {
     return;
 
   last_started_frame_++;
-  if (!g_compositor_thread)
+  if (!g_compositor_thread) {
     FOR_EACH_OBSERVER(CompositorObserver,
                       observer_list_,
                       OnCompositingWillStart(this));
+  }
+
+  // Fast track draw because visible state could not have changed.
+  if (!g_compositor_thread && commits_allowed_ == DISABLED) {
+    FOR_EACH_OBSERVER(CompositorObserver,
+                      observer_list_,
+                      OnCompositingStarted(this));
+  } else {
+    // TODO(nduca): Temporary while compositor calls
+    // compositeImmediately() directly.
+    layout();
+    host_.composite();
+  }
 
-  // TODO(nduca): Temporary while compositor calls
-  // compositeImmediately() directly.
-  layout();
-  host_.composite();
   if (!g_compositor_thread && !swap_posted_)
     NotifyEnd();
 }
@@ -351,12 +390,20 @@ WebKit::WebGraphicsContext3D* Compositor::createContext3D() {
 void Compositor::didRebindGraphicsContext(bool success) {
 }
 
-// Called once per draw in single-threaded compositor mode and potentially
-// many times between draws in the multi-threaded compositor mode.
 void Compositor::didCommit() {
-  FOR_EACH_OBSERVER(CompositorObserver,
-                    observer_list_,
-                    OnCompositingDidCommit(this));
+  if (commits_allowed_ != DISABLED) {
+    FOR_EACH_OBSERVER(CompositorObserver,
+                      observer_list_,
+                      OnCompositingDidCommit(this));
+  }
+
+  if (commits_allowed_ == DISABLED_PENDING_COMMIT) {
+    commits_allowed_ = DISABLED;
+    root_web_layer_.setDeferUpdates(true);
+  }
+
+  if (commits_allowed_ == ENABLED_NEEDS_COMMIT)
+    commits_allowed_ = ENABLED_DID_COMMIT;
 }
 
 void Compositor::didCommitAndDrawFrame() {
@@ -380,6 +427,27 @@ void Compositor::scheduleComposite() {
     ScheduleDraw();
 }
 
+scoped_refptr<CompositorLock> Compositor::GetCompositorLock() {
+  if (!compositor_lock_) {
+    compositor_lock_ = new CompositorLock(this);
+    if (commits_allowed_ == ENABLED_NEEDS_COMMIT) {
+      commits_allowed_ = DISABLED_PENDING_COMMIT;

[piman, 2012/08/01 18:25:34]

So, I think I understand why we're doing this - the case where we unlock the
compositor (we got the right-sized buffer) and draw a frame with it, before we
lock it again for the next resize.
My worry is that by waiting on a commit (it can take a long time in the
single-thread case, if the unlock comes while we're waiting for the swap), the
next resize might "sneak in" before we have a chance to lock again, causing us
to draw a "bad" frame.
The logic in RWHVA should prevent it from happening (we block resizes until the
commit did happen), but then, it feels that this logic (don't lock again until
the next commit) also belongs there. Otherwise an unrelated RWHVA (e.g. another
window) may want to lock as well, but will silently fail to do so (at least,
until the next commit).
WDYT? I'm afraid moving the logic there will make the logic more complicated,
and maybe this is "good enough" but OTOH I'm afraid that we may have to
work-around the limitation and complexity will build up at the wrong place.

+    } else {
+      commits_allowed_ = DISABLED;
+      root_web_layer_.setDeferUpdates(true);
+    }
+  }
+  return compositor_lock_;
+}
+
+void Compositor::Unlock() {
+  DCHECK(compositor_lock_);
+  compositor_lock_ = NULL;
+
+  commits_allowed_ = ENABLED_NEEDS_COMMIT;
+  root_web_layer_.setDeferUpdates(false);
+}
+
 void Compositor::SwizzleRGBAToBGRAAndFlip(unsigned char* pixels,
                                           const gfx::Size& image_size) {
   // Swizzle from RGBA to BGRA