Always call OMX_FreeBuffer(), even in ERRORING.

content/common/gpu/media/omx_video_decode_accelerator.cc

 // Copyright (c) 2012 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/omx_video_decode_accelerator.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 452 matching lines @@
 
   DCHECK(current_state_change_ == NO_TRANSITION ||
          current_state_change_ == FLUSHING ||
          current_state_change_ == RESETTING) << current_state_change_;
 
   // If we were never initializeed there's no teardown to do.
   if (client_state_ == OMX_StateMax)
     return;
   // If we can already call OMX_FreeHandle, simply do so.
   if (client_state_ == OMX_StateInvalid || client_state_ == OMX_StateLoaded) {
     ShutdownComponent();

[Pawel Osciak, 2012/08/14 22:17:14]

Can we get here without freeing the buffers first? If so, I think we should free
them here.

[Ami GONE FROM CHROMIUM, 2012/08/15 04:08:30]

No; Invalid & Loaded can only be reached through ERRORING or DESTROYING, or by
never have been initialized (see the state machine dispatcher below).

     return;
   }
   DCHECK(client_state_ == OMX_StateExecuting ||
          client_state_ == OMX_StateIdle ||
          client_state_ == OMX_StatePause);
   current_state_change_ = DESTROYING;
   client_ = NULL;
   BeginTransitionToState(OMX_StateIdle);
   BusyLoopInDestroying(deleter.Pass());
 }
@@ skipping 114 matching lines @@
          client_state_ == OMX_StateIdle ||
          client_state_ == OMX_StatePause);
   client_state_ = OMX_StateIdle;
 
   // Note that during the Executing -> Idle transition, the OMX spec guarantees
   // buffers have been returned to the client, so we don't need to do an
   // explicit FlushIOPorts().
 
   BeginTransitionToState(OMX_StateLoaded);
 
-  // TODO(fischman): evaluate what these conditionals are doing.  What happens
-  // if they're false??
-  if (!input_buffers_at_component_)
-    FreeInputBuffers();
-  if (!output_buffers_at_component_)
-    FreeOutputBuffers();
+  FreeOMXBuffers();
 }
 
 void OmxVideoDecodeAccelerator::OnReachedLoadedInDestroying() {
   DCHECK_EQ(client_state_, OMX_StateIdle);
   client_state_ = OMX_StateLoaded;
   current_state_change_ = NO_TRANSITION;
   ShutdownComponent();

[Pawel Osciak, 2012/08/14 22:17:14]

Same here, can we get here without freeing buffers?

[Ami GONE FROM CHROMIUM, 2012/08/15 04:08:30]

No, the only way to get to Loaded is through OnReachedIdleInDestroying(), which
calls FOB().

 }
 
 void OmxVideoDecodeAccelerator::OnReachedInvalidInErroring() {
   client_state_ = OMX_StateInvalid;
+  FreeOMXBuffers();
   ShutdownComponent();
 }
 
 void OmxVideoDecodeAccelerator::ShutdownComponent() {
   OMX_ERRORTYPE result = omx_free_handle(component_handle_);
   if (result != OMX_ErrorNone)
     DLOG(ERROR) << "OMX_FreeHandle() error. Error code: " << result;
   client_state_ = OMX_StateMax;
   omx_deinit();
   // Allow BusyLoopInDestroying to exit and delete |this|.
@@ skipping 73 matching lines @@
     RETURN_ON_OMX_FAILURE(result, "OMX_UseEGLImage", PLATFORM_FAILURE, false);
     // Here we set a garbage bitstream buffer id, and then overwrite it before
     // passing to PictureReady.
     int garbage_bitstream_buffer_id = -1;
     (*omx_buffer)->pAppPrivate =
         new media::Picture(picture_buffer.id(), garbage_bitstream_buffer_id);
   }
   return true;
 }
 
-void OmxVideoDecodeAccelerator::FreeInputBuffers() {
+void OmxVideoDecodeAccelerator::FreeOMXBuffers() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
-  // Calls to OMX to free buffers.
-  OMX_ERRORTYPE result;
-  OMX_BUFFERHEADERTYPE* omx_buffer;
   while (!free_input_buffers_.empty()) {
-    omx_buffer = free_input_buffers_.front();
+    OMX_BUFFERHEADERTYPE* omx_buffer = free_input_buffers_.front();
     free_input_buffers_.pop();
-    result = OMX_FreeBuffer(component_handle_, input_port_, omx_buffer);
+    OMX_ERRORTYPE result =
+        OMX_FreeBuffer(component_handle_, input_port_, omx_buffer);
     RETURN_ON_OMX_FAILURE(result, "OMX_FreeBuffer", PLATFORM_FAILURE,);
   }
-}
-
-void OmxVideoDecodeAccelerator::FreeOutputBuffers() {
-  DCHECK_EQ(message_loop_, MessageLoop::current());
-  // Calls to OMX to free buffers.
-  OMX_ERRORTYPE result;
-
   for (OutputPictureById::iterator it = pictures_.begin();
        it != pictures_.end(); ++it) {
     OMX_BUFFERHEADERTYPE* omx_buffer = it->second.omx_buffer_header;
     DCHECK(omx_buffer);
     delete reinterpret_cast<media::Picture*>(omx_buffer->pAppPrivate);
-    result = OMX_FreeBuffer(component_handle_, output_port_, omx_buffer);
+    OMX_ERRORTYPE result =
+        OMX_FreeBuffer(component_handle_, output_port_, omx_buffer);
     RETURN_ON_OMX_FAILURE(result, "OMX_FreeBuffer", PLATFORM_FAILURE,);

[marcheu, 2012/08/14 22:11:25]

Can we keep iterating even if we fail a FreeBuffer call? That way we can still
try to free the other pictures, and destroy the eglImages.

[Ami GONE FROM CHROMIUM, 2012/08/15 04:08:30]

I really dislike this sort of programming.
I guess you're going for belt&suspenders here, but I don't like writing code
which "should" never get run.  Do you know of any concrete ways in which an
OMX_FreeBuffer call can fail and yet have further OMX_FB calls be useful, or the
DestroyEglImage below be useful?

At any rate, done.

     texture_to_egl_image_translator_->DestroyEglImage(egl_display_,
                                                       it->second.egl_image);
     if (client_)
       client_->DismissPictureBuffer(it->first);
   }
   pictures_.clear();
 }
 
 void OmxVideoDecodeAccelerator::OnOutputPortDisabled() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
@@ skipping 349 matching lines @@
 
 bool OmxVideoDecodeAccelerator::SendCommandToPort(
     OMX_COMMANDTYPE cmd, int port_index) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   OMX_ERRORTYPE result = OMX_SendCommand(component_handle_,
                                          cmd, port_index, 0);
   RETURN_ON_OMX_FAILURE(result, "SendCommand() failed" << cmd,
                         PLATFORM_FAILURE, false);
   return true;
 }