Surfaceless OutputSurface implementation.

content/browser/compositor/buffered_output_surface.cc

Index: content/browser/compositor/buffered_output_surface.cc
diff --git a/content/browser/compositor/buffered_output_surface.cc b/content/browser/compositor/buffered_output_surface.cc
new file mode 100644
index 0000000000000000000000000000000000000000..01a68648a726997acd780f45d31860b43864d894
--- /dev/null
+++ b/content/browser/compositor/buffered_output_surface.cc
@@ -0,0 +1,140 @@
+// 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/buffered_output_surface.h"
+
+#include "content/common/gpu/client/context_provider_command_buffer.h"
+#include "gpu/GLES2/gl2extchromium.h"
+#include "gpu/command_buffer/client/gles2_interface.h"
+
+namespace content {
+
+BufferedOutputSurface::BufferedOutputSurface(
+    scoped_refptr<cc::ContextProvider> context_provider,
+    unsigned int internalformat)
+    : context_provider_(context_provider),
+      fbo_(0),
+      depth_rb_(0),
+      allocated_count_(0),
+      internalformat_(internalformat) {
+  Initialize();
+}
+
+BufferedOutputSurface::~BufferedOutputSurface() {
+  FreeAllSurfaces();
+
+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  if (fbo_)
+    gl->DeleteFramebuffers(1, &fbo_);
+  if (depth_rb_)
+    gl->DeleteRenderbuffers(1, &depth_rb_);

[piman, 2014/09/11 20:25:29]

We don't need a depth buffer for the compositor.

We will need a stencil buffer when we do GPU rasterization in the browser
process, but we don't do that yet (worth a TODO). We'd only want to allocate the
stencil buffer when that is enabled.

[achaulk, 2014/09/12 19:51:07]

Done.

+}
+
+void BufferedOutputSurface::Initialize() {
+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  gl->GenFramebuffers(1, &fbo_);
+  gl->GenRenderbuffers(1, &depth_rb_);
+  DCHECK(fbo_);
+  DCHECK(depth_rb_);

[piman, 2014/09/11 20:25:28]

Both these DCHECKs can fail in case of a lost context.

Initialization should be allowed to fail (resulting in
GpuSurfacelessBrowserCompositorOutputSurface::BindToClient returning false)

[achaulk, 2014/09/12 19:51:08]

Done.

+}
+
+void BufferedOutputSurface::BindFramebuffer() {
+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  gl->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
+
+  if (!current_surface_.first) {
+    current_surface_ = GetNextSurface();
+    gl->FramebufferTexture2D(GL_FRAMEBUFFER,
+                             GL_COLOR_ATTACHMENT0,
+                             GL_TEXTURE_2D,
+                             current_surface_.second,

[piman, 2014/09/11 20:25:28]

I'm confused. In BufferedOutputSurface::GetNextSurface, the texture id is
returned as the first member of the pair, the image id is returned as the second
member. It looks like here you're using the image id instead of the texture id.

See also comment in the .h

[achaulk, 2014/09/12 19:51:08]

Oh yes this should be first instead

+                             0);
+  }
+}
+
+void BufferedOutputSurface::SwapBuffers() {
+  if (last_surface_.first)
+    in_flight_surfaces_.push(last_surface_);

[piman, 2014/09/11 20:25:28]

in_flight_surfaces_ is confusing. It contains all in-flight surfaces except the
earliest one.
How about:
- always pushing the current surface into in_flight_surfaces_ (i.e. you don't
need last_surface_ any more)
- pushing an empty surface at the beginning, to ensure we don't recycle the
first one too early.
- in_flight_surfaces_ can never be empty (since you have at most as many
PageFlipComplete as SwapBuffers)

[achaulk, 2014/09/12 19:51:08]

I don't really like having a dummy surface, but I can merge last into here.

+  last_surface_ = current_surface_;
+  current_surface_.first = 0;
+  current_surface_.second = 0;
+}
+
+void BufferedOutputSurface::Reshape(const gfx::Size& size, float scale_factor) {
+  if (size == size_)
+    return;
+  size_ = size;
+  FreeAllSurfaces();

[piman, 2014/09/11 20:25:28]

Is it really ok to delete the surface that is currently being displayed (the
last one you swapped)?

[achaulk, 2014/09/12 19:51:07]

Yes. Only the GL-side structures will get torn down, the actual buffers are kept
around separately

+

[piman, 2014/09/11 20:25:28]

Can you add a DCHECK that current_surface_ is empty?
I think that's true today - Reshape is called before BindFramebuffer after a
swap, but if that changed to not be true, then you'd need to reallocate now and
update the FBO attachment.

[achaulk, 2014/09/12 19:51:08]

Done.

+  if (size_.width() == 0 || size_.height() == 0)
+    return;  // Nothing to do.

[piman, 2014/09/11 20:25:29]

You probably want to at least unbind the current attachments, so that you can
free the memory.

[achaulk, 2014/09/12 19:51:08]

Done.

+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  gl->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
+  gl->BindRenderbuffer(GL_RENDERBUFFER, depth_rb_);
+  gl->RenderbufferStorage(
+      GL_RENDERBUFFER, GL_DEPTH_COMPONENT24_OES, size_.width(), size_.height());
+  gl->FramebufferRenderbuffer(
+      GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, depth_rb_);
+}
+
+void BufferedOutputSurface::PageFlipComplete() {
+  if (!in_flight_surfaces_.empty()) {
+    available_surfaces_.push_back(in_flight_surfaces_.front());
+    in_flight_surfaces_.pop();
+  }
+}
+
+void BufferedOutputSurface::FreeAllSurfaces() {
+  FreeSurface(&last_surface_);
+  FreeSurface(&current_surface_);
+  while (!in_flight_surfaces_.empty()) {
+    FreeSurface(&in_flight_surfaces_.front());
+    in_flight_surfaces_.pop();
+  }
+  for (size_t i = 0; i < available_surfaces_.size(); i++)
+    FreeSurface(&available_surfaces_[i]);
+  available_surfaces_.clear();
+}
+
+void BufferedOutputSurface::FreeSurface(AllocatedSurface* surface) {
+  if (surface->first) {
+    gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+    gl->BindTexture(GL_TEXTURE_2D, surface->first);
+    gl->ReleaseTexImage2DCHROMIUM(GL_TEXTURE_2D, surface->second);
+    gl->DeleteTextures(1, &surface->first);
+    gl->DestroyImageCHROMIUM(surface->second);
+    surface->first = 0;
+    surface->second = 0;
+    allocated_count_--;
+  }
+}
+
+BufferedOutputSurface::AllocatedSurface
+BufferedOutputSurface::GetNextSurface() {
+  if (!available_surfaces_.empty()) {
+    AllocatedSurface id = available_surfaces_.back();
+    available_surfaces_.pop_back();
+    return id;
+  }
+
+  unsigned int tex;

[piman, 2014/09/11 20:25:28]

nit: texture

[achaulk, 2014/09/12 19:51:08]

Done.

+  gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
+  gl->GenTextures(1, &tex);
+
+  // We don't want to allow anything more than triple buffering.
+  DCHECK_LT(allocated_count_, 4U);
+
+  unsigned int id = context_provider_->ContextGL()->CreateImageCHROMIUM(
+      size_.width(),
+      size_.height(),
+      internalformat_,
+      GL_IMAGE_SCANOUT_CHROMIUM);
+  DCHECK(id);
+  allocated_count_++;
+  gl->BindTexture(GL_TEXTURE_2D, tex);
+  gl->BindTexImage2DCHROMIUM(GL_TEXTURE_2D, id);
+  return AllocatedSurface(tex, id);
+}
+
+}  // namespace content