ReleaseOutputBuffer to surface back and front buffers where possible.

content/common/gpu/media/avda_codec_image.cc

 // Copyright 2015 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/common/gpu/media/avda_codec_image.h"
 
 #include <string.h>
 
 #include <memory>
 
@@ skipping 93 matching lines @@
                                           gfx::OverlayTransform transform,
                                           const gfx::Rect& bounds_rect,
                                           const gfx::RectF& crop_rect) {
   // This should only be called when we're rendering to a SurfaceView.
   if (surface_texture_) {
     DVLOG(1) << "Invalid call to ScheduleOverlayPlane; this image is "
                 "SurfaceTexture backed.";
     return false;
   }
 
-  if (codec_buffer_index_ != kInvalidCodecBufferIndex) {
-    media_codec_->ReleaseOutputBuffer(codec_buffer_index_, true);
-    codec_buffer_index_ = kInvalidCodecBufferIndex;
-  }
+  ReleaseOutputBuffer(ReleaseMode::RENDER_ONLY);

[liberato (no reviews please), 2016/04/22 00:51:06]

RENDER_AND_UPDATE, else it will try to wait for frame available.  or, if you
like my new naming scheme, RENDER_TO_FRONT_BUFFER.

[DaleCurtis, 2016/04/22 01:33:58]

Done.

+  codec_buffer_index_ = kInvalidCodecBufferIndex;
   return true;
 }
 
 void AVDACodecImage::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
                                   uint64_t process_tracing_id,
                                   const std::string& dump_name) {}
 
 void AVDACodecImage::UpdateSurfaceTexture(RestoreBindingsMode mode) {
   DCHECK(surface_texture_);
 
-  // Render via the media codec if needed.
   if (!IsCodecBufferOutstanding())
     return;
 
-  // The decoder buffer is still pending.
-  // This must be synchronous, so wait for OnFrameAvailable.
-  media_codec_->ReleaseOutputBuffer(codec_buffer_index_, true);
-  {
-    SCOPED_UMA_HISTOGRAM_TIMER("Media.AvdaCodecImage.WaitTimeForFrame");
-    shared_state_->WaitForFrameAvailable();
-  }
-
-  // Don't bother to check if we're rendered again.
+  ReleaseOutputBuffer(ReleaseMode::RENDER_ONLY);
+  DCHECK_EQ(codec_buffer_index_, kUpdateOnly);
   codec_buffer_index_ = kInvalidCodecBufferIndex;
 
   // Swap the rendered image to the front.
   std::unique_ptr<ui::ScopedMakeCurrent> scoped_make_current =
       MakeCurrentIfNeeded();
 
   // If we changed contexts, then we always want to restore it, since the caller
   // doesn't know that we're switching contexts.
   if (scoped_make_current)
     mode = kDoRestoreBindings;
 
   // Save the current binding if requested.
   GLint bound_service_id = 0;
   if (mode == kDoRestoreBindings)
     glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, &bound_service_id);
 
   surface_texture_->UpdateTexImage();
   if (mode == kDoRestoreBindings)
     glBindTexture(GL_TEXTURE_EXTERNAL_OES, bound_service_id);
 
   // Helpfully, this is already column major.
   surface_texture_->GetTransformMatrix(gl_matrix_);
 }
 
 void AVDACodecImage::SetMediaCodecBufferIndex(int buffer_index) {
   codec_buffer_index_ = buffer_index;
 }
 
-int AVDACodecImage::GetMediaCodecBufferIndex() const {
-  return codec_buffer_index_;
-}
-
 void AVDACodecImage::SetSize(const gfx::Size& size) {
   size_ = size;
 }
 
