Make content::BufferQueue use scoped ptrs for AllocatedSurfaces

content/browser/compositor/buffer_queue.cc

Index: content/browser/compositor/buffer_queue.cc
diff --git a/content/browser/compositor/buffer_queue.cc b/content/browser/compositor/buffer_queue.cc
index b9b750ac2db96dd11d962850920bab6fb1e23114..f38bae7b9d0b0a34d7e8a10db7e5d84d932b07fd 100644
--- a/content/browser/compositor/buffer_queue.cc
+++ b/content/browser/compositor/buffer_queue.cc
@@ -30,6 +30,7 @@ BufferQueue::BufferQueue(
       allocated_count_(0),
       texture_target_(texture_target),
       internal_format_(internalformat),
+      deleted_in_flight_surface_count_(0),
       gl_helper_(gl_helper),
       gpu_memory_buffer_manager_(gpu_memory_buffer_manager),
       surface_id_(surface_id) {}
@@ -51,10 +52,11 @@ 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);
   }
 }
 
@@ -69,33 +71,37 @@ void BufferQueue::CopyBufferDamage(int texture,
 }
 
 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);
+  if (displayed_surface_)
+    displayed_surface_->damage.Union(damage);
 
-  for (std::deque<AllocatedSurface>::iterator it =
-           in_flight_surfaces_.begin();
-       it != in_flight_surfaces_.end();
-       ++it)
-    it->damage.Union(damage);
+  for (size_t i = 0; i < available_surfaces_.size(); i++)
+    available_surfaces_[i]->damage.Union(damage);
+  for (size_t i = 0; i < in_flight_surfaces_.size(); i++)
+    in_flight_surfaces_[i]->damage.Union(damage);
 }
 
 void BufferQueue::SwapBuffers(const gfx::Rect& damage) {
+  if (!current_surface_)
+    return;
+
   if (damage != gfx::Rect(size_)) {
-    // We must have a frame available to copy from.
-    DCHECK(!in_flight_surfaces_.empty() || displayed_surface_.texture);
-    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 there is damage, then there must have been a previous frame to copy
+    // from (though it may have been deleted).
+    DCHECK(deleted_in_flight_surface_count_ || !in_flight_surfaces_.empty() ||
+           displayed_surface_);
+    unsigned int texture_id = 0;
+    if (!in_flight_surfaces_.empty())
+      texture_id = in_flight_surfaces_.back()->texture;
+    else if (displayed_surface_)
+      texture_id = displayed_surface_->texture;
+    if (texture_id) {
+      CopyBufferDamage(current_surface_->texture, texture_id, damage,
+                       current_surface_->damage);
+    }
   }
   UpdateBufferDamage(damage);
-  current_surface_.damage = gfx::Rect();
-  in_flight_surfaces_.push_back(current_surface_);
-  current_surface_.texture = 0;
-  current_surface_.image = 0;
+  current_surface_->damage = gfx::Rect();
+  in_flight_surfaces_.push_back(current_surface_.Pass());
   // Some things reset the framebuffer (CopySubBufferDamage, some GLRenderer
   // paths), so ensure we restore it here.
   context_provider_->ContextGL()->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
@@ -108,7 +114,7 @@ void BufferQueue::Reshape(const gfx::Size& size, float scale_factor) {
   // 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;
 
@@ -122,93 +128,111 @@ void BufferQueue::Reshape(const gfx::Size& size, float scale_factor) {
 }
 
 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();
+  // Available surfaces do not need to be recreated, so they may be destoryed.
+  while (!available_surfaces_.empty()) {
+    FreeSurface(available_surfaces_.take_back().Pass());
+    available_surfaces_.pop_back();
+  }
 
-  for (auto& surface : in_flight_surfaces_)
-    surface = RecreateBuffer(&surface);
+  // Recreate all in-flight surfaces and put the recreated copies in the queue.
+  size_t old_in_flight_surface_size = in_flight_surfaces_.size();
+  for (size_t i = 0; i < old_in_flight_surface_size; ++i)
+    // Ensure that all entries in |in_flight_surfaces_| be non-null. If a
+    // recreation fails, then mark that it wasdeleted, to ensure it is accounted
+    // for in PageFlipComplete.
+    scoped_ptr<AllocatedSurface> recreated_buffer =
+        RecreateBuffer(in_flight_surfaces_.take_front().Pass());
+  if (recreated_buffer)
+    in_flight_surfaces_.push_back(recreated_buffer);
+  else
+    deleted_in_flight_surface_count_ += 1;
+}
 
-  current_surface_ = RecreateBuffer(&current_surface_);
-  displayed_surface_ = RecreateBuffer(&displayed_surface_);
+current_surface_ = RecreateBuffer(current_surface_.Pass());
+displayed_surface_ = RecreateBuffer(displayed_surface_.Pass());
 
-  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 scoped_ptr<AllocatedSurface>();
+
+  scoped_ptr<AllocatedSurface> new_surface(GetNextSurface());
+  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;
+  FreeSurface(surface.Pass());
+  return new_surface.Pass();
 }
 
 void BufferQueue::PageFlipComplete() {
-  DCHECK(!in_flight_surfaces_.empty());
-
-  if (displayed_surface_.texture)
-    available_surfaces_.push_back(displayed_surface_);
+  // If the next in-flight surface was destroyed, simply leave the currently
+  // displaying surface unchanged.
+  if (deleted_in_flight_surface_count_ > 0) {
+    deleted_in_flight_surface_count_ -= 1;
+    return;
+  }
 
-  displayed_surface_ = in_flight_surfaces_.front();
-  in_flight_surfaces_.pop_front();
+  DCHECK(!in_flight_surfaces_.empty());
+  if (displayed_surface_)
+    available_surfaces_.push_back(displayed_surface_.Pass());
+  displayed_surface_ = in_flight_surfaces_.take_front();
 }
 
 void BufferQueue::FreeAllSurfaces() {
-  FreeSurface(&displayed_surface_);
-  FreeSurface(&current_surface_);
-  // This is intentionally not emptied since the swap buffers acks are still
-  // expected to arrive.
-  for (auto& surface : in_flight_surfaces_)
-    FreeSurface(&surface);
+  FreeSurface(displayed_surface_.Pass());
+  FreeSurface(current_surface_.Pass());
 
-  for (size_t i = 0; i < available_surfaces_.size(); i++)
-    FreeSurface(&available_surfaces_[i]);
+  // Track the number of in-flight surfaces that have been destroyed, so that
+  // we account for them in PageFlipComplete.
+  deleted_in_flight_surface_count_ += in_flight_surfaces_.size();
+  while (!in_flight_surfaces_.empty())
+    FreeSurface(in_flight_surfaces_.take_front().Pass());
 
-  available_surfaces_.clear();
+  while (!available_surfaces_.empty()) {
+    FreeSurface(available_surfaces_.take_back().Pass());
+    available_surfaces_.pop_back();
+  }
 }
 
