GNOME Bugzilla – Bug 790312
msdk: memory leak always happen when encoding and decoding
Last modified: 2017-11-29 23:41:55 UTC
Reproduce step: # GST_DEBUG="GST_TRACER:7,GST_CALL_TRACE:7" GST_TRACERS="leaks" gst-launch-1.0 videotestsrc num-buffers=100 ! msdkh264enc ! fakesink Result: 0:00:00.213422022 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstBuffer, address=(gpointer)0x7fd028241300, description=(string)buffer: 0x7fd028241300, pts 99:99:99.999999999, dts 99:99:99.999999999, dur 99:99:99.999999999, size 115200, offset none, offset_end none, flags 0x0, ref-count=(uint)1, trace=(string); 0:00:00.213449373 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstBuffer, address=(gpointer)0x7fd028241520, description=(string)buffer: 0x7fd028241520, pts 99:99:99.999999999, dts 99:99:99.999999999, dur 99:99:99.999999999, size 115200, offset none, offset_end none, flags 0x0, ref-count=(uint)1, trace=(string); 0:00:00.213480031 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstBuffer, address=(gpointer)0x7fd0282411f0, description=(string)buffer: 0x7fd0282411f0, pts 99:99:99.999999999, dts 99:99:99.999999999, dur 99:99:99.999999999, size 115200, offset none, offset_end none, flags 0x0, ref-count=(uint)1, trace=(string); 0:00:00.213505559 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstBuffer, address=(gpointer)0x7fd028241410, description=(string)buffer: 0x7fd028241410, pts 99:99:99.999999999, dts 99:99:99.999999999, dur 99:99:99.999999999, size 115200, offset none, offset_end none, flags 0x0, ref-count=(uint)1, trace=(string); 0:00:00.213516695 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstBuffer, address=(gpointer)0x7fd0282410e0, description=(string)buffer: 0x7fd0282410e0, pts 0:00:00.000000000, dts 99:99:99.999999999, dur 0:00:00.033333333, size 115200, offset 0, offset_end 1, flags 0x40, ref-count=(uint)1, trace=(string); 0:00:00.213525424 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstCaps, address=(gpointer)0x7fd0280028f0, description=(string)video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)NV12, interlace-mode=(string)progressive, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, ref-count=(uint)2, trace=(string); 0:00:00.213552458 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstMemory, address=(gpointer)0x7fd0282612a0, description=(string)0x7fd0282612a0, ref-count=(uint)1, trace=(string); 0:00:00.213561953 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstMemory, address=(gpointer)0x7fd028224000, description=(string)0x7fd028224000, ref-count=(uint)1, trace=(string); 0:00:00.213570038 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstMemory, address=(gpointer)0x7fd028245000, description=(string)0x7fd028245000, ref-count=(uint)1, trace=(string); 0:00:00.213577833 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstMemory, address=(gpointer)0x7fd0282997e0, description=(string)0x7fd0282997e0, ref-count=(uint)1, trace=(string); 0:00:00.213585658 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstMemory, address=(gpointer)0x7fd02827d540, description=(string)0x7fd02827d540, ref-count=(uint)1, trace=(string); 0:00:00.213594623 25428 0xa97230 TRACE GST_TRACER :0:: object-alive, type-name=(string)GstVideoBufferPool, address=(gpointer)0x7fd0282221d0, description=(string)<videobufferpool0>, ref-count=(uint)1, trace=(string); ** (gst-launch-1.0:25428): WARNING **: Leaks detected Tested on CentOS7
Created attachment 363555 [details] [review] msdkenc: fix a frame leak This leak happens at the first frame to encode. We should release the frame even if we got no sync point from the driver. Also, a little bit of code clean-up added.
Review of attachment 363555 [details] [review]: Looks good but: ::: sys/msdk/gstmsdkenc.c @@ +527,3 @@ + GST_INFO_OBJECT (thiz, + "No sync point from the driver, skipping this frame"); + goto fail; This is no "fail" really, it's somewhat expected behaviour. Maybe call the label "out", "done", or similar? Also why would it give no sync point? And will the frame appear decoded at a later time, or is it literally gone?
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 363555 [details] [review] [review]: > > Looks good but: > > ::: sys/msdk/gstmsdkenc.c > @@ +527,3 @@ > + GST_INFO_OBJECT (thiz, > + "No sync point from the driver, skipping this frame"); > + goto fail; > > This is no "fail" really, it's somewhat expected behaviour. Maybe call the > label "out", "done", or similar? That's what I hesitated actually. Probably 'done' would be better. > > Also why would it give no sync point? And will the frame appear decoded at a > later time, or is it literally gone? Well, I realized that I misunderstood the situation :( In this case, the driver returns MORE_DATA, which means that the frame will be decoded later. And this patch is wrong! I'll re-propose a patch to handle MORE_DATA.
Created attachment 363565 [details] [review] msdkenc: fix a frame leak This leak happens at the first frame to encode. We should release the frame if we got MFX_ERR_MORE_DATA from the driver. Also, a little bit of code clean-up added.
Found a problem with filesrc. I'm re-working on it.
Comment on attachment 363565 [details] [review] msdkenc: fix a frame leak On MORE_DATA you probably also don't want to finish the frame, but just unref it and retrieve it from the base class again later when you actually got output for it (or it was decided that the frame is to be discarded, do we know about that?).
Created attachment 364093 [details] [review] msdkenc: handle the MORE_DATA case If the driver requires more data, just unref the frame at the moment then retreive/finish the frame after encoding is finished. This also fixes a memory leak.
Created attachment 364094 [details] [review] msdkenc: handle the MORE_DATA case If the driver requires more data, just unref the frame at the moment then retreive/finish the frame after encoding is finished. This also fixes a memory leak.
Review of attachment 364094 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +643,3 @@ + GstVideoCodecFrame *frame; + + frame = gst_video_encoder_get_oldest_frame (GST_VIDEO_ENCODER_CAST (thiz)); This is wrong if frame reordering happens. You're getting the oldest frame in PTS order here Ideally you would carry the system_frame_number of the frame through the MSDK, and then retrieve it here with that number again. @@ +647,3 @@ + gst_msdkenc_dequeue_frame (thiz, frame); + gst_video_encoder_finish_frame (GST_VIDEO_ENCODER (thiz), frame); + } What if we get here ("more_data") but no frame is pending? Shouldn't that be an error?
Created attachment 364105 [details] [review] msdkenc: handle the MORE_DATA case If the driver requires more data, just unref the frame at the moment then retreive/finish the frame after encoding is finished. This also fixes a memory leak.
Review of attachment 364105 [details] [review]: Looks good to me, thanks!
Review of attachment 364105 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +649,3 @@ + gst_msdkenc_dequeue_frame (thiz, frame); + gst_video_encoder_finish_frame (GST_VIDEO_ENCODER (thiz), frame); + gst_msdkenc_reset_task (task); not sure but I feel this line is missing here: task->more_data = FALSE;
Review of attachment 364105 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +649,3 @@ + gst_msdkenc_dequeue_frame (thiz, frame); + gst_video_encoder_finish_frame (GST_VIDEO_ENCODER (thiz), frame); + gst_msdkenc_reset_task (task); gst_msdkenc_reset_task is doing that.
Review of attachment 364105 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +649,3 @@ + gst_msdkenc_dequeue_frame (thiz, frame); + gst_video_encoder_finish_frame (GST_VIDEO_ENCODER (thiz), frame); + gst_msdkenc_reset_task (task); Oh. Ok. Sorry for the noise.
Probably the same is needed for the decoder also?
(In reply to Sebastian Dröge (slomo) from comment #15) > Probably the same is needed for the decoder also? I also found some leaks when decoding, but didn't check if that is similar to this. I'm going to look into this.
Created attachment 364161 [details] [review] msdkdec: fix buffer leaks during drain and a leak of videobufferpool
Review of attachment 364161 [details] [review]: ::: sys/msdk/gstmsdkdec.c @@ +703,3 @@ task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); + if ((flow = gst_msdkdec_finish_task (thiz, task)) != GST_FLOW_OK) + return flow; Should all other tasks not be finished too somehow, so that all pending frames are discarded at least?
commit 0933b8b45a5888ad9454be25b3e96a8851e67aea (HEAD -> master) Author: Hyunjun Ko <zzoon@igalia.com> Date: Wed Nov 22 11:33:54 2017 +0900 msdkdec: fix buffer leaks during drain and a leak of videobufferpool https://bugzilla.gnome.org/show_bug.cgi?id=790312 commit ddd9355767b03ed48ff180f9e9ab480afa3cf613 Author: Hyunjun Ko <zzoon@igalia.com> Date: Tue Nov 21 14:57:03 2017 +0900 msdkenc: handle the MORE_DATA case If the driver requires more data, just unref the frame at the moment then retreive/finish the frame after encoding is finished. This also fixes a memory leak. https://bugzilla.gnome.org/show_bug.cgi?id=790312
Keeping the bug open for the remaining patch as mentioned in comment 18.
Created attachment 364255 [details] [review] msdkdec: keep draining even if a finish_task fails Should continue draining so that it could try to discard the rest of pending frames even if a finish_task fails.
Attachment 364255 [details] pushed as 3301dd3 - msdkdec: keep draining even if a finish_task fails
Let me know if any of the many MSDK changes should be backported to 1.12, thanks :)