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 c82741654b59cf4a259091a154d6e7324c7a0b49..f88390a9d955cca8ed90b3a19a5220bd177efe78 100644
--- a/content/browser/compositor/buffer_queue.cc
+++ b/content/browser/compositor/buffer_queue.cc
@@ -51,10 +51,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 +70,29 @@ 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);
-
-  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_)
+    surface->damage.Union(damage);
 }
 
 void BufferQueue::SwapBuffers(const gfx::Rect& damage) {
+  DCHECK(current_surface_);
+
   if (!damage.IsEmpty() && damage != gfx::Rect(size_)) {
     // We must have a frame available to copy from.
-    DCHECK(!in_flight_surfaces_.empty() || displayed_surface_.texture);
+    DCHECK(!in_flight_surfaces_.empty() || displayed_surface_);
     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);
+                                  ? in_flight_surfaces_.back()->texture
+                                  : displayed_surface_->texture;
+    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());

[danakj, 2015/12/09 23:25:16]

std::move()

[ccameron, 2015/12/10 00:19:28]

Done.

   // Some things reset the framebuffer (CopySubBufferDamage, some GLRenderer
   // paths), so ensure we restore it here.
   context_provider_->ContextGL()->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
@@ -108,7 +105,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;
 
@@ -126,89 +123,84 @@ void BufferQueue::RecreateBuffers() {
   // 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(surface.Pass());

[danakj, 2015/12/09 23:25:15]

std::move() for all passing

[ccameron, 2015/12/10 00:19:28]

Done.

 
-  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_ && current_surface_->texture) {

[danakj, 2015/12/09 23:25:15]

you could just make RecreateBuffer return null if the texture would be 0?
doesn't it already?

[ccameron, 2015/12/10 00:19:28]

This is the "remnant" mentioned below.

     // 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>();

[danakj, 2015/12/09 23:25:15]

return nullptr

[ccameron, 2015/12/10 00:19:28]

Done.

+
+  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;
+  return new_surface.Pass();

[danakj, 2015/12/09 23:25:15]

don't std::move() on return unless you're upcasting. clang should yell at you
for this if you use move()

[ccameron, 2015/12/10 00:19:28]

It does :)

 }
 
 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_ && displayed_surface_->texture)

[danakj, 2015/12/09 23:25:15]

can you be non-null but 0 texture?

[ccameron, 2015/12/10 00:19:28]

Yes (... see discussion later).

+    available_surfaces_.push_back(displayed_surface_.Pass());
+  displayed_surface_ = in_flight_surfaces_.front().Pass();
   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]);
-
+    FreeSurfaceResources(surface.get());
   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->image = 0;
+  surface->texture = 0;
+  surface->buffer.reset();
+  surface->buffer_queue = nullptr;
+  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 = available_surfaces_.back().Pass();
     available_surfaces_.pop_back();
-    return surface;
+    return surface.Pass();

[danakj, 2015/12/09 23:25:15]

should be move() but on return shouldn't be anything

[ccameron, 2015/12/10 00:19:28]

Done.

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

[danakj, 2015/12/09 23:25:15]

make_scoped_ptr(new All...)

but why not return nullptr in these errors? or, why are you nullchecking in
other places?

IOW it'd be nice to have 1 way to error not two: either null or 0 texture,
choose one?

[ccameron, 2015/12/10 00:19:28]

Myself as of a few hours agreed with you (actually, there are some remanants of
that above).

Myself as of sending this out figured that was too big a change to merge into
this.

But ... I gave that another try, and it's not too hard.

The idea before was that nullptr is never returned on error. If a surface is
requested, you always get a pointer, but it may wrap a zero-texture -- so
nullptr represents something just not being there, not an error (the current
surface is nullptr when it is not bound yet, the displayed surface is nullptr
before it is swapped in).

But, now there is just nullptr.

 
   // We don't want to allow anything more than triple buffering.
   DCHECK_LT(allocated_count_, 4U);
@@ -221,7 +213,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>(new AllocatedSurface);

[danakj, 2015/12/09 23:25:15]

make_scoped_ptr

[ccameron, 2015/12/10 00:19:27]

Done.

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

[danakj, 2015/12/09 23:25:15]

make_scoped_ptr

[ccameron, 2015/12/10 00:19:28]

Done.

   }
   allocated_count_++;
   gl->BindTexture(texture_target_, texture);
   gl->BindTexImage2DCHROMIUM(texture_target_, id);
-  return AllocatedSurface(buffer.Pass(), texture, id, gfx::Rect(size_));
+  return scoped_ptr<AllocatedSurface>(new AllocatedSurface(this, buffer.Pass(),
+                                                           texture, id,
+                                                           gfx::Rect(size_)))
+      .Pass();
 }
 
-BufferQueue::AllocatedSurface::AllocatedSurface() : texture(0), image(0) {}
+BufferQueue::AllocatedSurface::AllocatedSurface()
+    : buffer_queue(nullptr), 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) {}
-
-BufferQueue::AllocatedSurface::~AllocatedSurface() {}
+    : buffer_queue(buffer_queue),
+      buffer(buffer.release()),
+      texture(texture),
+      image(image),
+      damage(rect) {}
+
+BufferQueue::AllocatedSurface::~AllocatedSurface() {
+  if (buffer_queue)

[danakj, 2015/12/09 23:25:15]

it can be null?

[ccameron, 2015/12/10 00:19:28]

Yeah, if the default ctor was used.

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