Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink

components/exo/exo_compositor_frame_sink.h

+// Copyright 2016 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 <list>
+#include <memory>
+#include <set>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "cc/ipc/compositor_frame.mojom.h"
+#include "cc/ipc/mojo_compositor_frame_sink.mojom.h"
+#include "cc/resources/transferable_resource.h"
+#include "cc/scheduler/begin_frame_source.h"
+#include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_factory_client.h"
+#include "cc/surfaces/surface_id_allocator.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "third_party/skia/include/core/SkRegion.h"
+#include "third_party/skia/include/core/SkXfermode.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
+#include "ui/gfx/geometry/rect.h"
+
+namespace cc {
+class SurfaceFactory;
+}
+
+namespace exo {
+
+// This class owns the SurfaceFactory and keeps track of references to the
+// contents of Buffers. It's keeped alive by references from
+// release_callbacks_. It's destroyed when its owning Surface is destroyed and
+// the last outstanding release callback is called.
+class ExoCompositorFrameSink : public base::RefCounted<ExoCompositorFrameSink>,
+                               public cc::SurfaceFactoryClient,
+                               public cc::BeginFrameObserver {
+ public:
+  ExoCompositorFrameSink(const cc::FrameSinkId& frame_sink_id,
+                         cc::SurfaceManager* surface_manager);
+
+  // cc::SurfaceFactoryClient:

[Fady Samuel, 2016/11/16 15:02:00]

Make all these overrides private to indicate that these are implementation
details and not part of the public API.

[reveman, 2016/11/16 17:43:30]

I'm not a fan of changing access to functions when overriding them as anyone
with a pointer to the base class will still be able to access it and that's
confusing, I think.

[Alex Z., 2016/11/16 19:56:16]

Done.

+  void ReturnResources(const cc::ReturnedResourceArray& resources) override;
+  void WillDrawSurface(const cc::LocalFrameId& id,
+                       const gfx::Rect& damage_rect) override;
+  void SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) override;
+
+  // To be cc::mojom::MojoCompositorFrameSink:
+  void SubmitCompositorFrame(cc::CompositorFrame frame);
+
+  // cc::BeginFrameObserver:

[Fady Samuel, 2016/11/16 15:02:00]

Make all these overrides private to indicate that these are implementation
details and not part of the public API.

[Alex Z., 2016/11/16 19:56:16]

Done.

+  void OnBeginFrame(const cc::BeginFrameArgs& args) override;
+  const cc::BeginFrameArgs& LastUsedBeginFrameArgs() const override;
+  void OnBeginFrameSourcePausedChanged(bool paused) override;
+
+  cc::SurfaceManager* surface_manager() { return surface_manager_; }

[Fady Samuel, 2016/11/16 15:02:00]

It would be nice to not read back things from ExoCompositorFrameSink. The thing
that created ExoCompositorFrameSink knows about SurfaceManager right, so we can
use it directly for now.

[Alex Z., 2016/11/16 19:56:16]

Done.

+
+ private:
+  friend class base::RefCounted<ExoCompositorFrameSink>;
+  friend class Surface;

[Fady Samuel, 2016/11/16 15:02:00]

Remove this friend.

[Alex Z., 2016/11/16 19:56:16]

Removing this friend prevents exo::Surface from accessing
ExoCompositorFrameSink::release_callbacks_. I added
ExoCompositorFrameSink::release_callbacks() as a temporary workaround.

+
+  ~ExoCompositorFrameSink() override;
+  void UpdateNeedsBeginFrameInternal();
+  void DidRecieveCompositorFrameAck();
+
+  std::map<int,
+           std::pair<scoped_refptr<ExoCompositorFrameSink>,
+                     std::unique_ptr<cc::SingleReleaseCallback>>>
+      release_callbacks_;

[Fady Samuel, 2016/11/16 15:02:00]

I really don't understand the purpose of this.

[reveman, 2016/11/16 17:43:30]

jbauman can the explain the details of this map better than me but the general
difference between exo and other display compositor clients in chrome is that
exo clients require additional synchronization requirements as they have direct
access to the gpu using a different channel than the gpu process in chrome. We
need to use real gpu fences to determine when a buffer can be reused by a client
and these release callbacks are related to that.

+  const cc::FrameSinkId frame_sink_id_;
+  cc::LocalFrameId local_frame_id_;
+  cc::SurfaceIdAllocator id_allocator_;
+
+  cc::SurfaceManager* surface_manager_;
+  cc::SurfaceFactory surface_factory_;
+
+  int ack_pending_count_ = 0;
+
+  cc::BeginFrameSource* begin_frame_source_ = nullptr;
+
+  gfx::Size last_submitted_frame_size_;
+
+  cc::BeginFrameArgs last_begin_frame_args_;
+
+  bool needs_begin_frame_ = false;
+  bool added_frame_observer_ = false;
+};

[Fady Samuel, 2016/11/16 15:02:00]

DISALLOW_COPY_AND_ASSIGN(ExoCompositorFrameSink);

[Alex Z., 2016/11/16 19:56:16]

Done.

+
+}  // namespace exo