cc/blimp: Add synchronization for scroll/scale state.

blimp/client/core/compositor/blimp_compositor.cc

 // Copyright 2015 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 "blimp/client/core/compositor/blimp_compositor.h"
 
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 69 matching lines @@
   surface_id_allocator_ = base::MakeUnique<cc::SurfaceIdAllocator>();
   GetEmbedderDeps()->GetSurfaceManager()->RegisterFrameSinkId(frame_sink_id_);
   CreateLayerTreeHost();
 
   if (use_threaded_layer_tree_host_) {
     std::unique_ptr<cc::ClientPictureCache> client_picture_cache =
         compositor_dependencies_->GetImageSerializationProcessor()
             ->CreateClientPictureCache();
     compositor_state_deserializer_ =
         base::MakeUnique<cc::CompositorStateDeserializer>(
-            host_.get(), std::move(client_picture_cache),
-            base::Bind(&BlimpCompositor::LayerScrolled,
-                       weak_ptr_factory_.GetWeakPtr()),
-            this);
+            host_.get(), std::move(client_picture_cache), this);
   }
 }
 
 BlimpCompositor::~BlimpCompositor() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   DestroyLayerTreeHost();
   GetEmbedderDeps()->GetSurfaceManager()->InvalidateFrameSinkId(frame_sink_id_);
 
   CheckPendingCommitCounts(true /* flush */);
