ReleaseOutputBuffer to surface back and front buffers where possible.

content/common/gpu/media/avda_codec_image.cc

Index: content/common/gpu/media/avda_codec_image.cc
diff --git a/content/common/gpu/media/avda_codec_image.cc b/content/common/gpu/media/avda_codec_image.cc
index 8f3595a69c91496dd01dbfc098f03a543b8b08b7..7302bd1cbccd0f1d8588a6f19a496dbb9e7785be 100644
--- a/content/common/gpu/media/avda_codec_image.cc
+++ b/content/common/gpu/media/avda_codec_image.cc
@@ -111,10 +111,8 @@ bool AVDACodecImage::ScheduleOverlayPlane(gfx::AcceleratedWidget widget,
     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;
 }
 
@@ -125,19 +123,11 @@ void AVDACodecImage::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
 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.
@@ -166,17 +156,73 @@ 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) {
@@ -214,28 +260,8 @@ std::unique_ptr<ui::ScopedMakeCurrent> AVDACodecImage::MakeCurrentIfNeeded() {
 }
 
 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_));
 }