Implement TextureImageTransportSurface using texture mailbox

content/common/gpu/texture_image_transport_surface.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[apatrick_chromium, 2012/11/09 19:19:04]

Deleted this line?

[no sievers, 2012/11/09 21:53:02]

Oops done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/gpu/texture_image_transport_surface.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "content/common/gpu/gl_scoped_binders.h"
 #include "content/common/gpu/gpu_channel.h"
 #include "content/common/gpu/gpu_channel_manager.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/common/gpu/sync_point_manager.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/service/context_group.h"
 #include "gpu/command_buffer/service/gpu_scheduler.h"
-#include "gpu/command_buffer/service/texture_manager.h"
+#include "gpu/command_buffer/service/texture_definition.h"
 
 using gpu::gles2::ContextGroup;
+using gpu::gles2::MailboxManager;
+using gpu::gles2::MailboxName;
+using gpu::gles2::TextureDefinition;
 using gpu::gles2::TextureManager;
-typedef TextureManager::TextureInfo TextureInfo;
 
 namespace content {
 
 TextureImageTransportSurface::Texture::Texture()
-    : client_id(0),
-      sent_to_client(false) {
+    : service_id(0),
+      identifier(0) {
 }
 
 TextureImageTransportSurface::Texture::~Texture() {
 }
 
 TextureImageTransportSurface::TextureImageTransportSurface(
     GpuChannelManager* manager,
     GpuCommandBufferStub* stub,
     const gfx::GLSurfaceHandle& handle)
       : fbo_id_(0),
-        front_(0),
         stub_destroyed_(false),
         backbuffer_suggested_allocation_(true),
         frontbuffer_suggested_allocation_(true),
-        frontbuffer_is_protected_(true),
-        protection_state_id_(0),
         handle_(handle),
-        parent_stub_(NULL),
         is_swap_buffers_pending_(false),
-        did_unschedule_(false),
-        did_flip_(false) {
+        did_unschedule_(false) {
   helper_.reset(new ImageTransportHelper(this,
                                          manager,
                                          stub,
                                          gfx::kNullPluginWindow));
 }
 
 TextureImageTransportSurface::~TextureImageTransportSurface() {
   DCHECK(stub_destroyed_);
   Destroy();
 }
 
 bool TextureImageTransportSurface::Initialize() {
+  mailbox_manager_ =
+      helper_->stub()->decoder()->GetContextGroup()->mailbox_manager();
+
+  backbuffer_.identifier = 1;
+
   GpuChannelManager* manager = helper_->manager();
-  GpuChannel* parent_channel = manager->LookupChannel(handle_.parent_client_id);
-  if (!parent_channel)
-    return false;
-
-  parent_stub_ = parent_channel->LookupCommandBuffer(handle_.parent_context_id);
-  if (!parent_stub_)
-    return false;
-
-  parent_stub_->AddDestructionObserver(this);
-  TextureManager* texture_manager =
-      parent_stub_->decoder()->GetContextGroup()->texture_manager();
-  DCHECK(texture_manager);
-
-  for (int i = 0; i < 2; ++i) {
-    Texture& texture = textures_[i];
-    texture.client_id = handle_.parent_texture_id[i];
-    texture.info = texture_manager->GetTextureInfo(texture.client_id);
-    if (!texture.info)
-      return false;
-
-    if (!texture.info->target())
-      texture_manager->SetInfoTarget(texture.info, GL_TEXTURE_2D);
-    texture_manager->SetParameter(
-        texture.info, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
-    texture_manager->SetParameter(
-        texture.info, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
-    texture_manager->SetParameter(
-        texture.info, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
-    texture_manager->SetParameter(
-        texture.info, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
-  }
-
   surface_ = manager->GetDefaultOffscreenSurface();
   if (!surface_.get())
     return false;
 
   if (!helper_->Initialize())
     return false;
 
-  const CommandLine* command_line = CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kUIPrioritizeInGpuProcess))
-    helper_->SetPreemptByCounter(parent_channel->MessagesPendingCount());
+  // TODO: Move this somewhere else.

[jonathan.backer, 2012/11/12 16:52:15]

Not sure it belongs anywhere else. We only want pre-emption for TITSurfaces with
a parent channel.

[no sievers, 2012/11/19 20:30:44]

I've removed the TODO for now. But maybe a more generic mechanism to prioritize
channels would be nice.

[piman, 2012/11/19 22:09:52]

The preemption concept is orthogonal to whether we use
TextureImageTransportSurfaces. In the ÜC world, we won't use them any more, but
still want the preemption feature.

+  GpuChannel* parent_channel = manager->LookupChannel(handle_.parent_client_id);
+  if (parent_channel) {
+    const CommandLine* command_line = CommandLine::ForCurrentProcess();
+    if (command_line->HasSwitch(switches::kUIPrioritizeInGpuProcess))
+      helper_->SetPreemptByCounter(parent_channel->MessagesPendingCount());
+  }
 
   return true;
 }
 
 void TextureImageTransportSurface::Destroy() {
-  if (parent_stub_) {
-    parent_stub_->decoder()->MakeCurrent();
-    ReleaseParentStub();
-  }
-
   if (surface_.get())
     surface_ = NULL;
 
   helper_->Destroy();
 }
 
 bool TextureImageTransportSurface::DeferDraws() {
   // The command buffer hit a draw/clear command that could clobber the
   // texture in use by the UI compositor. If a Swap is pending, abort
   // processing of the command by returning true and unschedule until the Swap
@@ skipping 18 matching lines @@
 bool TextureImageTransportSurface::OnMakeCurrent(gfx::GLContext* context) {
   if (stub_destroyed_) {
     // Early-exit so that we don't recreate the fbo. We still want to return
     // true, so that the context is made current and the GLES2DecoderImpl can
     // release its own resources.
     return true;
   }
 
   if (!fbo_id_) {
     glGenFramebuffersEXT(1, &fbo_id_);
     glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);

[apatrick_chromium, 2012/11/09 19:19:04]

This changes the FBO binding for the context and it is not restored. Is that
okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.

[no sievers, 2012/11/09 21:53:02]

Hmm if it's a problem, it should exist before my patch :)
I assume we rely on the compositor to issue a Bind() every time it uses an
intermediate render surface.

On 2012/11/09 19:19:04, apatrick_chromium wrote:

[piman, 2012/11/09 22:02:01]

It's probably not a problem, since this codes executes on the first MakeCurrent.
It might even be actually needed (I'm not sure the Decoder will explicitly set
GetBackingFrameBufferObject() as the current FBO on initialization).

[jonathan.backer, 2012/11/12 16:52:15]

So this is the first time that MakeCurrent is ever called. In this case, we'd
have the default fbo (0) active no?

We've got a slight of hand here: for TITSurface, the default fbo is meant to be
|fbo_id|. This remapping is usually handled by
|GLES2DecoderImpl::DoBindFramebuffer|. I think that it's appropriate here.

[piman, 2012/11/19 22:09:52]

Yes.
Yeah, that's more or less what I meant above.

-    CreateBackTexture(gfx::Size(1, 1));
+    current_size_ = gfx::Size(1, 1);
+    CreateBackTexture();
 
 #ifndef NDEBUG
     GLenum status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER);
     if (status != GL_FRAMEBUFFER_COMPLETE) {
       DLOG(ERROR) << "Framebuffer incomplete.";
       glDeleteFramebuffersEXT(1, &fbo_id_);
       fbo_id_ = 0;
       return false;
     }
 #endif
@@ skipping 10 matching lines @@
 
 void TextureImageTransportSurface::SetBackbufferAllocation(bool allocation) {
   if (backbuffer_suggested_allocation_ == allocation)
      return;
   backbuffer_suggested_allocation_ = allocation;
 
   if (!helper_->MakeCurrent())
     return;
 
   if (backbuffer_suggested_allocation_) {
-    DCHECK(!textures_[back()].info->service_id() ||
-           !textures_[back()].sent_to_client);
-    CreateBackTexture(textures_[back()].size);
+    DCHECK(!backbuffer_.service_id);
+    CreateBackTexture();
   } else {
-    ReleaseTexture(back());
+    ReleaseBackBuffer();
   }
 }
 
 void TextureImageTransportSurface::SetFrontbufferAllocation(bool allocation) {
   if (frontbuffer_suggested_allocation_ == allocation)
     return;
   frontbuffer_suggested_allocation_ = allocation;
   AdjustFrontBufferAllocation();
 }
 
 void TextureImageTransportSurface::AdjustFrontBufferAllocation() {
+  DCHECK(!is_swap_buffers_pending_);

[piman, 2012/11/09 22:02:01]

I don't think this check is valid. This can be called from
SetFrontbufferAllocation which can be called from a posted task at any time.

[no sievers, 2012/11/19 20:30:44]

Done.

   if (!helper_->MakeCurrent())

[piman, 2012/11/09 22:02:01]

We don't need this anymore.

[no sievers, 2012/11/19 20:30:44]

Done.

     return;
 
-  if (!frontbuffer_suggested_allocation_ && !frontbuffer_is_protected_ &&
-      textures_[front()].info->service_id()) {
-    ReleaseTexture(front());
-    if (textures_[front()].sent_to_client) {
-      GpuHostMsg_AcceleratedSurfaceRelease_Params params;
-      params.identifier = textures_[front()].client_id;
-      helper_->SendAcceleratedSurfaceRelease(params);
-      textures_[front()].sent_to_client = false;
-    }
+  if (!frontbuffer_suggested_allocation_) {
+    GpuHostMsg_AcceleratedSurfaceRelease_Params params;
+    helper_->SendAcceleratedSurfaceRelease(params);
   }
 }
 
 void* TextureImageTransportSurface::GetShareHandle() {
   return GetHandle();
 }
 
 void* TextureImageTransportSurface::GetDisplay() {
   return surface_.get() ? surface_->GetDisplay() : NULL;
 }
 
 void* TextureImageTransportSurface::GetConfig() {
   return surface_.get() ? surface_->GetConfig() : NULL;
 }
 
 void TextureImageTransportSurface::OnResize(gfx::Size size) {
-  CreateBackTexture(size);
+  current_size_ = size;
+  CreateBackTexture();
 }
 
 void TextureImageTransportSurface::OnWillDestroyStub(
     GpuCommandBufferStub* stub) {
-  if (stub == parent_stub_) {
-    ReleaseParentStub();
-    helper_->SetPreemptByCounter(NULL);
-  } else {
-    DCHECK(stub == helper_->stub());
-    stub->RemoveDestructionObserver(this);
+  DCHECK(stub == helper_->stub());
+  stub->RemoveDestructionObserver(this);
 
-    // We are losing the stub owning us, this is our last chance to clean up the
-    // resources we allocated in the stub's context.
-    if (fbo_id_) {
-      glDeleteFramebuffersEXT(1, &fbo_id_);
-      CHECK_GL_ERROR();
-      fbo_id_ = 0;
-    }
+  // We are losing the stub owning us, this is our last chance to clean up the
+  // resources we allocated in the stub's context.
+  if (fbo_id_) {
+    glDeleteFramebuffersEXT(1, &fbo_id_);
+    CHECK_GL_ERROR();
+    fbo_id_ = 0;
+  }
 
-    stub_destroyed_ = true;
-  }
+  stub_destroyed_ = true;
 }
 
 bool TextureImageTransportSurface::SwapBuffers() {
   DCHECK(backbuffer_suggested_allocation_);
-  if (!frontbuffer_suggested_allocation_ || !frontbuffer_is_protected_)
+  if (!frontbuffer_suggested_allocation_)
     return true;
-  if (!parent_stub_) {
-    LOG(ERROR) << "SwapBuffers failed because no parent stub.";
-    return false;
-  }
 
   glFlush();
-  front_ = back();
-  previous_damage_rect_ = gfx::Rect(textures_[front()].size);
+  const uint64 identifier = backbuffer_.identifier;
+  ProduceTexture(backbuffer_);
 
-  DCHECK(textures_[front()].client_id != 0);
+  // Do not allow destruction while we are still waiting for a swap ACK.
+  AddRef();

[piman, 2012/11/09 22:02:01]

I'm not sure you'll actually be getting an ACK if the renderer gets closed
(which then would be a leak). Have you verified this?

[jonathan.backer, 2012/11/12 16:52:15]

We have a default ACK in GpuProcessHostUIShim. I think this is safe. Still ugly.

[no sievers, 2012/11/19 20:30:44]

Yes, agreed that it's not pretty. Although I think conceptually it's nice to
avoid mailbox leaks through simply guaranteeing ACK cycles to be finished. I
didn't want to mess with a ton of plumbing or other objects' lifetimes.

Will the planned changes to mailbox (where the mailbox owns the gl texture id)
somehow make it easier to avoid leaks in the mailbox and allow me to remove this
hack?

 
   GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params params;
-  params.surface_handle = textures_[front()].client_id;
-  params.size = textures_[front()].size;
-  params.protection_state_id = protection_state_id_;
-  params.skip_ack = false;
+  params.surface_handle = identifier;
+  params.size = current_size_;
   helper_->SendAcceleratedSurfaceBuffersSwapped(params);
 
   DCHECK(!is_swap_buffers_pending_);
   is_swap_buffers_pending_ = true;
   return true;
 }
 
 bool TextureImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
   DCHECK(backbuffer_suggested_allocation_);
-  DCHECK(textures_[back()].info->service_id());
-  if (!frontbuffer_suggested_allocation_ || !frontbuffer_is_protected_)
+  DCHECK(backbuffer_.service_id);
+  if (!frontbuffer_suggested_allocation_)
     return true;
-  // If we are recreating the frontbuffer with this swap, make sure we are
-  // drawing a full frame.
-  DCHECK(textures_[front()].info->service_id() ||
-         (!x && !y && gfx::Size(width, height) == textures_[back()].size));
-  if (!parent_stub_) {
-    LOG(ERROR) << "PostSubBuffer failed because no parent stub.";
-    return false;
-  }
-
   const gfx::Rect new_damage_rect(x, y, width, height);
+  DCHECK(gfx::Rect(gfx::Point(), current_size_).Contains(new_damage_rect));
 
   // An empty damage rect is a successful no-op.
   if (new_damage_rect.IsEmpty())
     return true;
 
-  int back_texture_service_id = textures_[back()].info->service_id();
-  int front_texture_service_id = textures_[front()].info->service_id();
+  glFlush();
+  const uint64 identifier = backbuffer_.identifier;
+  ProduceTexture(backbuffer_);
 
-  gfx::Size expected_size = textures_[back()].size;
-  bool surfaces_same_size = textures_[front()].size == expected_size;
-
-  if (surfaces_same_size) {
-    std::vector<gfx::Rect> regions_to_copy;
-    GetRegionsToCopy(previous_damage_rect_, new_damage_rect, &regions_to_copy);
-
-    ScopedFrameBufferBinder fbo_binder(fbo_id_);
-    glFramebufferTexture2DEXT(GL_FRAMEBUFFER,
-        GL_COLOR_ATTACHMENT0,
-        GL_TEXTURE_2D,
-        front_texture_service_id,
-        0);
-    ScopedTextureBinder texture_binder(back_texture_service_id);
-
-    for (size_t i = 0; i < regions_to_copy.size(); ++i) {
-      const gfx::Rect& region_to_copy = regions_to_copy[i];
-      if (!region_to_copy.IsEmpty()) {
-        glCopyTexSubImage2D(GL_TEXTURE_2D, 0, region_to_copy.x(),
-            region_to_copy.y(), region_to_copy.x(), region_to_copy.y(),
-            region_to_copy.width(), region_to_copy.height());
-      }
-    }
-  } else if (!surfaces_same_size && did_flip_) {
-    DCHECK(new_damage_rect == gfx::Rect(expected_size));
-  }
-
-  glFlush();
-  front_ = back();
-  previous_damage_rect_ = new_damage_rect;
-
-  DCHECK(textures_[front()].client_id);
+  // Do not allow destruction while we are still waiting for a swap ACK.
+  AddRef();
 
   GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params params;
-  params.surface_handle = textures_[front()].client_id;
-  params.surface_size = textures_[front()].size;
+  params.surface_handle = identifier;
+  params.surface_size = current_size_;
   params.x = x;
   params.y = y;
   params.width = width;
   params.height = height;
-  params.protection_state_id = protection_state_id_;
   helper_->SendAcceleratedSurfacePostSubBuffer(params);
 
   DCHECK(!is_swap_buffers_pending_);
   is_swap_buffers_pending_ = true;
   return true;
 }
 
 std::string TextureImageTransportSurface::GetExtensions() {
   std::string extensions = gfx::GLSurface::GetExtensions();
   extensions += extensions.empty() ? "" : " ";
   extensions += "GL_CHROMIUM_front_buffer_cached ";
   extensions += "GL_CHROMIUM_post_sub_buffer";
   return extensions;
 }
 
 gfx::Size TextureImageTransportSurface::GetSize() {
-  gfx::Size size = textures_[back()].size;
+  gfx::Size size = current_size_;
 
   // OSMesa expects a non-zero size.
   return gfx::Size(size.width() == 0 ? 1 : size.width(),
                    size.height() == 0 ? 1 : size.height());
 }
 
 void* TextureImageTransportSurface::GetHandle() {
   return surface_.get() ? surface_->GetHandle() : NULL;
 }
 
 unsigned TextureImageTransportSurface::GetFormat() {
   return surface_.get() ? surface_->GetFormat() : 0;
 }
 
-void TextureImageTransportSurface::OnSetFrontSurfaceIsProtected(
-    bool is_protected, uint32 protection_state_id) {
-  protection_state_id_ = protection_state_id;
-  if (frontbuffer_is_protected_ == is_protected)
-    return;
-  frontbuffer_is_protected_ = is_protected;
-  AdjustFrontBufferAllocation();
-
-  // If surface is set to protected, and we haven't actually released it yet,
-  // we can set the ui surface handle now just by sending a swap message.
-  if (is_protected && textures_[front()].info->service_id() &&
-      textures_[front()].sent_to_client) {
-    GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params params;
-    params.surface_handle = textures_[front()].client_id;
-    params.size = textures_[front()].size;
-    params.protection_state_id = protection_state_id_;
-    params.skip_ack = true;
-    helper_->SendAcceleratedSurfaceBuffersSwapped(params);
-  }
-}
-
-void TextureImageTransportSurface::OnBufferPresented(bool presented,
+void TextureImageTransportSurface::OnBufferPresented(uint64 surface_handle,
                                                      uint32 sync_point) {
   if (sync_point == 0) {
-    BufferPresentedImpl(presented);
+    BufferPresentedImpl(surface_handle);
   } else {
     helper_->manager()->sync_point_manager()->AddSyncPointCallback(
         sync_point,
         base::Bind(&TextureImageTransportSurface::BufferPresentedImpl,
-                   this->AsWeakPtr(),
-                   presented));
+                   this,
+                   surface_handle));
   }
+
+  // Careful, we might get deleted now if we were only waiting for
+  // a final swap ACK.
+  Release();
 }
 
-void TextureImageTransportSurface::BufferPresentedImpl(bool presented) {
+void TextureImageTransportSurface::BufferPresentedImpl(uint64 surface_handle) {
+  DCHECK(!backbuffer_.service_id);
+  if (surface_handle) {
+    backbuffer_.identifier = surface_handle;
+    ConsumeTexture(backbuffer_);
+  } else {
+    // We didn't get back a texture, so allocate 'the other' buffer.
+    backbuffer_.identifier = (backbuffer_.identifier == 1) ? 2 : 1;
+  }
+
+  if (stub_destroyed_ && backbuffer_.service_id) {
+    GpuChannelManager* manager = helper_->manager();
+    DCHECK(manager);
+    if (manager->MakeCurrent(surface_.get()))
+      glDeleteTextures(1, &backbuffer_.service_id);

[piman, 2012/11/09 22:02:01]

Al is working on changes that will make it so that the service id is owned by
the mailbox, that will be owned by the client ids in the respective contexts, so
this can go away eventually. Add a TODO to remove?
As is, it could in theory leak GPU data if the GPU process gets in a state where
it doesn't have contexts any more (not in the Aura case, but you may want to
check Android behavior. I'm also planning on reusing this for Pepper in non-Aura
cases where it could happen).

[no sievers, 2012/11/19 20:30:44]

Ok, sounds good and like it also will render the AddRef() hack unnecessary then.

+
+    return;
+  }
+
   DCHECK(is_swap_buffers_pending_);
   is_swap_buffers_pending_ = false;
 
-  if (presented) {
-    // If we had not flipped, the two frame damage tracking is inconsistent.
-    // So conservatively take the whole frame.
-    if (!did_flip_)
-      previous_damage_rect_ = gfx::Rect(textures_[front()].size);
-  } else {
-    front_ = back();
-    previous_damage_rect_ = gfx::Rect(0, 0, 0, 0);
-  }
-
-  did_flip_ = presented;
-
   // We're relying on the fact that the parent context is
   // finished with it's context when it inserts the sync point that
   // triggers this callback.
   if (helper_->MakeCurrent()) {
-    if (textures_[front()].size != textures_[back()].size ||
-        !textures_[back()].info->service_id() ||
-        !textures_[back()].sent_to_client) {
+    if (backbuffer_.size != current_size_ ||
+        !backbuffer_.service_id) {
       // We may get an ACK from a stale swap just to reschedule.  In that case,
       // we may not have a backbuffer suggestion and should not recreate one.
       if (backbuffer_suggested_allocation_)
-        CreateBackTexture(textures_[front()].size);
+        CreateBackTexture();
     } else {
       AttachBackTextureToFBO();
     }
   }
 
   // Even if MakeCurrent fails, schedule anyway, to trigger the lost context
   // logic.
   if (did_unschedule_) {
     did_unschedule_ = false;
     helper_->SetScheduled(true);
   }
 }
 
 void TextureImageTransportSurface::OnResizeViewACK() {
   NOTREACHED();
 }
 
-void TextureImageTransportSurface::ReleaseTexture(int id) {
-  if (!parent_stub_)
+void TextureImageTransportSurface::ReleaseBackBuffer() {
+  if (!backbuffer_.service_id)
     return;
-  Texture& texture = textures_[id];
-  TextureInfo* info = texture.info;
-  DCHECK(info);
-
-  GLuint service_id = info->service_id();
-  if (!service_id)
-    return;
-  info->SetServiceId(0);
 
   {
     ScopedFrameBufferBinder fbo_binder(fbo_id_);

[jonathan.backer, 2012/11/12 16:52:15]

Why the framebuffer binder? Seems like a no-op. Maybe we also want a
glFramebufferTexture2D to release the backbuffer from the fbo?

[no sievers, 2012/11/19 20:30:44]

Done.

-    glDeleteTextures(1, &service_id);
+    glDeleteTextures(1, &backbuffer_.service_id);
+    backbuffer_.service_id = 0;
   }
   glFlush();
   CHECK_GL_ERROR();
 }
 
-void TextureImageTransportSurface::CreateBackTexture(const gfx::Size& size) {
-  if (!parent_stub_)
-    return;
-  Texture& texture = textures_[back()];
-  TextureInfo* info = texture.info;
-  DCHECK(info);
+void TextureImageTransportSurface::CreateBackTexture() {
+  // We are waiting for our backbuffer in the mailbox, so we shouldn't be
+  // reallocating the backbuffer now.

[jonathan.backer, 2012/11/12 16:52:15]

comment nit: "If |is_swap_buffers_pending|, we are waiting..."

[no sievers, 2012/11/19 20:30:44]

Done.

+  DCHECK(!is_swap_buffers_pending_);
 
-  GLuint service_id = info->service_id();
+  gfx::Size& size = current_size_;

[jonathan.backer, 2012/11/12 16:52:15]

Got tired of typing?

[no sievers, 2012/11/19 20:30:44]

Done.

+  Texture& texture = backbuffer_;

[piman, 2012/11/09 22:02:01]

nit: you have a single texture now, so that indirection isn't really necessary
anymore (i.e. can use backbuffer_ and current_size_ directly below).

[no sievers, 2012/11/19 20:30:44]

Done.

 
-  if (service_id && texture.size == size && texture.sent_to_client)
+  if (texture.service_id && texture.size == size)
     return;
 
-  if (!service_id) {
-    glGenTextures(1, &service_id);
-    info->SetServiceId(service_id);
+  if (!texture.service_id) {

[piman, 2012/11/09 22:02:01]

nit: no need for braces any more.

[no sievers, 2012/11/19 20:30:44]

Done.

+    glGenTextures(1, &texture.service_id);
   }
 
   if (size != texture.size) {
     texture.size = size;

[jonathan.backer, 2012/11/12 16:52:15]

nit: no if necessary.

[no sievers, 2012/11/19 20:30:44]

Done.

-    TextureManager* texture_manager =
-        parent_stub_->decoder()->GetContextGroup()->texture_manager();
-    texture_manager->SetLevelInfo(
-        info,
-        GL_TEXTURE_2D,
-        0,
-        GL_RGBA,
-        size.width(),
-        size.height(),
-        1,
-        0,
-        GL_RGBA,
-        GL_UNSIGNED_BYTE,
-        true);
   }
 
   {
-    ScopedTextureBinder texture_binder(service_id);
+    ScopedTextureBinder texture_binder(texture.service_id);
     glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
     glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
     glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA,
         size.width(), size.height(), 0,
         GL_RGBA, GL_UNSIGNED_BYTE, NULL);
     CHECK_GL_ERROR();
   }
 
   AttachBackTextureToFBO();
 
+  MailboxName mailbox_name;
+  mailbox_manager_->GenerateMailboxName(&mailbox_name);
+  mailbox_names_[texture.identifier] = mailbox_name;

[piman, 2012/11/09 22:02:01]

Al can confirm but I don't think it's ok to change the mailbox name associated
with a service id, so you probably want to do this lazily.

[no sievers, 2012/11/19 20:30:44]

Done.

+
   GpuHostMsg_AcceleratedSurfaceNew_Params params;
   params.width = size.width();
   params.height = size.height();
-  params.surface_handle = texture.client_id;
+  params.surface_handle = texture.identifier;
+  params.mailbox_name.resize(sizeof(mailbox_name));
+  memcpy(params.mailbox_name.data(), &mailbox_name, sizeof(mailbox_name));
   helper_->SendAcceleratedSurfaceNew(params);
-  texture.sent_to_client = true;
 }
 
 void TextureImageTransportSurface::AttachBackTextureToFBO() {
-  if (!parent_stub_)
-    return;
-  TextureInfo* info = textures_[back()].info;
-  DCHECK(info);
-
+  DCHECK(backbuffer_.service_id);
   ScopedFrameBufferBinder fbo_binder(fbo_id_);
   glFramebufferTexture2DEXT(GL_FRAMEBUFFER,
       GL_COLOR_ATTACHMENT0,
       GL_TEXTURE_2D,
-      info->service_id(),
+      backbuffer_.service_id,
       0);
   glFlush();
   CHECK_GL_ERROR();
 
 #ifndef NDEBUG
   GLenum status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER);
   if (status != GL_FRAMEBUFFER_COMPLETE) {
-    DLOG(ERROR) << "Framebuffer incomplete.";
+    DLOG(ERROR) << "Framebuffer incomplete: " << status;
+    DCHECK(false);

[piman, 2012/11/09 22:02:01]

nit: just make the above DLOG(FATAL)?

[no sievers, 2012/11/19 20:30:44]

Done.

   }
 #endif
 }
 
-void TextureImageTransportSurface::ReleaseParentStub() {
-  DCHECK(parent_stub_);
-  parent_stub_->RemoveDestructionObserver(this);
-  for (int i = 0; i < 2; ++i) {
-    Texture& texture = textures_[i];
-    texture.info = NULL;
-    if (!texture.sent_to_client)
-      continue;
-    GpuHostMsg_AcceleratedSurfaceRelease_Params params;
-    params.identifier = texture.client_id;
-    helper_->SendAcceleratedSurfaceRelease(params);

[piman, 2012/11/09 22:02:01]

This should still happen somewhere. If we switch off compositing (it can still
happen under some conditions), we need to tell the RWHV* to release its data
structures associated with the surface.

+void TextureImageTransportSurface::ConsumeTexture(Texture& texture) {
+  DCHECK(!texture.service_id);
+  DCHECK(texture.identifier == 1 || texture.identifier == 2);
+
+  scoped_ptr<TextureDefinition> definition(mailbox_manager_->ConsumeTexture(
+      GL_TEXTURE_2D, mailbox_names_[texture.identifier]));
+  if (definition.get()) {
+    texture.service_id = definition->ReleaseServiceId();
+    texture.size = gfx::Size(definition->level_infos()[0][0].width,
+                             definition->level_infos()[0][0].height);
   }
-  parent_stub_ = NULL;
+}
+
+void TextureImageTransportSurface::ProduceTexture(Texture& texture) {
+  DCHECK(texture.service_id);
+  DCHECK(texture.identifier == 1 || texture.identifier == 2);
+  TextureManager* texture_manager =
+      helper_->stub()->decoder()->GetContextGroup()->texture_manager();
+  DCHECK(texture.size.width() > 0 && texture.size.height() > 0);
+  TextureDefinition::LevelInfo info(
+      GL_TEXTURE_2D, GL_RGBA, texture.size.width(), texture.size.height(), 1,
+      0, GL_RGBA, GL_UNSIGNED_BYTE, true);
+
+  TextureDefinition::LevelInfos level_infos;
+  level_infos.resize(1);
+  level_infos[0].resize(texture_manager->MaxLevelsForTarget(GL_TEXTURE_2D));
+  level_infos[0][0] = info;
+  scoped_ptr<TextureDefinition> definition(new TextureDefinition(
+      GL_TEXTURE_2D,
+      texture.service_id,
+      true,
+      level_infos));
+  mailbox_manager_->ProduceTexture(
+      GL_TEXTURE_2D,
+      mailbox_names_[texture.identifier],
+      definition.release(),
+      helper_->stub()->decoder()->GetContextGroup()->texture_manager());
+  texture.service_id = 0;
 }
 
 }  // namespace content