+void AVDACodecImage::ReleaseOutputBuffer(ReleaseMode release_mode) {
+  if (!IsCodecBufferOutstanding())
+    return;
+
+  DCHECK_NE(codec_buffer_index_, kInvalidCodecBufferIndex);
+
+  if (release_mode == ReleaseMode::SKIP_RENDER) {
+    if (codec_buffer_index_ != kUpdateOnly)
+      media_codec_->ReleaseOutputBuffer(codec_buffer_index_, false);
+    codec_buffer_index_ = kInvalidCodecBufferIndex;
+    return;
+  }
+
+  if (release_mode == ReleaseMode::RENDER_ONLY) {

[liberato (no reviews please), 2016/04/22 00:51:06]

DCHECK(surface_texture_)

[DaleCurtis, 2016/04/22 01:33:58]

Done.

+    if (codec_buffer_index_ == kUpdateOnly)
+      return;
+
+    media_codec_->ReleaseOutputBuffer(codec_buffer_index_, true);
+    codec_buffer_index_ = kUpdateOnly;
+
+    // This must be synchronous, so wait for OnFrameAvailable.
+    SCOPED_UMA_HISTOGRAM_TIMER("Media.AvdaCodecImage.WaitTimeForFrame");
+    shared_state_->WaitForFrameAvailable();
+    return;
+  }
+
+  DCHECK(release_mode == ReleaseMode::RENDER_AND_UPDATE);
+
+  if (!surface_texture_) {
+    DCHECK_NE(codec_buffer_index_, kUpdateOnly);
+    media_codec_->ReleaseOutputBuffer(codec_buffer_index_, true);
+    codec_buffer_index_ = kInvalidCodecBufferIndex;
+    return;
+  }
+
+  // Surface texture is already attached, so just update it.
+  if (shared_state_->surface_texture_is_attached()) {
+    // Warning: This will cause reentrancy to this function.
+    UpdateSurfaceTexture(kDoRestoreBindings);

[liberato (no reviews please), 2016/04/22 00:51:06]

i think that this can be simplified to avoid the mutual recursion.  in the new
world:

this fn handles rendering only, and succeeds silently with kUpdateOnly if it's
already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
ReleaseOutputBuffer, and, depending on the release mode, does the update.

UpdateSurfaceTexture can handle just the context switch and UpdateTexImage call.
 renaming to PromoteBackBuffer or similar would be clearer of this new function.

finally, add a new function UpdateSurface(render_mode, restore_bindings_mode)". 
this function would call Render, then optionally Promote.  it would also own the
code to temporarily attach the surface texture.

all existing calls to UpdateSurfaceTexture and Render would become calls to
UpdateSurface, i think.  default is "do restore bindings", so the outside
doesn't have to care.

[DaleCurtis, 2016/04/22 01:33:58]

I agree the recursion is nasty, will think on it some more. I don't completely
follow your suggestion though. What is the public AVDACodecImage interface for
the deferred strategy to call when it wants to render to the front buffer now?
It seems odd to leak the bindings mode outside of the image.

[liberato (no reviews please), 2016/04/22 02:16:43]

the public api would be UpdateSurface().  i agree about not leaking the bindings
parameter.  that's what i meant about having a default parameter. 
alternatively, public UpdateSurface takes only the renderMode flag, and
protected UpdateSurface takes both.

the deferred strategy would call UpdateSurface(render mode), the same as it does
now with a new name.  UpdateSurface would do something approximately like this:

if (buffer == kInvalidCodecBUffer)
  return;  // success -- copyTexImage gets here!

// move the codec buffer to the surface.
// does nothing if already kUpdateOnly
// switches to kUpdateOnly if ST.
// switches to kInvalidCodecBuffer if SV.
RenderOutputBuffer(render_mode);

if (render_mode != RENDER_TO_FRONT_BUFFER || !surface_texture_)
  return;

if (attached)  {
  PromoteBackBuffer(doRestoreBindings);
  return;
}

// generate fake texture, attach, dontRestoreBindings, detach

}

actually, i guess that it hides the part about restoring bindings too.

+    return;
+  }
+
+  // Don't attach the surface texture permanently.  Perhaps we should just
+  // attach the surface texture in avda and be done with it.
+  GLuint service_id = 0;
+  glGenTextures(1, &service_id);
+  GLint bound_service_id = 0;
+  glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, &bound_service_id);
+  glBindTexture(GL_TEXTURE_EXTERNAL_OES, service_id);
+  AttachSurfaceTextureToContext();
+
+  // Warning: This will cause reentrancy to this function.
+  UpdateSurfaceTexture(kDontRestoreBindings);
+
+  // Detach the surface texture, which deletes the generated texture.
+  surface_texture_->DetachFromGLContext();
+  shared_state_->DidDetachSurfaceTexture();
+  glBindTexture(GL_TEXTURE_EXTERNAL_OES, bound_service_id);
+}
+
 void AVDACodecImage::SetMediaCodec(media::MediaCodecBridge* codec) {
   media_codec_ = codec;
-  codec_buffer_index_ = -1;
+  codec_buffer_index_ = kInvalidCodecBufferIndex;
 }
 
 void AVDACodecImage::SetTexture(gpu::gles2::Texture* texture) {
   texture_ = texture;
 }
 
 void AVDACodecImage::AttachSurfaceTextureToContext() {
   DCHECK(surface_texture_);
 
   // We assume that the currently bound texture is the intended one.
@@ skipping 17 matching lines @@
   std::unique_ptr<ui::ScopedMakeCurrent> scoped_make_current;
   if (!shared_state_->context()->IsCurrent(NULL)) {
     scoped_make_current.reset(new ui::ScopedMakeCurrent(
         shared_state_->context(), shared_state_->surface()));
   }
 
   return scoped_make_current;
 }
 
 void AVDACodecImage::GetTextureMatrix(float matrix[16]) {
-  if (IsCodecBufferOutstanding() && shared_state_ && surface_texture_) {
-    // Our current matrix may be stale.  Update it if possible.
-    if (!shared_state_->surface_texture_is_attached()) {
-      // Don't attach the surface texture permanently.  Perhaps we should
-      // just attach the surface texture in avda and be done with it.
-      GLuint service_id = 0;
-      glGenTextures(1, &service_id);
-      GLint bound_service_id = 0;
-      glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, &bound_service_id);
-      glBindTexture(GL_TEXTURE_EXTERNAL_OES, service_id);
-      AttachSurfaceTextureToContext();
-      UpdateSurfaceTexture(kDontRestoreBindings);
-      // Detach the surface texture, which deletes the generated texture.
-      surface_texture_->DetachFromGLContext();
-      shared_state_->DidDetachSurfaceTexture();
-      glBindTexture(GL_TEXTURE_EXTERNAL_OES, bound_service_id);
-    } else {
-      // Surface texture is already attached, so just update it.
-      UpdateSurfaceTexture(kDoRestoreBindings);
-    }
-  }
-
+  // Our current matrix may be stale.  Update it if possible.
+  ReleaseOutputBuffer(ReleaseMode::RENDER_AND_UPDATE);

[liberato (no reviews please), 2016/04/22 00:51:06]

this eats the check for |surface_texture_|.  should still early out, or return
identity or something.

[DaleCurtis, 2016/04/22 01:33:58]

Done.

   memcpy(matrix, gl_matrix_, sizeof(gl_matrix_));
 }
 
 bool AVDACodecImage::IsCodecBufferOutstanding() const {
   return codec_buffer_index_ != kInvalidCodecBufferIndex && media_codec_;
 }
 
 }  // namespace content