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);
   }
 }
 
 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_)
+    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_);
 }
 
 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(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);
 
   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 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(
       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 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) {}
+    : 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/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