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

content/common/gpu/media/vaapi_h264_decoder.cc

Index: content/common/gpu/media/vaapi_h264_decoder.cc
diff --git a/content/common/gpu/media/vaapi_h264_decoder.cc b/content/common/gpu/media/vaapi_h264_decoder.cc
index cb716b29fb75c32969b160cbd644919ceb60394d..f602a889c2852b4b980d8495bb66fe8ec69a85a4 100644
--- a/content/common/gpu/media/vaapi_h264_decoder.cc
+++ b/content/common/gpu/media/vaapi_h264_decoder.cc
@@ -1021,50 +1021,52 @@ bool VaapiH264Decoder::DecodePicture() {
                                        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;