exo::CompositorFrameSinkHolder's release callbacks hold ref

components/exo/buffer.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/buffer.h"
 
 #include <GLES2/gl2.h>
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "cc/output/context_provider.h"
 #include "cc/resources/single_release_callback.h"
 #include "cc/resources/texture_mailbox.h"
+#include "components/exo/compositor_frame_sink_holder.h"
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "ui/aura/env.h"
 #include "ui/compositor/compositor.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 namespace exo {
 namespace {
 
 // The amount of time before we wait for release queries using
@@ skipping 354 matching lines @@
                bool use_zero_copy,
                bool is_overlay_candidate)
     : gpu_memory_buffer_(std::move(gpu_memory_buffer)),
       texture_target_(texture_target),
       query_type_(query_type),
       use_zero_copy_(use_zero_copy),
       is_overlay_candidate_(is_overlay_candidate) {}
 
 Buffer::~Buffer() {}
 
 std::unique_ptr<cc::SingleReleaseCallback> Buffer::ProduceTextureMailbox(

[reveman, 2017/01/02 19:57:56]

It doesn't make sense for this function to return a release callback anymore.
Also doesn't make much sense to be producing a TextureMailbox anymore. Please
have this return a bool and a cc::TransferableResource instead. The buffer is
the instance that cares about the release callback and not the surface so this
buffer code is also where a release callback should be set for the sink holder.
Please change the signature to:

bool Buffer::ProduceTransferableResource(CompositorFrameSinkHolder*
compositor_frame_sink_holder,
                                         int resource_id,
                                         bool secure_output_only,
                                         bool client_usage,
                                         cc::TransferableResource* resource);

     cc::TextureMailbox* texture_mailbox,
     bool secure_output_only,
-    bool client_usage) {
+    bool client_usage,
+    CompositorFrameSinkHolder* compositor_frame_sink_holder) {
   DCHECK(attach_count_);
   DLOG_IF(WARNING, use_count_ && client_usage)
       << "Producing a texture mailbox for a buffer that has not been released";
 
   // Some clients think that they can reuse a buffer before it's released by
   // performing a fast blit into the buffer. This behavior is bad as it prevents
   // the client from knowing when the buffer is actually released (e.g. the
   // release notification for the previous use of buffer can arrive after the
   // buffer has been reused). We stop running the release callback when this
   // type of behavior is detected as having the buffer always be busy will
@@ skipping 14 matching lines @@
       aura::Env::GetInstance()->context_factory();
   // Note: This can fail if GPU acceleration has been disabled.
   scoped_refptr<cc::ContextProvider> context_provider =
       context_factory->SharedMainThreadContextProvider();
   if (!context_provider) {
     DLOG(WARNING) << "Failed to acquire a context provider";
     Release();  // Decrements the use count
     return nullptr;
   }
 
+  compositor_frame_sink_holder_ = compositor_frame_sink_holder;

[reveman, 2017/01/02 19:57:56]

nit: short comment here saying something like: "We need to keep a reference to
the sink holder to receive a release callback.

+
   // Create a new image texture for |gpu_memory_buffer_| with |texture_target_|
   // if one doesn't already exist. The contents of this buffer are copied to
   // |texture| using a call to CopyTexImage.
   if (!contents_texture_) {
     contents_texture_ = base::MakeUnique<Texture>(
         context_factory, context_provider.get(), gpu_memory_buffer_.get(),
         texture_target_, query_type_);
   }
 
   if (use_zero_copy_) {
@@ skipping 78 matching lines @@
   CheckReleaseCallback();
 }
 
 void Buffer::CheckReleaseCallback() {
   if (attach_count_ || use_count_)
     return;
 
   // Run release callback to notify the client that buffer has been released.
   if (!release_callback_.is_null())
     release_callback_.Run();
+
+  compositor_frame_sink_holder_ = nullptr;

[reveman, 2017/01/02 19:57:56]

Do we really need this here? Removing it would solve the sink holder destruction
problems afaict.

 }
 
 void Buffer::ReleaseTexture(std::unique_ptr<Texture> texture) {
   texture_ = std::move(texture);
 }
 
 void Buffer::ReleaseContentsTexture(std::unique_ptr<Texture> texture) {
   TRACE_EVENT0("exo", "Buffer::ReleaseContentsTexture");
 
   contents_texture_ = std::move(texture);
   Release();
 }
 
 }  // namespace exo