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 13 matching lines @@
 
 #define VA_SUCCESS_OR_RETURN(va_res, err_msg, ret)         \
   do {                                                     \
     if ((va_res) != VA_STATUS_SUCCESS) {                   \
       DVLOG(1) << err_msg                                  \
                << " VA error: " << VAAPI_ErrorStr(va_res); \
       return (ret);                                        \
     }                                                      \
   } while (0)
 
+#define VA_ERROR_RETURN(va_res, err_msg, ret)              \

[piman, 2012/08/03 03:55:40]

TBH, I see the value in the macro that logs things, but I don't see the value in
folding the return into here.
It'd be better to be explicit at the calling sites:

VA_LOG_ERROR(va_res, "error");
return false;

+  do {                                                     \
+    DVLOG(1) << err_msg                                  \
+             << " VA error: " << VAAPI_ErrorStr(va_res); \
+    return (ret);                                        \
+  } while (0)
+
 namespace content {
 
 void *vaapi_handle = dlopen("libva.so", RTLD_NOW);
 void *vaapi_x11_handle = dlopen("libva-x11.so", RTLD_NOW);
 
 typedef VADisplay (*VaapiGetDisplay)(Display *dpy);
 typedef int (*VaapiDisplayIsValid)(VADisplay dpy);
 typedef VAStatus (*VaapiInitialize)(VADisplay dpy,
                                     int *major_version,
                                     int *minor_version);
@@ skipping 320 matching lines @@
   frame_num_ = 0;
   prev_frame_num_ = -1;
   prev_frame_num_offset_ = -1;
 
   prev_ref_has_memmgmnt5_ = false;
   prev_ref_top_field_order_cnt_ = -1;
   prev_ref_pic_order_cnt_msb_ = -1;
   prev_ref_pic_order_cnt_lsb_ = -1;
   prev_ref_field_ = H264Picture::FIELD_NONE;
 
+  // When called from the constructor, although va_display_ is invalid,
+  // |pending_slice_bufs_| and |pending_va_bufs_| are empty.
+  while(!pending_slice_bufs_.empty()) {

[piman, 2012/08/03 03:55:40]

nit: space before (

+    VABufferID buffer = pending_slice_bufs_.front();
+    VAStatus va_res = VAAPI_DestroyBuffer(va_display_, buffer);
+    VA_LOG_ON_ERROR(va_res, "vaDestroyBuffer failed");
+    pending_slice_bufs_.pop();
+  }
+  while(!pending_va_bufs_.empty()) {

[piman, 2012/08/03 03:55:40]

nit: space before (

+    VABufferID buffer = pending_va_bufs_.front();
+    VAStatus va_res = VAAPI_DestroyBuffer(va_display_, buffer);
+    VA_LOG_ON_ERROR(va_res, "vaDestroyBuffer failed");
+    pending_va_bufs_.pop();
+  }
+
   pending_slice_bufs_ = std::queue<VABufferID>();
   pending_va_bufs_ = std::queue<VABufferID>();
 
   ref_pic_list0_.clear();
   ref_pic_list1_.clear();
 
   for (POCToDecodeSurfaces::iterator it = poc_to_decode_surfaces_.begin();
        it != poc_to_decode_surfaces_.end(); ) {
     int poc = it->second->poc();
     // Must be incremented before UnassignSurfaceFromPoC as this call
@@ skipping 27 matching lines @@
     case kError:
       destroy_surfaces = true;
       // fallthrough
     case kInitialized:
       if (!make_context_current_.Run())
         break;
       if (destroy_surfaces)
         DestroyVASurfaces();
       va_res = VAAPI_DestroyConfig(va_display_, va_config_id_);
       VA_LOG_ON_ERROR(va_res, "vaDestroyConfig failed");
-       va_res = VAAPI_Terminate(va_display_);
-       VA_LOG_ON_ERROR(va_res, "vaTerminate failed");
+      va_res = VAAPI_Terminate(va_display_);
+      VA_LOG_ON_ERROR(va_res, "vaTerminate failed");
       // fallthrough
     case kUninitialized:
       break;
   }
 
   state_ = kUninitialized;
 }
 
 // Maps Profile enum values to VaProfile values.
 bool VaapiH264Decoder::SetProfile(media::VideoCodecProfile profile) {
@@ skipping 160 matching lines @@
                                          GetRequiredNumOfPictures(),
                                          va_surface_ids_);
   VA_SUCCESS_OR_RETURN(va_res, "vaCreateSurfaces failed", false);
 
   DCHECK(decode_surfaces_.empty());
   // And create a context associated with them.
   va_res = VAAPI_CreateContext(va_display_, va_config_id_,
                                pic_width_, pic_height_, VA_PROGRESSIVE,
                                va_surface_ids_, GetRequiredNumOfPictures(),
                                &va_context_id_);
-  VA_SUCCESS_OR_RETURN(va_res, "vaCreateContext failed", false);
+
+  if (va_res != VA_STATUS_SUCCESS) {
+    VAAPI_DestroySurfaces(va_display_, va_surface_ids_,
+                          GetRequiredNumOfPictures());
+    DVLOG(1) << "vaCreateContext failed"
+             << " VA error: " << VAAPI_ErrorStr(va_res);
+    return false;
+  }
 
   return true;
 }
 
 void VaapiH264Decoder::DestroyVASurfaces() {
   DCHECK(state_ == kDecoding || state_ == kError || state_ == kAfterReset);
   decode_surfaces_.clear();
 
   VAStatus va_res = VAAPI_DestroyContext(va_display_, va_context_id_);
   VA_LOG_ON_ERROR(va_res, "vaDestroyContext failed");
@@ skipping 374 matching lines @@
 
   if (!SendVASliceParam(slice_hdr))
     return false;
 
   if (!SendSliceData(slice_hdr->nalu_data, slice_hdr->nalu_size))
     return false;
 
   return true;
 }
 
+void VaapiH264Decoder::DestroyBuffers(VABufferID* va_buffers,
+                                      size_t num_va_buffers) {
+  VAStatus va_res;
+
+  for (size_t i = 0; i < num_va_buffers; ++i) {
+    va_res = VAAPI_DestroyBuffer(va_display_, va_buffers[i]);

[piman, 2012/08/03 03:55:40]

nit: declare va_res here.

+    VA_LOG_ON_ERROR(va_res, "vaDestroyBuffer failed");
+  }
+}
+
 // TODO(posciak) start using vaMapBuffer instead of vaCreateBuffer wherever
 // possible.
 
 bool VaapiH264Decoder::DecodePicture() {
   DCHECK(!frame_ready_at_hw_);
   DCHECK(curr_pic_.get());
 
   static const size_t kMaxVABuffers = 32;
   DCHECK_LE(pending_va_bufs_.size(), kMaxVABuffers);
   DCHECK_LE(pending_slice_bufs_.size(), kMaxVABuffers);
 
   DVLOG(4) << "Pending VA bufs to commit: " << pending_va_bufs_.size();
   DVLOG(4) << "Pending slice bufs to commit: " << pending_slice_bufs_.size();
 
   // Find the surface associated with the picture to be decoded.
   DCHECK(pending_slice_bufs_.size());
   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_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for va_bufs failed", false);
-
-  DVLOG(4) << "Committed " << num_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);
+  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, va_buffers,
+                               num_va_buffers);
+  if (va_res != VA_STATUS_SUCCESS) {
+    DestroyBuffers(va_buffers, num_va_buffers);
+    VA_ERROR_RETURN(va_res, "vaRenderPicture 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();
+  DVLOG(4) << "Committed " << num_va_buffers << "VA buffers";
+
+  // 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();

[Pawel Osciak, 2012/08/03 04:15:02]

Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() on
its destruction and make pending_*_bufs scoped containers (ScopedVectors?) that
would call those destructors on clear(). Then we would not pop from here at all,
but just clear() the whole container at the end of this function. In all other
cases, including intermediate failures in this function, they would clean
themselves up when the containers get destroyed in the destructor. Then the only
other place we'd have to have an explicit clear of this container would be at
Reset(), which we already have (not really needed in Flush(), because it'd
either finish the current picture cleaning up or failing and destroying, or not
have anything in flight at all).

   }
 
   // And send them to the Hw decoder.
-  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, buffers,
-                               num_buffers);
-  VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for slices failed", false);
+  va_res = VAAPI_RenderPicture(va_display_, va_context_id_, slice_buffers,
+                               num_slice_buffers);
+  if (va_res != VA_STATUS_SUCCESS) {
+    DestroyBuffers(va_buffers, num_va_buffers);
+    DestroyBuffers(slice_buffers, num_slice_buffers);
+    VA_ERROR_RETURN(va_res, "vaRenderPicture for slices failed", false);
+  }
 
-  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);
+  VA_LOG_ON_ERROR(va_res, "vaEndPicture failed");
+
+  // Now that EndPicture has passed we can destroy our buffers.
+  DestroyBuffers(va_buffers, num_va_buffers);
+  DestroyBuffers(slice_buffers, num_slice_buffers);
 
   // 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 1015 matching lines @@
     }
   }
 }
 
 // static
 size_t VaapiH264Decoder::GetRequiredNumOfPictures() {
   return kNumReqPictures;
 }
 
 }  // namespace content