exo::CompositorFrameSinkHolder's release callbacks hold ref

components/exo/buffer.h

Index: components/exo/buffer.h
diff --git a/components/exo/buffer.h b/components/exo/buffer.h
index e7b5b605c45e423e78ce5ce91cf92ee346110338..8a83b7ecb1d0b11458c97e071086e2e31c03b077 100644
--- a/components/exo/buffer.h
+++ b/components/exo/buffer.h
@@ -29,6 +29,8 @@ 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.
@@ -56,7 +58,8 @@ class Buffer : public base::SupportsWeakPtr<Buffer> {
   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();
@@ -82,12 +85,16 @@ class Buffer : public base::SupportsWeakPtr<Buffer> {
 
   // 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_;