va: Destroy picture unreleased buffers when finalize.

The current way of GstVaDecodePicture's finalize will leak some
resource such as parameter buffers and slice data.
The current way deliberately leaves these resource releasing logic
to va decoder related function and trigger a warning if we free the
GstVaDecodePicture without releasing these resources.
But in practice, sometimes, you do not have the chance to release
these resource before picture is freed. For example, H264/Mpeg2
support multi slice NALs/Packets for one frame. It is possible that
we already succeed to parse and generate the first several slices
data by _decode_slice(), but then we get a wrong slice NAL/packet
and fail to parse it. We decide to discard the whole frame in the
decoder's base class, it just free the current picture and does not
trigger sub class's function again. In this kind of cases, we do
not have the chance to cleanup the resource, and the resource will
be leaked.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/1841>

6 files changed, 49 insertions, 28 deletions

diff --git a/sys/va/gstvadecoder.c b/sys/va/gstvadecoder.c
index c68e9437f..7e21e32a1 100644
--- a/sys/va/gstvadecoder.c
+++ b/sys/va/gstvadecoder.c
@@ -640,34 +640,22 @@ fail_end_pic:
   }
 }
 
-gboolean
-gst_va_decoder_destroy_buffers (GstVaDecoder * self, GstVaDecodePicture * pic)
+static gboolean
+_va_decoder_picture_destroy_buffers (GstVaDecodePicture * pic)
 {
   VABufferID buffer;
   VADisplay dpy;
   VAStatus status;
-  VASurfaceID surface;
   guint i;
   gboolean ret = TRUE;
 
-  g_return_val_if_fail (GST_IS_VA_DECODER (self), FALSE);
-  g_return_val_if_fail (pic, FALSE);
-
-  surface = gst_va_decode_picture_get_surface (pic);
-  if (surface == VA_INVALID_ID) {
-    GST_ERROR_OBJECT (self, "Decode picture without VASurfaceID");
-    return FALSE;
-  }
-
-  GST_TRACE_OBJECT (self, "Destroy buffers of surface %#x", surface);
-
-  dpy = gst_va_display_get_va_dpy (self->display);
+  dpy = gst_va_display_get_va_dpy (pic->display);
 
   for (i = 0; i < pic->buffers->len; i++) {
     buffer = g_array_index (pic->buffers, VABufferID, i);
-    gst_va_display_lock (self->display);
+    gst_va_display_lock (pic->display);
     status = vaDestroyBuffer (dpy, buffer);
-    gst_va_display_unlock (self->display);
+    gst_va_display_unlock (pic->display);
     if (status != VA_STATUS_SUCCESS) {
       ret = FALSE;
       GST_WARNING ("Failed to destroy parameter buffer: %s",
@@ -677,9 +665,9 @@ gst_va_decoder_destroy_buffers (GstVaDecoder * self, GstVaDecodePicture * pic)
 
   for (i = 0; i < pic->slices->len; i++) {
     buffer = g_array_index (pic->slices, VABufferID, i);
-    gst_va_display_lock (self->display);
+    gst_va_display_lock (pic->display);
     status = vaDestroyBuffer (dpy, buffer);
-    gst_va_display_unlock (self->display);
+    gst_va_display_unlock (pic->display);
     if (status != VA_STATUS_SUCCESS) {
       ret = FALSE;
       GST_WARNING ("Failed to destroy slice buffer: %s", vaErrorStr (status));
@@ -692,18 +680,41 @@ gst_va_decoder_destroy_buffers (GstVaDecoder * self, GstVaDecodePicture * pic)
   return ret;
 }
 
+gboolean
+gst_va_decoder_destroy_buffers (GstVaDecoder * self, GstVaDecodePicture * pic)
+{
+  VASurfaceID surface;
+
+  g_return_val_if_fail (GST_IS_VA_DECODER (self), FALSE);
+  g_return_val_if_fail (pic, FALSE);
+
+  surface = gst_va_decode_picture_get_surface (pic);
+  if (surface == VA_INVALID_ID) {
+    GST_ERROR_OBJECT (self, "Decode picture without VASurfaceID");
+    return FALSE;
+  }
+
+  g_assert (pic->display == self->display);
+
+  GST_TRACE_OBJECT (self, "Destroy buffers of surface %#x", surface);
+
+  return _va_decoder_picture_destroy_buffers (pic);
+}
+
 
 GstVaDecodePicture *
-gst_va_decode_picture_new (GstBuffer * buffer)
+gst_va_decode_picture_new (GstVaDecoder * self, GstBuffer * buffer)
 {
   GstVaDecodePicture *pic;
 
   g_return_val_if_fail (buffer && GST_IS_BUFFER (buffer), NULL);
+  g_return_val_if_fail (self && GST_IS_VA_DECODER (self), NULL);
 
   pic = g_slice_new (GstVaDecodePicture);
   pic->gstbuffer = gst_buffer_ref (buffer);
   pic->buffers = g_array_sized_new (FALSE, FALSE, sizeof (VABufferID), 16);
   pic->slices = g_array_sized_new (FALSE, FALSE, sizeof (VABufferID), 64);
+  pic->display = gst_object_ref (self->display);
 
   return pic;
 }
@@ -722,12 +733,15 @@ gst_va_decode_picture_free (GstVaDecodePicture * pic)
 {
   g_return_if_fail (pic);
 
-  if (pic->buffers->len > 0 || pic->slices->len > 0)
-    GST_WARNING ("VABufferID are leaked");
+  if (pic->buffers->len > 0 || pic->slices->len > 0) {
+    GST_WARNING ("VABufferIDs have not been released.");
+    _va_decoder_picture_destroy_buffers (pic);
+  }
 
   gst_buffer_unref (pic->gstbuffer);
   g_clear_pointer (&pic->buffers, g_array_unref);
   g_clear_pointer (&pic->slices, g_array_unref);
+  gst_clear_object (&pic->display);
 
   g_slice_free (GstVaDecodePicture, pic);
 }
diff --git a/sys/va/gstvadecoder.h b/sys/va/gstvadecoder.h
index a37136375..0c6631ddf 100644
--- a/sys/va/gstvadecoder.h
+++ b/sys/va/gstvadecoder.h
@@ -27,6 +27,7 @@ G_BEGIN_DECLS
 typedef struct _GstVaDecodePicture GstVaDecodePicture;
 struct _GstVaDecodePicture
 {
+  VADisplay display;
   GArray *buffers;
   GArray *slices;
   GstBuffer *gstbuffer;
@@ -69,7 +70,8 @@ gboolean              gst_va_decoder_decode               (GstVaDecoder * self,
 gboolean              gst_va_decoder_destroy_buffers      (GstVaDecoder * self,
                                                            GstVaDecodePicture * pic);
 
-GstVaDecodePicture *  gst_va_decode_picture_new           (GstBuffer * buffer);
+GstVaDecodePicture *  gst_va_decode_picture_new           (GstVaDecoder * self,
+                                                           GstBuffer * buffer);
 VASurfaceID           gst_va_decode_picture_get_surface   (GstVaDecodePicture * pic);
 void                  gst_va_decode_picture_free          (GstVaDecodePicture * pic);
 GstVaDecodePicture *  gst_va_decode_picture_dup           (GstVaDecodePicture * pic);
diff --git a/sys/va/gstvah264dec.c b/sys/va/gstvah264dec.c
index 657d5277f..a355c0037 100644
--- a/sys/va/gstvah264dec.c
+++ b/sys/va/gstvah264dec.c
@@ -500,12 +500,13 @@ gst_va_h264_dec_new_picture (GstH264Decoder * decoder,
   GstVaH264Dec *self = GST_VA_H264_DEC (decoder);
   GstVaDecodePicture *pic;
   GstVideoDecoder *vdec = GST_VIDEO_DECODER (decoder);
+  GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
 
   self->last_ret = gst_video_decoder_allocate_output_frame (vdec, frame);
   if (self->last_ret != GST_FLOW_OK)
     goto error;
 
-  pic = gst_va_decode_picture_new (frame->output_buffer);
+  pic = gst_va_decode_picture_new (base->decoder, frame->output_buffer);
 
   gst_h264_picture_set_user_data (picture, pic,
       (GDestroyNotify) gst_va_decode_picture_free);
@@ -530,12 +531,13 @@ gst_va_h264_dec_new_field_picture (GstH264Decoder * decoder,
 {
   GstVaDecodePicture *first_pic, *second_pic;
   GstVaH264Dec *self = GST_VA_H264_DEC (decoder);
+  GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
 
   first_pic = gst_h264_picture_get_user_data ((GstH264Picture *) first_field);
   if (!first_pic)
     return FALSE;
 
-  second_pic = gst_va_decode_picture_new (first_pic->gstbuffer);
+  second_pic = gst_va_decode_picture_new (base->decoder, first_pic->gstbuffer);
   gst_h264_picture_set_user_data (second_field, second_pic,
       (GDestroyNotify) gst_va_decode_picture_free);
 
diff --git a/sys/va/gstvah265dec.c b/sys/va/gstvah265dec.c
index 6919efa00..19e5c9f87 100644
--- a/sys/va/gstvah265dec.c
+++ b/sys/va/gstvah265dec.c
@@ -603,12 +603,13 @@ gst_va_h265_dec_new_picture (GstH265Decoder * decoder,
   GstVaH265Dec *self = GST_VA_H265_DEC (decoder);
   GstVaDecodePicture *pic;
   GstVideoDecoder *vdec = GST_VIDEO_DECODER (decoder);
+  GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
 
   self->last_ret = gst_video_decoder_allocate_output_frame (vdec, frame);
   if (self->last_ret != GST_FLOW_OK)
     goto error;
 
-  pic = gst_va_decode_picture_new (frame->output_buffer);
+  pic = gst_va_decode_picture_new (base->decoder, frame->output_buffer);
 
   gst_h265_picture_set_user_data (picture, pic,
       (GDestroyNotify) gst_va_decode_picture_free);
diff --git a/sys/va/gstvavp8dec.c b/sys/va/gstvavp8dec.c
index 3457a4b2c..bb4fac933 100644
--- a/sys/va/gstvavp8dec.c
+++ b/sys/va/gstvavp8dec.c
@@ -197,12 +197,13 @@ gst_va_vp8_dec_new_picture (GstVp8Decoder * decoder,
   GstVaVp8Dec *self = GST_VA_VP8_DEC (decoder);
   GstVaDecodePicture *pic;
   GstVideoDecoder *vdec = GST_VIDEO_DECODER (decoder);
+  GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
 
   self->last_ret = gst_video_decoder_allocate_output_frame (vdec, frame);
   if (self->last_ret != GST_FLOW_OK)
     goto error;
 
-  pic = gst_va_decode_picture_new (frame->output_buffer);
+  pic = gst_va_decode_picture_new (base->decoder, frame->output_buffer);
 
   gst_vp8_picture_set_user_data (picture, pic,
       (GDestroyNotify) gst_va_decode_picture_free);
diff --git a/sys/va/gstvavp9dec.c b/sys/va/gstvavp9dec.c
index 0d7a2716a..b1657ea8b 100644
--- a/sys/va/gstvavp9dec.c
+++ b/sys/va/gstvavp9dec.c
@@ -198,12 +198,13 @@ gst_va_vp9_dec_new_picture (GstVp9Decoder * decoder,
   GstVaVp9Dec *self = GST_VA_VP9_DEC (decoder);
   GstVaDecodePicture *pic;
   GstVideoDecoder *vdec = GST_VIDEO_DECODER (decoder);
+  GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
 
   ret = gst_video_decoder_allocate_output_frame (vdec, frame);
   if (ret != GST_FLOW_OK)
     goto error;
 
-  pic = gst_va_decode_picture_new (frame->output_buffer);
+  pic = gst_va_decode_picture_new (base->decoder, frame->output_buffer);
 
   gst_vp9_picture_set_user_data (picture, pic,
       (GDestroyNotify) gst_va_decode_picture_free);