Implement service-side surface synchronization

cc/surfaces/surface_dependency_tracker.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.

[vmpstr, 2017/02/07 22:46:19]

nit: 2017

[Fady Samuel, 2017/02/08 00:52:50]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "cc/surfaces/surface_dependency_tracker.h"
+
+#include "cc/surfaces/surface.h"
+#include "cc/surfaces/surface_info.h"
+#include "cc/surfaces/surface_manager.h"
+
+namespace cc {
+
+namespace {
+static constexpr uint32_t MAX_BEGIN_FRAME_COUNT = 4;

[vmpstr, 2017/02/07 22:46:19]

nit: no need for static or no need for namespace {}
nit: kMaxBeginFrameCount

[Fady Samuel, 2017/02/08 00:52:49]

Done.

+}
+
+SurfaceDependencyTracker::SurfaceDependencyTracker(
+    SurfaceManager* surface_manager,
+    BeginFrameSource* begin_frame_source)
+    : surface_manager_(surface_manager),
+      begin_frame_source_(begin_frame_source) {
+  surface_manager_->AddObserver(this);
+}
+
+SurfaceDependencyTracker::~SurfaceDependencyTracker() {
+  surface_manager_->RemoveObserver(this);
+}
+
+void SurfaceDependencyTracker::RequestSurfaceResolution(
+    Surface* pending_surface) {
+  if (!pending_surface->HasPendingFrame())

[vmpstr, 2017/02/07 22:46:20]

From reading the header comment, it seems that the surface wants to be informed
of something. Is that a strong requirement? If so I think you need to call
something on it before returning :P

Either that or this should be a DCHECK?

[Fady Samuel, 2017/02/08 00:52:49]

Yes, this should be a DCHECK. Also renamed instances of pending_surface to
surface because pending_surface sounds weird here because the surface may have
an active frame too.

+    return;
+  const CompositorFrame& pending_frame = pending_surface->GetPendingFrame();
+  bool needs_begin_frame = pending_frame.metadata.respect_deadline;
+
+  // Referenced surface IDs that aren't currently known to the surface manager

[vmpstr, 2017/02/07 22:46:19]

I guess I'm misunderstanding a bit, but is

"known to the surface manager" imply both surface is in there _and_ it has an
active frame? Specifically, if it _doesn't_ have an active frame, does that mean
the surface is not known?

[Fady Samuel, 2017/02/08 00:52:50]

Yes, I've added an extra bit to clarify. Thanks!

+  // block this frame.
+  for (const SurfaceId& surface_id :
+       pending_frame.metadata.referenced_surfaces) {
+    Surface* surface = surface_manager_->GetSurfaceForId(surface_id);
+    if (!surface || !surface->HasActiveFrame())
+      blocking_surfaces_[surface_id].insert(pending_surface);
+  }
+
+  if (!pending_surfaces_.count(pending_surface)) {
+    pending_surface->AddObserver(this);
+    pending_surfaces_.insert(pending_surface);
+  }
+
+  if (needs_begin_frame && !has_deadline_) {
+    begin_frame_source_->AddObserver(this);
+    has_deadline_ = true;
+  }
+}
+
+void SurfaceDependencyTracker::OnBeginFrame(const BeginFrameArgs& args) {
+  // TODO(fsamuel, kylechar): We have a single global deadline here. We should
+  // scope deadlines to surface subtrees. We cannot do that until

[vmpstr, 2017/02/07 22:46:19]

Can you file a bug for this and reference it here, please?

[Fady Samuel, 2017/02/08 00:52:50]

Done.

+  // SurfaceReferences have been fully implemented.
+  last_begin_frame_args_ = args;
+  // Nothing to do if we haven't hit a deadline yet.
+  if (++begin_frame_count_ != MAX_BEGIN_FRAME_COUNT)
+    return;
+
+  // Activate all surfaces that respect the deadline.
+  PendingSurfaceSet pending_surfaces(pending_surfaces_);
+  for (Surface* pending_surface : pending_surfaces)
+    pending_surface->ActivatePendingFrameForDeadline();
+
+  if (has_deadline_) {

[vmpstr, 2017/02/07 22:46:19]

Instead of adding/removing |this| from the observers, do you think it's better
to just always observe but early out if it has no deadline?

Also, how is it possible that we're in this function and has_deadline_ is false?

[Fady Samuel, 2017/02/08 00:52:49]

I agree, that's easier. Also, eww, this looks like cruft from my prototype.

+    begin_frame_source_->RemoveObserver(this);
+    has_deadline_ = false;
+  }
+  begin_frame_count_ = 0;
+}
+
+const BeginFrameArgs& SurfaceDependencyTracker::LastUsedBeginFrameArgs() const {
+  return last_begin_frame_args_;
+}
+
+void SurfaceDependencyTracker::OnBeginFrameSourcePausedChanged(bool paused) {}
+
+void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* pending_surface) {
+  if (!pending_surface->HasPendingFrame())

[vmpstr, 2017/02/07 22:46:20]

Comment please

[Fady Samuel, 2017/02/08 00:52:49]

Done.

+    return;
+
+  const CompositorFrame& pending_frame = pending_surface->GetPendingFrame();
+
+  DCHECK(!pending_frame.metadata.referenced_surfaces.empty());
+
+  for (const SurfaceId& surface_id :
+       pending_frame.metadata.referenced_surfaces) {
+    auto it = blocking_surfaces_.find(surface_id);
+    if (it != blocking_surfaces_.end()) {

[vmpstr, 2017/02/07 22:46:19]

if (it == blocking_surface_.end())
  continue;
...

[Fady Samuel, 2017/02/08 00:52:49]

Done.

+      auto& pending_surface_set = it->second;
+      auto pending_surface_it = pending_surface_set.find(pending_surface);
+      if (pending_surface_it != pending_surface_set.end()) {
+        pending_surface_set.erase(pending_surface);
+        if (pending_surface_set.empty())
+          blocking_surfaces_.erase(surface_id);
+      }
+    }
+  }
+
+  if (has_deadline_ && blocking_surfaces_.empty()) {
+    begin_frame_source_->RemoveObserver(this);
+    has_deadline_ = false;
+  }
+
+  pending_surfaces_.erase(pending_surface);
+  pending_surface->RemoveObserver(this);
+
+  // TODO(fsamuel): We should consider removing this surface as a dependency on

[vmpstr, 2017/02/07 22:46:19]

File a bug for this please and reference here (I know this is just an
exploratory TODO, but it's easier to track progress on crbug)

[Fady Samuel, 2017/02/08 00:52:49]

Done.

+  // other surfaces. This dependency will be removed when the deadline hits
+  // anyway but maybe we should try to actiate these frames early.
+}
+
+void SurfaceDependencyTracker::OnSurfaceActivated(Surface* pending_surface) {
+  pending_surface->RemoveObserver(this);
+  pending_surfaces_.erase(pending_surface);
+  ReportSurfaceIdAvailable(pending_surface->surface_id());
+}
+
+void SurfaceDependencyTracker::OnSurfaceChanged(
+    Surface* pending_surface,
+    const SurfaceDependencies& added_dependencies,
+    const SurfaceDependencies& removed_dependencies) {
+  // Update the |blocking_surfaces_| map with the changes in dependencies.
+  for (const SurfaceId& surface_id : added_dependencies)
+    blocking_surfaces_[surface_id].insert(pending_surface);
+
+  for (const SurfaceId& surface_id : removed_dependencies) {
+    blocking_surfaces_[surface_id].erase(pending_surface);

[vmpstr, 2017/02/07 22:46:19]

Do a find and use the iterator for both erase and checking if empty and erasing
(less lookups)

[Fady Samuel, 2017/02/08 00:52:49]

Done.

+    if (blocking_surfaces_[surface_id].empty())
+      blocking_surfaces_.erase(surface_id);
+  }
+
+  // If there are no more dependencies to resolve then we don't need to listen
+  // to BeginFrames.
+  if (blocking_surfaces_.empty() && has_deadline_) {
+    begin_frame_source_->RemoveObserver(this);
+    has_deadline_ = false;
+  }
+}
+
+// SurfaceObserver implementation:
+void SurfaceDependencyTracker::OnSurfaceCreated(
+    const SurfaceInfo& surface_info) {
+  // This is called when a Surface has an activated frame for the first time.
+  // SurfaceDependencyTracker only observes Surfaces that contain pending
+  // frames. SurfaceDependencyTracker becomes aware of CompositorFrames that
+  // activate immediately go through here.
+  ReportSurfaceIdAvailable(surface_info.id());
+}
+
+void SurfaceDependencyTracker::OnSurfaceDamaged(const SurfaceId& surface_id,
+                                                bool* changed) {}
+
+void SurfaceDependencyTracker::ReportSurfaceIdAvailable(
+    const SurfaceId& surface_id) {
+  auto it = blocking_surfaces_.find(surface_id);
+  if (it == blocking_surfaces_.end())
+    return;
+
+  // Unblock surfaces that depend on this pending surface.
+  PendingSurfaceSet blocked_pending_surface_set(it->second);
+  blocking_surfaces_.erase(it);
+  // If there are no more blockers in the system, then we no longer need to
+  // listen to BeginFrames.
+  if (has_deadline_ && blocking_surfaces_.empty()) {
+    begin_frame_source_->RemoveObserver(this);
+    has_deadline_ = false;
+  }
+  // Tell each surface about the availability of its blocker.
+  for (Surface* blocked_pending_surface : blocked_pending_surface_set)
+    blocked_pending_surface->ReportSurfaceIdAvailable(surface_id);
+}
+
+}  // namespace cc