gstreamer vpudec memory leak

[martinetd]

Hi,

One of our customers reported a leak when using vpudec, we could reproduce with the latest BSP on the i.MX8MP evk as well.

I've attached the reproducing file we were provided, and the following command reproduces it (normal invocation with waylandsink instead of fakesink leaks as well)

gst-launch-1.0 multifilesrc location=./test.ts loop=true ! tsdemux ! h264parse ! vpudec ! fakesink

After analyzing the problem, it turns out h264parse provides two frames everytime vpudec actually decodes one, every other frame goes through this code:

GstFlowReturn
gst_vpu_dec_object_decode (GstVpuDecObject * vpu_dec_object, \
    GstVideoDecoder * bdec, GstVideoCodecFrame * frame)
{     
...
    if (buf_ret & VPU_DEC_NO_ENOUGH_INBUF) {
      GST_LOG_OBJECT (vpu_dec_object, "Got not enough input message!!");
      if (!IS_AMPHION() && vpu_dec_object->state < STATE_REGISTRIED_FRAME_BUFFER) {
        GST_WARNING_OBJECT (vpu_dec_object, "Dropped video frame before VPU init ok!");
        ret = gst_vpu_dec_object_send_output (vpu_dec_object, bdec, TRUE);
        if (ret != GST_FLOW_OK) {
          GST_ERROR_OBJECT(vpu_dec_object, "gst_vpu_dec_object_send_output fail: %s\n", \
              gst_flow_get_name (ret));
          return ret;
        }
      }
      break;
    }

(imx8mp is not AMPHION so we go through that check, but state is already registered at this point)

The leak is fixed if we make the drop unconditional (hopefully forum didn't mangle too much, but I just moved the send_output(TRUE) call out of the if, adding another warning if the frame was not used just in case):

From 6ab1adba011dfab4330a712ff505aef39852586b Mon Sep 17 00:00:00 2001
From: Dominique Martinet <dominique.martinet@atmark-techno.com>
Date: Fri, 31 Mar 2023 09:42:37 +0900
Subject: [PATCH] vpudec: Fix leak when not enough input message

When the input is too small we lose track of the current frame, we
should always drop it as it has been processed.

Add a warning in case buffer hasn't been consumed.

diff --git a/plugins/vpu/gstvpudecobject.c b/plugins/vpu/gstvpudecobject.c
index eab7625bd8c0..6f2f7df05c89 100644
--- a/plugins/vpu/gstvpudecobject.c
+++ b/plugins/vpu/gstvpudecobject.c
@@ -1501,12 +1501,14 @@ gst_vpu_dec_object_decode (GstVpuDecObject * vpu_dec_object, \
       GST_LOG_OBJECT (vpu_dec_object, "Got not enough input message!!");
       if (!IS_AMPHION() && vpu_dec_object->state < STATE_REGISTRIED_FRAME_BUFFER) {
         GST_WARNING_OBJECT (vpu_dec_object, "Dropped video frame before VPU init ok!");
-        ret = gst_vpu_dec_object_send_output (vpu_dec_object, bdec, TRUE);
-        if (ret != GST_FLOW_OK) {
-          GST_ERROR_OBJECT(vpu_dec_object, "gst_vpu_dec_object_send_output fail: %s\n", \
-              gst_flow_get_name (ret));
-          return ret;
-        }
+      } else if (!(buf_ret & VPU_DEC_INPUT_USED)) {
+        GST_WARNING_OBJECT (vpu_dec_object, "VPU hasn't consumed partial input data, dropping frame anyway");
+      }
+      ret = gst_vpu_dec_object_send_output (vpu_dec_object, bdec, TRUE);
+      if (ret != GST_FLOW_OK) {
+        GST_ERROR_OBJECT(vpu_dec_object, "gst_vpu_dec_object_send_output fail: %s\n", \
+            gst_flow_get_name (ret));
+        return ret;
       }
       break;
     }

Please consider adding this patch or something similar to the next release of https://github.com/nxp-imx/imx-gst1.0-plugin

Thank you,

Dominique Martinet

[joanxie]

thanks for your advising, I will forward to the multimedia team

[martinetd]

hi @joanxie 

Have you heard back from the multimedia team? I do not see any commit related to such a leak fix in the imx-gst1.0-plugin/vpuwrap repositories and it still reproduces on the L6.1.22_2.0.0_MX8MP BSP, so I'm not sure the sample I submitted was analyzed but it turns out my fix was too simplistic and while it does inhibit the leak it also drops too many frames, resulting in stutter that wasn't observable in the test sample because it is too static.

Running with GST_DEBUG=vpu_dec_object:9, we can observe that the "system frames list" gets out of hand (system_frame_number_in_vpu / frames obtained via gst_video_decoder_get_frame); in particular this line in the leak.log released version output I attached shows there are 439 buffers pending after 3s and it keeps growing:

> 0:00:03.501632945 [...] system frame number send out: 443 list length: 439

Unfortunately this removes too many frames as with my patch and the above list gets fully emptied when it shouldn't, so some valid frames that should be output don't get displayed properly ("gst_video_decoder_get_frame failed" message in stutter.log , obtained from my patched version)

I'm currently trying to simplify the leak reproducer as much as possible (removed filler frames etc) to try to identify the problem, but it seems to me that it might rather be a bug in the vpuwrap or actual vpu code that doesn't report a frame should be dropped properly?

Any input to help working around this would be greatly appreciated.

Thanks,

Dominique

[martinetd]

I unfortunately haven't been able to find any common pattern; there just aren't enough frames coming back from VPU_DecDecodeBuf with buf_ret as either VPU_DEC_OUTPUT_DIS or something dropped (VPU_DEC_OUTPUT_DROPPED/VPU_DEC_SKIP)

For now I'll be updating my workaround to count how many buffers we have in the system on every NO_ENOUGH_INBUF, and skip if we have too many as that's a reliable sign of leakage. At the very least it can't be worse than my previous workaround, as even if my assumption that there can be many buffers in system in a legitimate stream we only check in the NO_ENOUGH_INBUF case that would drop too much...

Here's the actual patch:

From d0eeb79176deb5e25f918ce148da2feef1328900 Mon Sep 17 00:00:00 2001
From: Dominique Martinet <dominique.martinet@atmark-techno.com>
Date: Fri, 31 Mar 2023 09:42:37 +0900
Subject: [PATCH] vpudec: workaround leak when too many system buffers and
 NO_ENOGUH_INBUF

In some cases, 'system frame' keep accumulating as can be seen in
GST_DEBUG=vpu_dec_object:9 logs, e.g.:
      system frame number send out: 330 list length: 340

(the list should probably never be longer than 'actual_buf_cnt' which is
11 for our platform)

There does not seem to be any obvious pattern as VPU_DEC_NO_ENOUGH_INBUF
can also happen on valid streams where the frame comes back in the next
iteration of the loop, but in the samples we've seen in these case the
VPU has processed the frame (either VPU_DEC_ONE_FRM_CONSUMED or
VPU_DEC_INPUT_USED is set)

Workaround the issue by just counting how many system buffers we have in
list and dropping one if we had too many.

diff --git a/plugins/vpu/gstvpudecobject.c b/plugins/vpu/gstvpudecobject.c
index eab7625bd8c0..582f5a349535 100644
--- a/plugins/vpu/gstvpudecobject.c
+++ b/plugins/vpu/gstvpudecobject.c
@@ -1501,12 +1501,17 @@ gst_vpu_dec_object_decode (GstVpuDecObject * vpu_dec_object, \
       GST_LOG_OBJECT (vpu_dec_object, "Got not enough input message!!");
       if (!IS_AMPHION() && vpu_dec_object->state < STATE_REGISTRIED_FRAME_BUFFER) {
         GST_WARNING_OBJECT (vpu_dec_object, "Dropped video frame before VPU init ok!");
-        ret = gst_vpu_dec_object_send_output (vpu_dec_object, bdec, TRUE);
-        if (ret != GST_FLOW_OK) {
-          GST_ERROR_OBJECT(vpu_dec_object, "gst_vpu_dec_object_send_output fail: %s\n", \
-              gst_flow_get_name (ret));
-          return ret;
-        }
+      } else if (g_list_length (vpu_dec_object->system_frame_number_in_vpu) > 2 * vpu_dec_object->actual_buf_cnt) {
+        GST_WARNING_OBJECT (vpu_dec_object, "Too many frames in system buffers, dropping one");
+      } else {
+        /* probably not leaking, just skip */
+        break;
+      }
+      ret = gst_vpu_dec_object_send_output (vpu_dec_object, bdec, TRUE);
+      if (ret != GST_FLOW_OK) {
+        GST_ERROR_OBJECT(vpu_dec_object, "gst_vpu_dec_object_send_output fail: %s\n", \
+            gst_flow_get_name (ret));
+        return ret;
       }
       break;
     }

I'd really appreciate some feedback and/or an officially supported fix when someone from the multimedia team has time to look at it, it would be great if you could follow up on them.

Thank you,

Dominique