Make content::BufferQueue use scoped ptrs for AllocatedSurfaces

content/browser/compositor/buffer_queue.cc

 // Copyright 2014 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 "content/browser/compositor/buffer_queue.h"
 
 #include "content/browser/compositor/image_transport_factory.h"
 #include "content/browser/gpu/browser_gpu_memory_buffer_manager.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
 #include "content/common/gpu/client/gl_helper.h"
@@ skipping 33 matching lines @@
 
 void BufferQueue::Initialize() {
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   gl->GenFramebuffers(1, &fbo_);
 }
 
 void BufferQueue::BindFramebuffer() {
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   gl->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
 
-  if (!current_surface_.texture) {
+  if (!current_surface_) {
     current_surface_ = GetNextSurface();
-    gl->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
-                             texture_target_, current_surface_.texture, 0);
+    gl->FramebufferTexture2D(
+        GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture_target_,
+        current_surface_ ? current_surface_->texture : 0, 0);

[danakj, 2015/12/10 01:36:02]

is there a point to doing FramebufferTexture2D(..., 0, 0) here if
GetNextSurface() failed?  We don't in RecreateBuffers, should we there?

[ccameron, 2015/12/10 07:39:27]

The spec says that the texture is automatically un-bound, so, no point. Removed.

   }
 }
 
 void BufferQueue::CopyBufferDamage(int texture,
                                    int source_texture,
                                    const gfx::Rect& new_damage,
                                    const gfx::Rect& old_damage) {
   gl_helper_->CopySubBufferDamage(
       texture_target_, texture, source_texture,
       SkRegion(gfx::RectToSkIRect(new_damage)),
       SkRegion(gfx::RectToSkIRect(old_damage)));
 }
 
 void BufferQueue::UpdateBufferDamage(const gfx::Rect& damage) {
-  displayed_surface_.damage.Union(damage);
-  for (size_t i = 0; i < available_surfaces_.size(); i++)
-    available_surfaces_[i].damage.Union(damage);
-
-  for (std::deque<AllocatedSurface>::iterator it =
-           in_flight_surfaces_.begin();
-       it != in_flight_surfaces_.end();
-       ++it)
-    it->damage.Union(damage);
+  if (displayed_surface_)
+    displayed_surface_->damage.Union(damage);
+  for (auto& surface : available_surfaces_)
+    surface->damage.Union(damage);
+  for (auto& surface : in_flight_surfaces_) {
+    if (surface)
+      surface->damage.Union(damage);
+  }
 }
 
 void BufferQueue::SwapBuffers(const gfx::Rect& damage) {
-  if (!damage.IsEmpty() && damage != gfx::Rect(size_)) {
-    // We must have a frame available to copy from.
-    DCHECK(!in_flight_surfaces_.empty() || displayed_surface_.texture);

[danakj, 2015/12/10 01:36:02]

no more dcheck?

[ccameron, 2015/12/10 07:39:27]

That was wrong-by-inspection. It was entirely possible for
|displayed_surface_.texture| to be zero if we fail to allocate the buffer.

-    unsigned int texture_id = !in_flight_surfaces_.empty()
-                                  ? in_flight_surfaces_.back().texture
-                                  : displayed_surface_.texture;
-
-    CopyBufferDamage(current_surface_.texture, texture_id, damage,
-                     current_surface_.damage);
+  if (current_surface_) {
+    if (!damage.IsEmpty() && damage != gfx::Rect(size_)) {
+      unsigned int texture_id = 0;
+      if (!in_flight_surfaces_.empty() && in_flight_surfaces_.back())

[danakj, 2015/12/10 01:36:02]

This logic is a bit different.

Before if there was an inflight surface it would use that id, even 0. Now if
it's 0 it'll try use displayed_surface_.

Maybe it's missing a test for this code.

[ccameron, 2015/12/10 07:39:27]

Yeah. Actually, to be more thorough, we should do the in-flight surfaces
back-to-front, then hit up the displayed surface.
Moar tests added!!

+        texture_id = in_flight_surfaces_.back()->texture;
+      else if (displayed_surface_)
+        texture_id = displayed_surface_->texture;
+      if (texture_id) {

[danakj, 2015/12/10 01:36:02]

nit: can you put a whitespace line above this to separate it from the if/elseif
stuff above?

[ccameron, 2015/12/10 07:39:27]

Done.

+        CopyBufferDamage(current_surface_->texture, texture_id, damage,
+                         current_surface_->damage);
+      }
+    }
+    current_surface_->damage = gfx::Rect();
   }
   UpdateBufferDamage(damage);
-  current_surface_.damage = gfx::Rect();
-  in_flight_surfaces_.push_back(current_surface_);
-  current_surface_.texture = 0;
-  current_surface_.image = 0;
+  in_flight_surfaces_.push_back(std::move(current_surface_));
   // Some things reset the framebuffer (CopySubBufferDamage, some GLRenderer
   // paths), so ensure we restore it here.
   context_provider_->ContextGL()->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
 }
 
 void BufferQueue::Reshape(const gfx::Size& size, float scale_factor) {
   if (size == size_)
     return;
   // TODO(ccameron): This assert is being hit on Mac try jobs. Determine if that
   // is cause for concern or if it is benign.
   // http://crbug.com/524624
 #if !defined(OS_MACOSX)
-  DCHECK(!current_surface_.texture);
+  DCHECK(!current_surface_);
 #endif
   size_ = size;
 
   // TODO: add stencil buffer when needed.
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   gl->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
   gl->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
                            texture_target_, 0, 0);
 
   FreeAllSurfaces();
 }
 
 void BufferQueue::RecreateBuffers() {
   // We need to recreate the buffers, for whatever reason the old ones are not
   // presentable on the device anymore.
   // Unused buffers can be freed directly, they will be re-allocated as needed.
   // Any in flight, current or displayed surface must be replaced.
-  for (size_t i = 0; i < available_surfaces_.size(); i++)
-    FreeSurface(&available_surfaces_[i]);
   available_surfaces_.clear();
 
+  // Recreate all in-flight surfaces and put the recreated copies in the queue.
   for (auto& surface : in_flight_surfaces_)
-    surface = RecreateBuffer(&surface);
+    surface = RecreateBuffer(std::move(surface));
 
-  current_surface_ = RecreateBuffer(&current_surface_);
-  displayed_surface_ = RecreateBuffer(&displayed_surface_);
+  current_surface_ = RecreateBuffer(std::move(current_surface_));
+  displayed_surface_ = RecreateBuffer(std::move(displayed_surface_));
 
-  if (current_surface_.texture) {
+  if (current_surface_) {
     // If we have a texture bound, we will need to re-bind it.
     gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
     gl->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
     gl->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
-                             texture_target_, current_surface_.texture, 0);
+                             texture_target_, current_surface_->texture, 0);
   }
 }
 