@@ skipping 10 matching lines @@
   if (outstanding_commits_ == 0) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
     return;
   }
 
   pending_commit_trackers_.push_back(
       std::make_pair(outstanding_commits_, callback));
 }
 
 void BlimpCompositor::UpdateLayerTreeHost() {
+  DCHECK(use_threaded_layer_tree_host_);
+
+  // UpdateLayerTreeHost marks the end of reporting of any deltas from the impl
+  // thread. So send a client state update if the local state was modified now.
+  FlushClientState();
+
   if (pending_frame_update_) {
     DCHECK(use_threaded_layer_tree_host_);
     compositor_state_deserializer_->DeserializeCompositorUpdate(
         pending_frame_update_->layer_tree_host());
     pending_frame_update_ = nullptr;
     cc::proto::CompositorMessage frame_ack;
     frame_ack.set_frame_ack(true);
     client_->SendCompositorMessage(frame_ack);
   }
+
+  // Send back any deltas that have not yet been resolved on the main thread
+  // back to the impl thread.
+  compositor_state_deserializer_->SendUnappliedDeltasToLayerTreeHost();
+}
+
+void BlimpCompositor::ApplyViewportDeltas(
+    const gfx::Vector2dF& inner_delta,
+    const gfx::Vector2dF& outer_delta,
+    const gfx::Vector2dF& elastic_overscroll_delta,
+    float page_scale,
+    float top_controls_delta) {
+  DCHECK(use_threaded_layer_tree_host_);
+  compositor_state_deserializer_->ApplyViewportDeltas(
+      inner_delta, outer_delta, elastic_overscroll_delta, page_scale,
+      top_controls_delta);
 }
 
 void BlimpCompositor::RequestNewCompositorFrameSink() {
   DCHECK(!surface_factory_);
   DCHECK(!compositor_frame_sink_request_pending_);
 
   compositor_frame_sink_request_pending_ = true;
   GetEmbedderDeps()->GetContextProviders(
       base::Bind(&BlimpCompositor::OnContextProvidersCreated,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BlimpCompositor::DidInitializeCompositorFrameSink() {
   compositor_frame_sink_request_pending_ = false;
 }
 
 void BlimpCompositor::DidCommitAndDrawFrame() {
   BlimpStats::GetInstance()->Add(BlimpStats::COMMIT, 1);
 
+  // TODO(khushalsagar): Fix before landing, the code here assumes that every

[aelias_OOO_until_Jul13, 2016/10/28 03:40:19]

Could you also upload this fix for code review since it seems like it'll be
nontrivial?  Might make sense to split into separate patch for landing before
this one moving the readback to use SwapPromise, since I think it's isolatable.

[Khushal, 2016/10/28 04:11:17]

Actually the problem is interesting here. If we want to use SwapPromises for
this, it has to be using a commit to push them to impl, which is easy for the
new code path where we use a threaded compositor on the client. You can have a
local commit at any time.

But the old code keeps the split point in the middle of Proxy, the main thread
on the client is as good as dead, and you can't do local commits. They land
here: https://cs.chromium.org/chromium/src/cc/trees/remote_channel_impl.cc?l=190
One of the reasons being that if a main frame request had been sent to the
server, the next commit has to include resolution of deltas. :P

So I think one way to address is to structure the BlimpCompositor API correctly
so the call to BlimpCompositor specifies that it is a copy request for the
current main frame. And then keep this outstanding_commits_ logic for the old
path, and use SwapPromises for the updated code path.

Doing that in a separate patch sounds like a good idea.

[Khushal, 2016/10/28 07:43:56]

I have uploaded a patch for fixing this separately.
https://codereview.chromium.org/2459823003/

+  // commit came from the engine. :X.
+  return;
+
   DCHECK_GT(outstanding_commits_, 0U);
   outstanding_commits_--;
 
   CheckPendingCommitCounts(false /* flush */);
 }
 
 void BlimpCompositor::SetProtoReceiver(ProtoReceiver* receiver) {
   DCHECK(!use_threaded_layer_tree_host_);
   remote_proto_channel_receiver_ = receiver;
 }
 
 void BlimpCompositor::SendCompositorProto(
     const cc::proto::CompositorMessage& proto) {
   DCHECK(!use_threaded_layer_tree_host_);
   client_->SendCompositorMessage(proto);
 }
 
 void BlimpCompositor::OnCompositorMessageReceived(
     std::unique_ptr<cc::proto::CompositorMessage> message) {
   if (message->has_to_impl()) {
     HandleCompositorMessageToImpl(std::move(message));
     return;
   }
 
   DCHECK(use_threaded_layer_tree_host_);
-  DCHECK(message->has_layer_tree_host())
-      << "The engine only sends frame updates";
-  DCHECK(!pending_frame_update_)
-      << "We should have only a single frame in flight";
+  cc::proto::CompositorMessage* message_received = message.get();
 
-  UMA_HISTOGRAM_MEMORY_KB("Blimp.Compositor.CommitSizeKb",
-                          (float)message->ByteSize() / 1024);
-  pending_frame_update_ = std::move(message);
-  outstanding_commits_++;
-  host_->SetNeedsAnimate();
+  if (message_received->has_layer_tree_host()) {
+    DCHECK(!pending_frame_update_)
+        << "We should have only a single frame in flight";
+
+    UMA_HISTOGRAM_MEMORY_KB("Blimp.Compositor.CommitSizeKb",
+                            (float)message->ByteSize() / 1024);
+    pending_frame_update_ = std::move(message);
+    outstanding_commits_++;
+    host_->SetNeedsAnimate();
+  }
+
+  if (message_received->client_state_update_ack()) {
+    DCHECK(client_state_update_ack_pending_);
+
+    client_state_update_ack_pending_ = false;
+    compositor_state_deserializer_->DidApplyStateUpdatesOnEngine();
+
+    // If there are any updates that we have queued because we were waiting for
+    // an ack, send them now.
+    FlushClientState();
+  }
 }
 
 void BlimpCompositor::HandleCompositorMessageToImpl(
     std::unique_ptr<cc::proto::CompositorMessage> message) {
   DCHECK(!use_threaded_layer_tree_host_);
   DCHECK(message->has_to_impl());
 
   const cc::proto::CompositorMessageToImpl to_impl_proto = message->to_impl();
   DCHECK(to_impl_proto.has_message_type());
 
@@ skipping 118 matching lines @@
 void BlimpCompositor::ReturnResources(
     const cc::ReturnedResourceArray& resources) {
   DCHECK(surface_factory_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(
           &BlimpCompositorFrameSinkProxyClient::ReclaimCompositorResources,
           proxy_client_, resources));
 }
 
-bool BlimpCompositor::ShouldRetainClientScroll(
-    int engine_layer_id,
-    const gfx::ScrollOffset& new_offset) {
-  // TODO(khushalsagar): Update when adding scroll/scale sync. See
-  // crbug.com/648442.
-  return true;
+void BlimpCompositor::DidUpdateLocalState() {
+  client_state_dirty_ = true;
 }
 
-bool BlimpCompositor::ShouldRetainClientPageScale(float new_page_scale) {
-  // TODO(khushalsagar): Update when adding scroll/scale sync. See
-  // crbug.com/648442.
-  return true;
-}
+void BlimpCompositor::FlushClientState() {
+  // If the client state has not been modified, we don't need to send an update.
+  if (!client_state_dirty_)
+    return;
 
-void BlimpCompositor::LayerScrolled(int engine_layer_id) {
-  DCHECK(use_threaded_layer_tree_host_);
+  // If we had sent an update and an ack for it is still pending, we can't send
+  // another update till the ack is received.
+  if (client_state_update_ack_pending_)
+    return;
+
+  cc::proto::CompositorMessage message;
+  message.set_frame_ack(false);
+  compositor_state_deserializer_->PullClientStateUpdate(
+      message.mutable_client_state_update());
+
+  client_state_dirty_ = false;
+  client_state_update_ack_pending_ = true;
+  client_->SendCompositorMessage(message);
 }
 
 CompositorDependencies* BlimpCompositor::GetEmbedderDeps() {
   return compositor_dependencies_->GetEmbedderDependencies();
 }
 
 void BlimpCompositor::DestroyDelegatedContent() {
   if (local_frame_id_.is_null())
     return;
 
@@ skipping 65 matching lines @@
       it->second.Run();
       it = pending_commit_trackers_.erase(it);
     } else {
       ++it;
     }
   }
 }
 
 }  // namespace client
 }  // namespace blimp