exo::CompositorFrameSinkHolder's release callbacks hold ref

components/exo/buffer.h

 // 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.
 
 #ifndef COMPONENTS_EXO_BUFFER_H_
 #define COMPONENTS_EXO_BUFFER_H_
 
 #include <memory>
 
 #include "base/callback.h"
@@ skipping 11 matching lines @@
 class SingleReleaseCallback;
 class TextureMailbox;
 }
 
 namespace gfx {
 class GpuMemoryBuffer;
 }
 
 namespace exo {
 
+class CompositorFrameSinkHolder;
+
 // This class provides the content for a Surface. The mechanism by which a
 // client provides and updates the contents is the responsibility of the client
 // and not defined as part of this class.
 class Buffer : public base::SupportsWeakPtr<Buffer> {
  public:
   explicit Buffer(std::unique_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer);
   Buffer(std::unique_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer,
          unsigned texture_target,
          unsigned query_type,
          bool use_zero_copy,
          bool is_overlay_candidate);
   ~Buffer();
 
   // Set the callback to run when the buffer is no longer used by the
   // compositor. The client is free to re-use or destroy this buffer and
   // its backing storage after this has been called.
   void set_release_callback(const base::Closure& release_callback) {
     release_callback_ = release_callback;
   }
 
   // This function can be used to acquire a texture mailbox for the contents of
   // buffer. Returns a release callback on success. The release callback should
   // be called before a new texture mailbox can be acquired unless
   // |non_client_usage| is true.
   std::unique_ptr<cc::SingleReleaseCallback> ProduceTextureMailbox(
       cc::TextureMailbox* mailbox,
       bool secure_output_only,
-      bool client_usage);
+      bool client_usage,
+      scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder);

[reveman, 2016/12/21 20:05:42]

nit: CompositorFrameSinkHolder* compositor_frame_sink_holder

[Alex Z., 2016/12/22 16:33:25]

Done.

 
   // This should be called when the buffer is attached to a Surface.
   void OnAttach();
 
   // This should be called when the buffer is detached from a surface.
   void OnDetach();
 
   // Returns the size of the buffer.
   gfx::Size GetSize() const;
 
   // Returns a trace value representing the state of the buffer.
   std::unique_ptr<base::trace_event::TracedValue> AsTracedValue() const;
 
  private:
   class Texture;
 
   // Decrements the use count of buffer and notifies the client that buffer
   // as been released if it reached 0.
   void Release();
 
   // Runs the release callback if the buffer isn't attached or in use.
   void CheckReleaseCallback();
 
   // This is used by ProduceTextureMailbox() to produce a release callback
   // that releases a texture so it can be destroyed or reused.
-  void ReleaseTexture(std::unique_ptr<Texture> texture);
+  void ReleaseTexture(
+      std::unique_ptr<Texture> texture,
+      scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder);

[reveman, 2016/12/21 20:05:42]

these Release(Contents)Texture functions don't actually use the sink_holder so
can your wrap the call to these functions in a function that handles the
sink_holder reference part? e.g. ReleaseTextureAndCompositorFrameSinkHolder

[Alex Z., 2016/12/21 21:53:20]

I'm not sure if I understand. SinkHolder doesn't get explicitly released and
destroyed. Do you want another function
(ReleaseTextureAndCompositorFrameSinkHolder()) that takes a texture and a ref to
SinkHolder to hold onto the ref and pass the texture to ReleaseTexture()? The
function still doesn't "do" anything to release the SinkHolder.

[reveman, 2016/12/21 22:46:00]

Yes, it's confusing that a function named "ReleaseTexture" for some reason takes
a sink holder reference but doesn't use it. If you add a function that is called
ReleaseTextureAndCompositorFrameSinkHolder that calls ReleaseTexture and then
implicitly or explicitly drops the sink holder reference then that makes it more
clear that this reference being kept alive is critical. In the current patch, it
just looks like we're passing an unnecessary argument and someone might think
it's a good idea to clean that up by removing that argument.

[Alex Z., 2016/12/22 16:33:25]

Done.

 
   // This is used by ProduceTextureMailbox() to produce a release callback
   // that releases the buffer contents referenced by a texture before the
   // texture is destroyed or reused.
-  void ReleaseContentsTexture(std::unique_ptr<Texture> texture);
+  void ReleaseContentsTexture(
+      std::unique_ptr<Texture> texture,
+      scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder);
 
   // The GPU memory buffer that contains the contents of this buffer.
   std::unique_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer_;
 
   // Texture target that must be used when creating a texture for buffer.
   const unsigned texture_target_;
 
   // Query type that must be used when releasing buffer from a texture.
   const unsigned query_type_;
 
@@ skipping 21 matching lines @@
 
   // The client release callback.
   base::Closure release_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Buffer);
 };
 
 }  // namespace exo
 
 #endif  // COMPONENTS_EXO_BUFFER_H_