-BufferQueue::AllocatedSurface BufferQueue::RecreateBuffer(
-    AllocatedSurface* surface) {
-  if (!surface->texture)
-    return *surface;
-  AllocatedSurface new_surface = GetNextSurface();
-  new_surface.damage = surface->damage;
+scoped_ptr<BufferQueue::AllocatedSurface> BufferQueue::RecreateBuffer(
+    scoped_ptr<AllocatedSurface> surface) {
+  if (!surface)
+    return nullptr;
+
+  scoped_ptr<AllocatedSurface> new_surface(GetNextSurface());
+  if (!new_surface)
+    return nullptr;
+
+  new_surface->damage = surface->damage;
 
   // Copy the entire texture.
-  CopyBufferDamage(new_surface.texture, surface->texture, gfx::Rect(),
+  CopyBufferDamage(new_surface->texture, surface->texture, gfx::Rect(),
                    gfx::Rect(size_));
-
-  FreeSurface(surface);
   return new_surface;
 }
 
 void BufferQueue::PageFlipComplete() {
   DCHECK(!in_flight_surfaces_.empty());
-
-  if (displayed_surface_.texture)
-    available_surfaces_.push_back(displayed_surface_);
-
-  displayed_surface_ = in_flight_surfaces_.front();
+  if (displayed_surface_)
+    available_surfaces_.push_back(std::move(displayed_surface_));
+  displayed_surface_ = std::move(in_flight_surfaces_.front());
   in_flight_surfaces_.pop_front();
 }
 
 void BufferQueue::FreeAllSurfaces() {
-  FreeSurface(&displayed_surface_);
-  FreeSurface(&current_surface_);
+  displayed_surface_.reset();
+  current_surface_.reset();
   // This is intentionally not emptied since the swap buffers acks are still
   // expected to arrive.
   for (auto& surface : in_flight_surfaces_)
-    FreeSurface(&surface);
-
-  for (size_t i = 0; i < available_surfaces_.size(); i++)
-    FreeSurface(&available_surfaces_[i]);
-
+    surface = nullptr;
   available_surfaces_.clear();
 }
 
-void BufferQueue::FreeSurface(AllocatedSurface* surface) {
-  if (surface->texture) {
-    gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
-    gl->BindTexture(texture_target_, surface->texture);
-    gl->ReleaseTexImage2DCHROMIUM(texture_target_, surface->image);
-    gl->DeleteTextures(1, &surface->texture);
-    gl->DestroyImageCHROMIUM(surface->image);
-    surface->image = 0;
-    surface->texture = 0;
-    allocated_count_--;
-  }
+void BufferQueue::FreeSurfaceResources(AllocatedSurface* surface) {
+  if (!surface->texture)
+    return;
+
+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  gl->BindTexture(texture_target_, surface->texture);
+  gl->ReleaseTexImage2DCHROMIUM(texture_target_, surface->image);
+  gl->DeleteTextures(1, &surface->texture);
+  gl->DestroyImageCHROMIUM(surface->image);
+  surface->buffer.reset();
+  allocated_count_--;
 }
 
-BufferQueue::AllocatedSurface BufferQueue::GetNextSurface() {
+scoped_ptr<BufferQueue::AllocatedSurface> BufferQueue::GetNextSurface() {
   if (!available_surfaces_.empty()) {
-    AllocatedSurface surface = available_surfaces_.back();
+    scoped_ptr<AllocatedSurface> surface =
+        std::move(available_surfaces_.back());
     available_surfaces_.pop_back();
     return surface;
   }
 
   unsigned int texture = 0;
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   gl->GenTextures(1, &texture);
   if (!texture)
-    return AllocatedSurface();
+    return nullptr;
 
   // We don't want to allow anything more than triple buffering.
   DCHECK_LT(allocated_count_, 4U);
 
   scoped_ptr<gfx::GpuMemoryBuffer> buffer(
       gpu_memory_buffer_manager_->AllocateGpuMemoryBufferForScanout(
           size_, gpu::ImageFactory::DefaultBufferFormatForImageFormat(
                      internal_format_),
           surface_id_));
   if (!buffer.get()) {
     gl->DeleteTextures(1, &texture);
     DLOG(ERROR) << "Failed to allocate GPU memory buffer";
-    return AllocatedSurface();
+    return nullptr;
   }
 
   unsigned int id = gl->CreateImageCHROMIUM(
       buffer->AsClientBuffer(), size_.width(), size_.height(),
       internal_format_);
-
   if (!id) {
     LOG(ERROR) << "Failed to allocate backing image surface";
     gl->DeleteTextures(1, &texture);
-    return AllocatedSurface();
+    return nullptr;
   }
+
   allocated_count_++;
   gl->BindTexture(texture_target_, texture);
   gl->BindTexImage2DCHROMIUM(texture_target_, id);
-  return AllocatedSurface(buffer.Pass(), texture, id, gfx::Rect(size_));
+  return make_scoped_ptr(new AllocatedSurface(this, std::move(buffer), texture,
+                                              id, gfx::Rect(size_)));
 }
 
-BufferQueue::AllocatedSurface::AllocatedSurface() : texture(0), image(0) {}
-
 BufferQueue::AllocatedSurface::AllocatedSurface(
+    BufferQueue* buffer_queue,
     scoped_ptr<gfx::GpuMemoryBuffer> buffer,
     unsigned int texture,
     unsigned int image,
     const gfx::Rect& rect)
-    : buffer(buffer.release()), texture(texture), image(image), damage(rect) {}
+    : buffer_queue(buffer_queue),
+      buffer(buffer.release()),
+      texture(texture),
+      image(image),
+      damage(rect) {}
 
-BufferQueue::AllocatedSurface::~AllocatedSurface() {}
+BufferQueue::AllocatedSurface::~AllocatedSurface() {
+  if (buffer_queue)

[danakj, 2015/12/10 01:36:02]

i don't see calls to default constructor anymore. never null? maybe remove
default constructor?

[ccameron, 2015/12/10 07:39:27]

Good call -- I removed the default ctor, so this can be killed now. 

+    buffer_queue->FreeSurfaceResources(this);
+}
 
 }  // namespace content