Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink

components/exo/surface.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 "components/exo/surface.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
@@ skipping 123 matching lines @@
     if (window->parent())
       aura::Window::ConvertPointToTarget(window->parent(), window,
                                          &local_point);
     return surface->HitTestRect(gfx::Rect(local_point, gfx::Size(1, 1)));
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CustomWindowTargeter);
 };
 
-void SatisfyCallback(cc::SurfaceManager* manager,
+void SatisfyCallback(cc::mojom::MojoCompositorFrameSink* compositor_frame_sink,
                      const cc::SurfaceSequence& sequence) {
-  std::vector<uint32_t> sequences;
-  sequences.push_back(sequence.sequence);
-  manager->DidSatisfySequences(sequence.frame_sink_id, &sequences);
+  compositor_frame_sink->Satisfy(sequence);
 }
 
-void RequireCallback(cc::SurfaceManager* manager,
+void RequireCallback(cc::mojom::MojoCompositorFrameSink* compositor_frame_sink,
                      const cc::SurfaceId& id,
                      const cc::SurfaceSequence& sequence) {
-  cc::Surface* surface = manager->GetSurfaceForId(id);
-  if (!surface) {
-    LOG(ERROR) << "Attempting to require callback on nonexistent surface";
-    return;
-  }
-  surface->AddDestructionDependency(sequence);
+  compositor_frame_sink->Require(id.local_frame_id(), sequence);
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
-// SurfaceFactoryOwner, public:
-
-SurfaceFactoryOwner::SurfaceFactoryOwner() {}
-
-////////////////////////////////////////////////////////////////////////////////
-// cc::SurfaceFactoryClient overrides:
-
-void SurfaceFactoryOwner::ReturnResources(
-    const cc::ReturnedResourceArray& resources) {
-  scoped_refptr<SurfaceFactoryOwner> holder(this);
-  for (auto& resource : resources) {
-    auto it = release_callbacks_.find(resource.id);
-    DCHECK(it != release_callbacks_.end());
-    it->second.second->Run(resource.sync_token, resource.lost);
-    release_callbacks_.erase(it);
-  }
-}
-
-void SurfaceFactoryOwner::WillDrawSurface(const cc::LocalFrameId& id,
-                                          const gfx::Rect& damage_rect) {
-  if (surface_)
-    surface_->WillDraw();
-}
-
-void SurfaceFactoryOwner::SetBeginFrameSource(
-    cc::BeginFrameSource* begin_frame_source) {
-  if (surface_)
-    surface_->SetBeginFrameSource(begin_frame_source);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// SurfaceFactoryOwner, private:
-
-SurfaceFactoryOwner::~SurfaceFactoryOwner() {
-  if (surface_factory_->manager())
-    surface_factory_->manager()->InvalidateFrameSinkId(frame_sink_id_);
-}
-
-////////////////////////////////////////////////////////////////////////////////
 // Surface, public:
 
 Surface::Surface()
     : window_(new aura::Window(new CustomWindowDelegate(this))),
+      frame_sink_id_(
+          aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId()),
       surface_manager_(
           aura::Env::GetInstance()->context_factory()->GetSurfaceManager()),
-      factory_owner_(new SurfaceFactoryOwner) {
+      weak_ptr_factory_(this) {
+  cc::mojom::MojoCompositorFrameSinkClientPtr pholder;

[reveman, 2016/12/07 00:46:57]

pholder? frame_sink_holder_ptr instead?

[Alex Z., 2016/12/07 20:09:36]

Done.

+  cc::mojom::MojoCompositorFrameSinkPtr mojo_compositor_frame_sink_ptr;

[reveman, 2016/12/07 00:46:57]

just "frame_sink_ptr" ?

[Alex Z., 2016/12/07 20:09:36]

Done.

+  cc::mojom::MojoCompositorFrameSinkClientRequest holder_request =

[reveman, 2016/12/07 00:46:57]

s/holder_request/frame_sink_client_request/ ?

[Alex Z., 2016/12/07 20:09:36]

Done.

+      mojo::GetProxy(&pholder);
+  CompositorFrameSink::Create(frame_sink_id_, surface_manager_,
+                              std::move(pholder),
+                              mojo::GetProxy(&mojo_compositor_frame_sink_ptr));
+  compositor_frame_sink_holder_ = new CompositorFrameSinkHolder(
+      std::move(mojo_compositor_frame_sink_ptr), weak_ptr_factory_.GetWeakPtr(),
+      std::move(holder_request));
   window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
   window_->SetName("ExoSurface");
   window_->SetProperty(kSurfaceKey, this);
   window_->Init(ui::LAYER_SOLID_COLOR);
   window_->SetEventTargeter(base::WrapUnique(new CustomWindowTargeter));
   window_->set_owned_by_parent(false);
   window_->AddObserver(this);
-  factory_owner_->surface_ = this;
-  factory_owner_->frame_sink_id_ =
-      aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId();
-  factory_owner_->id_allocator_.reset(new cc::SurfaceIdAllocator());
-  surface_manager_->RegisterFrameSinkId(factory_owner_->frame_sink_id_);
-  surface_manager_->RegisterSurfaceFactoryClient(factory_owner_->frame_sink_id_,
-                                                 factory_owner_.get());
-  factory_owner_->surface_factory_.reset(new cc::SurfaceFactory(
-      factory_owner_->frame_sink_id_, surface_manager_, factory_owner_.get()));
   aura::Env::GetInstance()->context_factory()->AddObserver(this);
 }
 
 Surface::~Surface() {
   aura::Env::GetInstance()->context_factory()->RemoveObserver(this);
   for (SurfaceObserver& observer : observers_)
     observer.OnSurfaceDestroying(this);
 
   window_->RemoveObserver(this);
   window_->layer()->SetShowSolidColorContent();
 
-  factory_owner_->surface_ = nullptr;
+  frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
+  compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_);
+  compositor_frame_sink_holder_->CancelFrameCallbacks();
 
-  // Call pending frame callbacks with a null frame time to indicate that they
-  // have been cancelled.
-  frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
-  active_frame_callbacks_.splice(active_frame_callbacks_.end(),
-                                 frame_callbacks_);
-  for (const auto& frame_callback : active_frame_callbacks_)
-    frame_callback.Run(base::TimeTicks());
-  if (begin_frame_source_ && needs_begin_frame_)
-    begin_frame_source_->RemoveObserver(this);
-
-  factory_owner_->surface_factory_->EvictSurface();
-
-  surface_manager_->UnregisterSurfaceFactoryClient(
-      factory_owner_->frame_sink_id_);
+  compositor_frame_sink_holder_->GetCompositorFrameSink()->EvictFrame();
 }
 
 // static
 Surface* Surface::AsSurface(const aura::Window* window) {
   return window->GetProperty(kSurfaceKey);
 }
 
 cc::SurfaceId Surface::GetSurfaceId() const {
-  return cc::SurfaceId(factory_owner_->frame_sink_id_, local_frame_id_);
+  return cc::SurfaceId(frame_sink_id_, local_frame_id_);
 }
 
 void Surface::Attach(Buffer* buffer) {
   TRACE_EVENT1("exo", "Surface::Attach", "buffer",
                buffer ? buffer->GetSize().ToString() : "null");
 
   has_pending_contents_ = true;
   pending_buffer_.Reset(buffer ? buffer->AsWeakPtr() : base::WeakPtr<Buffer>());
 }
 
@@ skipping 196 matching lines @@
     has_pending_contents_ = false;
 
     current_buffer_ = std::move(pending_buffer_);
 
     UpdateResource(true);
   }
 
   cc::LocalFrameId old_local_frame_id = local_frame_id_;
   if (needs_commit_to_new_surface_ || !local_frame_id_.is_valid()) {
     needs_commit_to_new_surface_ = false;
-    local_frame_id_ = factory_owner_->id_allocator_->GenerateId();
+    local_frame_id_ = id_allocator_.GenerateId();
   }
 
   UpdateSurface(true);
 
   if (old_local_frame_id != local_frame_id_) {
+    needs_commit_to_new_surface_ = false;

[jbauman, 2016/12/06 23:44:49]

Shouldn't this always be false here already?

[reveman, 2016/12/07 00:46:57]

why was this added here?

[Alex Z., 2016/12/07 20:09:36]

Done.

     float contents_surface_to_layer_scale = 1.0;
     // The bounds must be updated before switching to the new surface, because
     // the layer may be mirrored, in which case a surface change causes the
     // mirror layer to update its surface using the latest bounds.
     window_->layer()->SetBounds(
         gfx::Rect(window_->layer()->bounds().origin(), content_size_));
     window_->layer()->SetShowSurface(
-        cc::SurfaceId(factory_owner_->frame_sink_id_, local_frame_id_),
-        base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
-        base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
+        cc::SurfaceId(frame_sink_id_, local_frame_id_),
+        base::Bind(
+            &SatisfyCallback,
+            base::Unretained(
+                compositor_frame_sink_holder_->GetCompositorFrameSink())),
+        base::Bind(

[jbauman, 2016/12/06 23:44:49]

Are we sure these lifetimes are correct? If the exo::Surface goes away and it's
not holding onto any resources (because no buffer's bound), then the layer could
reference a CompositorFrameSinkHolder until the next browser compositor commit
happens even though it's gone away.

I think a similar problem could happen now, but it'd just print "Attempting to
require callback on nonexistent surface" and destroy a surface with no contents
(which therefore doesn't draw anything), and not be a use after free, so that's
not too bad.

[Fady Samuel, 2016/12/07 14:34:04]

Makes sense. I think it sounds like we want to hold onto a weakptr to
CompositorFrameSinkHolder in these callbacks.

[Alex Z., 2016/12/07 20:09:36]

Done.

+            &RequireCallback,
+            base::Unretained(
+                compositor_frame_sink_holder_->GetCompositorFrameSink())),
         content_size_, contents_surface_to_layer_scale, content_size_);
     window_->layer()->SetFillsBoundsOpaquely(
         state_.blend_mode == SkBlendMode::kSrc ||
         state_.opaque_region.contains(
             gfx::RectToSkIRect(gfx::Rect(content_size_))));
   }
 
   // Reset damage.
   pending_damage_.setEmpty();
 
-  DCHECK(!current_resource_.id ||
-         factory_owner_->release_callbacks_.count(current_resource_.id));
+  DCHECK(
+      !current_resource_.id ||
+      compositor_frame_sink_holder_->HasReleaseCallbacks(current_resource_.id));
 
   // Move pending frame callbacks to the end of frame_callbacks_.
   frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
 
   // Synchronize window hierarchy. This will position and update the stacking
   // order of all sub-surfaces after committing all pending state of sub-surface
   // descendants.
   aura::Window* stacking_target = nullptr;
   for (auto& sub_surface_entry : pending_sub_surfaces_) {
     Surface* sub_surface = sub_surface_entry.first;
@@ skipping 84 matching lines @@
 }
 
 std::unique_ptr<base::trace_event::TracedValue> Surface::AsTracedValue() const {
   std::unique_ptr<base::trace_event::TracedValue> value(
       new base::trace_event::TracedValue());
   value->SetString("name", window_->layer()->name());
   return value;
 }
 
 void Surface::WillDraw() {
-  active_frame_callbacks_.splice(active_frame_callbacks_.end(),
-                                 frame_callbacks_);
-  UpdateNeedsBeginFrame();
-}
-
-void Surface::SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) {
-  if (begin_frame_source_ && needs_begin_frame_) {
-    begin_frame_source_->RemoveObserver(this);
-    needs_begin_frame_ = false;
-  }
-  begin_frame_source_ = begin_frame_source;
-  UpdateNeedsBeginFrame();
+  compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_);
 }
 
 void Surface::CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces() {
   if (HasLayerHierarchyChanged())
     SetSurfaceHierarchyNeedsCommitToNewSurfaces();
 }
 
 void Surface::OnLostResources() {
   if (!local_frame_id_.is_valid())
     return;
 
   UpdateResource(false);
   UpdateSurface(false);
 }
 
 void Surface::OnWindowAddedToRootWindow(aura::Window* window) {
-  window->layer()->GetCompositor()->AddFrameSink(
-      factory_owner_->frame_sink_id_);
+  window->layer()->GetCompositor()->AddFrameSink(frame_sink_id_);
 }
 
 void Surface::OnWindowRemovingFromRootWindow(aura::Window* window,
                                              aura::Window* new_root) {
-  window->layer()->GetCompositor()->RemoveFrameSink(
-      factory_owner_->frame_sink_id_);
-}
-
-void Surface::OnBeginFrame(const cc::BeginFrameArgs& args) {
-  while (!active_frame_callbacks_.empty()) {
-    active_frame_callbacks_.front().Run(args.frame_time);
-    active_frame_callbacks_.pop_front();
-  }
-  last_begin_frame_args_ = args;
-}
-
-const cc::BeginFrameArgs& Surface::LastUsedBeginFrameArgs() const {
-  return last_begin_frame_args_;
+  window->layer()->GetCompositor()->RemoveFrameSink(frame_sink_id_);
 }
 
 Surface::State::State() : input_region(SkIRect::MakeLargest()) {}
 
 Surface::State::~State() = default;
 
 bool Surface::State::operator==(const State& other) {
   return (other.crop == crop && alpha == other.alpha &&
           other.blend_mode == blend_mode && other.viewport == viewport &&
           other.opaque_region == opaque_region &&
@@ skipping 67 matching lines @@
     resource.id = next_resource_id_++;
     resource.format = cc::RGBA_8888;
     resource.filter =
         texture_mailbox.nearest_neighbor() ? GL_NEAREST : GL_LINEAR;
     resource.size = texture_mailbox.size_in_pixels();
     resource.mailbox_holder = gpu::MailboxHolder(texture_mailbox.mailbox(),
                                                  texture_mailbox.sync_token(),
                                                  texture_mailbox.target());
     resource.is_overlay_candidate = texture_mailbox.is_overlay_candidate();
 
-    factory_owner_->release_callbacks_[resource.id] = std::make_pair(
-        factory_owner_, std::move(texture_mailbox_release_callback));
+    compositor_frame_sink_holder_->AddResourceReleaseCallback(
+        resource.id, std::move(texture_mailbox_release_callback));
     current_resource_ = resource;
   } else {
     current_resource_.id = 0;
     current_resource_.size = gfx::Size();
   }
 }
 
 void Surface::UpdateSurface(bool full_damage) {
   gfx::Size buffer_size = current_resource_.size;
   gfx::SizeF scaled_buffer_size(
@@ skipping 68 matching lines @@
         texture_quad->set_resource_size_in_pixels(current_resource_.size);
       frame.resource_list.push_back(current_resource_);
     }
   } else {
     cc::SolidColorDrawQuad* solid_quad =
         render_pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
     solid_quad->SetNew(quad_state, quad_rect, quad_rect, SK_ColorBLACK, false);
   }
 
   frame.render_pass_list.push_back(std::move(render_pass));
-
-  factory_owner_->surface_factory_->SubmitCompositorFrame(
-      local_frame_id_, std::move(frame), cc::SurfaceFactory::DrawCallback());
-}
-
-void Surface::UpdateNeedsBeginFrame() {
-  if (!begin_frame_source_)
-    return;
-
-  bool needs_begin_frame = !active_frame_callbacks_.empty();
-  if (needs_begin_frame == needs_begin_frame_)
-    return;
-
-  needs_begin_frame_ = needs_begin_frame;
-  if (needs_begin_frame)
-    begin_frame_source_->AddObserver(this);
-  else
-    begin_frame_source_->RemoveObserver(this);
+  compositor_frame_sink_holder_->GetCompositorFrameSink()
+      ->SubmitCompositorFrame(local_frame_id_, std::move(frame));

[jbauman, 2016/12/06 23:44:49]

Does this happen synchronously, or does it post a task that's executed
asynchronously? If the latter, I'm worried about the ordering of this with
respect to the browser compositor commit and draw.

Also, if it's asynchronous I think we have a problem if there are two surfaces
in a synchronized subtree that need to be committed together and there are no
layer hierarchy changes. Both surface content changes need to be committed
atomically so we don't have a case where one is updated and the other isn't.

[Fady Samuel, 2016/12/07 13:54:32]

This is a problem. All submits are async, and will eventually be cross-process
IPCs. You're saying there's an expectation for multiple surfaces to submit
CompositorFrames atomically? The system the surfaces system uses is to allocate
a new surface ID to do this. I think we're already doing this if
needs_commit_to_new_surface_?

 }
 
 int64_t Surface::SetPropertyInternal(const void* key,
                                      const char* name,
                                      PropertyDeallocator deallocator,
                                      int64_t value,
                                      int64_t default_value) {
   int64_t old = GetPropertyInternal(key, default_value);
   if (value == default_value) {
     prop_map_.erase(key);
   } else {
     Value prop_value;
     prop_value.name = name;
     prop_value.value = value;
     prop_value.deallocator = deallocator;
     prop_map_[key] = prop_value;
   }
   return old;
 }
 
 int64_t Surface::GetPropertyInternal(const void* key,
                                      int64_t default_value) const {
   std::map<const void*, Value>::const_iterator iter = prop_map_.find(key);
   if (iter == prop_map_.end())
     return default_value;
   return iter->second.value;
 }
 
 }  // namespace exo