VAVDA: Destroy vaapi buffers after we're done drawing.

content/common/gpu/media/vaapi_h264_decoder.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 <dlfcn.h>
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/stl_util.h"
@@ skipping 1003 matching lines @@
   DecodeSurface* dec_surface =
       poc_to_decode_surfaces_[curr_pic_->pic_order_cnt];
   DVLOG(4) << "Decoding POC " << curr_pic_->pic_order_cnt
            << " into surface " << dec_surface->va_surface_id();
 
   // Get ready to decode into surface.
   VAStatus va_res = VAAPI_BeginPicture(va_display_, va_context_id_,
                                        dec_surface->va_surface_id());
   VA_SUCCESS_OR_RETURN(va_res, "vaBeginPicture failed", false);
 
-  // Put buffer IDs for pending parameter buffers into buffers[].
-  VABufferID buffers[kMaxVABuffers];
-  size_t num_buffers = pending_va_bufs_.size();
-  for (size_t i = 0; i < num_buffers && i < kMaxVABuffers; ++i) {
-    buffers[i] = pending_va_bufs_.front();
+  // Put buffer IDs for pending parameter buffers into va_buffers[].
+  VABufferID va_buffers[kMaxVABuffers];
+  size_t num_va_buffers = pending_va_bufs_.size();
+  for (size_t i = 0; i < num_va_buffers && i < kMaxVABuffers; ++i) {
+    va_buffers[i] = pending_va_bufs_.front();
     pending_va_bufs_.pop();
   }
 
   // And send them to the HW decoder.
-  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, buffers,
-                               num_buffers);
+  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, va_buffers,
+                               num_va_buffers);
   VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for va_bufs failed", false);

[piman, 2012/08/02 23:36:42]

Can't we have a leak here if we return early because of error?

[Pawel Osciak, 2012/08/03 02:13:20]

TL;DR: yes, and everywhere else you mentioned too.

This will cause us to return false, which will be propagated all the way up to
VaapiVideoDecodeAccelerator class, which will get kDecodeError and will notify
the VDA client of the error and Cleanup() itself, which will result in
Destroy()ing this class as well, which frees all VA surfaces, and calls
vaDestroyConfig() and vaTerminate(). vaTerminate() will free the buffers
allocated with vaCreateBuffer by destroying their buffer_heap (and other heaps
too) in i965_Terminate.

 
-  DVLOG(4) << "Committed " << num_buffers << "VA buffers";
+  DVLOG(4) << "Committed " << num_va_buffers << "VA buffers";
 
-  for (size_t i = 0; i < num_buffers; ++i) {
-    va_res = VAAPI_DestroyBuffer(va_display_, buffers[i]);
-    VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for va_bufs failed", false);
-  }
-
-  // Put buffer IDs for pending slice data buffers into buffers[].
-  num_buffers = pending_slice_bufs_.size();
-  for (size_t i = 0; i < num_buffers && i < kMaxVABuffers; ++i) {
-    buffers[i] = pending_slice_bufs_.front();
+  // Put buffer IDs for pending slice data buffers into slice_buffers[].
+  VABufferID slice_buffers[kMaxVABuffers];
+  size_t num_slice_buffers = pending_slice_bufs_.size();
+  for (size_t i = 0; i < num_slice_buffers && i < kMaxVABuffers; ++i) {
+    slice_buffers[i] = pending_slice_bufs_.front();
     pending_slice_bufs_.pop();
   }
 
   // And send them to the Hw decoder.
-  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, buffers,
-                               num_buffers);
+  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, slice_buffers,
+                               num_slice_buffers);
   VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for slices failed", false);

[piman, 2012/08/02 23:36:42]

Same here?

 
-  DVLOG(4) << "Committed " << num_buffers << "slice buffers";
-
-  for (size_t i = 0; i < num_buffers; ++i) {
-    va_res = VAAPI_DestroyBuffer(va_display_, buffers[i]);
-    VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for slices failed", false);
-  }
+  DVLOG(4) << "Committed " << num_slice_buffers << "slice buffers";
 
   // Instruct HW decoder to start processing committed buffers (decode this
   // picture). This does not block until the end of decode.
   va_res = VAAPI_EndPicture(va_display_, va_context_id_);
   VA_SUCCESS_OR_RETURN(va_res, "vaEndPicture failed", false);

[piman, 2012/08/02 23:36:42]

And here

 
+  // Now that EndPicture has passed we can destroy our buffers.
+  for (size_t i = 0; i < num_va_buffers; ++i) {
+    va_res = VAAPI_DestroyBuffer(va_display_, va_buffers[i]);
+    VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for va_bufs failed", false);

[piman, 2012/08/02 23:36:42]

And here

+  }
+
+  for (size_t i = 0; i < num_slice_buffers; ++i) {
+    va_res = VAAPI_DestroyBuffer(va_display_, slice_buffers[i]);
+    VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for slices failed", false);

[piman, 2012/08/02 23:36:42]

And here

+  }
+
   // Used to notify clients that we had sufficient data to start decoding
   // a new frame.
   frame_ready_at_hw_ = true;
   return true;
 }
 
 
 bool VaapiH264Decoder::InitCurrPicture(H264SliceHeader* slice_hdr) {
   DCHECK(curr_pic_.get());
 
@@ skipping 1014 matching lines @@
     }
   }
 }
 
 // static
 size_t VaapiH264Decoder::GetRequiredNumOfPictures() {
   return kNumReqPictures;
 }
 
 }  // namespace content