-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::FreeSurface(scoped_ptr<AllocatedSurface> surface) {

[reveman, 2015/10/27 17:03:43]

can we move all this into the AllocatedSurface dtor instead? that way we don't
have to worry about failing to call this and leaking the resources..

+  if (!surface)
+    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->image = 0;
+  surface->texture = 0;
+  allocated_count_--;
 }
 
-BufferQueue::AllocatedSurface BufferQueue::GetNextSurface() {
+scoped_ptr<BufferQueue::AllocatedSurface> BufferQueue::GetNextSurface() {
   if (!available_surfaces_.empty()) {
-    AllocatedSurface surface = available_surfaces_.back();
+    scoped_ptr<AllocatedSurface> result =
+        available_surfaces_.take_back().Pass();

[reveman, 2015/10/27 17:03:43]

hm, is .Pass() really needed here?

     available_surfaces_.pop_back();
-    return surface;
+    return result.Pass();
   }
 
   unsigned int texture = 0;
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   gl->GenTextures(1, &texture);
   if (!texture)
-    return AllocatedSurface();
+    return scoped_ptr<AllocatedSurface>();

[reveman, 2015/10/27 17:03:44]

nullptr

 
   // We don't want to allow anything more than triple buffering.
   DCHECK_LT(allocated_count_, 4U);
@@ -221,7 +245,7 @@ BufferQueue::AllocatedSurface BufferQueue::GetNextSurface() {
   if (!buffer.get()) {
     gl->DeleteTextures(1, &texture);
     DLOG(ERROR) << "Failed to allocate GPU memory buffer";
-    return AllocatedSurface();
+    return scoped_ptr<AllocatedSurface>();

[reveman, 2015/10/27 17:03:43]

nullptr

   }
 
   unsigned int id = gl->CreateImageCHROMIUM(
@@ -231,12 +255,14 @@ BufferQueue::AllocatedSurface BufferQueue::GetNextSurface() {
   if (!id) {
     LOG(ERROR) << "Failed to allocate backing image surface";
     gl->DeleteTextures(1, &texture);
-    return AllocatedSurface();
+    return scoped_ptr<AllocatedSurface>();

[reveman, 2015/10/27 17:03:44]

nullptr

   }
   allocated_count_++;

[reveman, 2015/10/27 17:03:43]

is this member needed? is this just not the sum of the size of the vectors?

   gl->BindTexture(texture_target_, texture);
   gl->BindTexImage2DCHROMIUM(texture_target_, id);
-  return AllocatedSurface(buffer.Pass(), texture, id, gfx::Rect(size_));
+  return scoped_ptr<AllocatedSurface>(

[reveman, 2015/10/27 17:03:44]

make_scoped_ptr(new All...)?

+             new AllocatedSurface(buffer.Pass(), texture, id, gfx::Rect(size_)))
+      .Pass();
 }
 
 BufferQueue::AllocatedSurface::AllocatedSurface() : texture(0), image(0) {}