GNOME Bugzilla – Bug 793708
msdk: dec: memleaks when flushing
Last modified: 2018-03-08 19:43:28 UTC
Since changes of flush in decoder in https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=a66d562, there happens mem leaks. This seems to be existed originally and we should fix this.
Created attachment 368737 [details] [review] msdk: dec: fix leaks when flushing
Note that this patch is based on patches in bug #793413
Review of attachment 368737 [details] [review]: ::: sys/msdk/gstmsdkdec.c @@ +947,3 @@ for (i = 0; i < thiz->tasks->len; i++) { task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); + gst_msdkdec_finish_task (thiz, task); Why remove the return value checking? Assume that the finish_frame returns error due to flushing, in this case, you are not supposed to send a queued task/frame again. right?
(In reply to sreerenj from comment #3) > Review of attachment 368737 [details] [review] [review]: > > ::: sys/msdk/gstmsdkdec.c > @@ +947,3 @@ > for (i = 0; i < thiz->tasks->len; i++) { > task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); > + gst_msdkdec_finish_task (thiz, task); > > Why remove the return value checking? > Assume that the finish_frame returns error due to flushing, in this case, > you are not supposed to send a queued task/frame again. right? The frame is not sent to downstream eventually in gst_msdkdec_finish_task. And I think we have to call gst_msdkdec_finish_task for every task instead of returning in the middle of drain, causing memleaks. What do you think?
(In reply to Hyunjun Ko from comment #4) > (In reply to sreerenj from comment #3) > > Review of attachment 368737 [details] [review] [review] [review]: > > > > ::: sys/msdk/gstmsdkdec.c > > @@ +947,3 @@ > > for (i = 0; i < thiz->tasks->len; i++) { > > task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); > > + gst_msdkdec_finish_task (thiz, task); > > > > Why remove the return value checking? > > Assume that the finish_frame returns error due to flushing, in this case, > > you are not supposed to send a queued task/frame again. right? > > The frame is not sent to downstream eventually in gst_msdkdec_finish_task. > And I think we have to call gst_msdkdec_finish_task for every task instead > of returning in the middle of drain, causing memleaks. > > What do you think? gst_msdkdec_finish_task () will try to push each frame downstream using gst_video_decoder_finish_frame(). If the first task returns GST_FLOW_ERROR due to flushing and finish_task() trying to push few more pending frames may cause undefined behavior or Not, I am not sure?? You may ask Slomo or Tim :)
(In reply to sreerenj from comment #5) > (In reply to Hyunjun Ko from comment #4) > > (In reply to sreerenj from comment #3) > > > Review of attachment 368737 [details] [review] [review] [review] [review]: > > > > > > ::: sys/msdk/gstmsdkdec.c > > > @@ +947,3 @@ > > > for (i = 0; i < thiz->tasks->len; i++) { > > > task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); > > > + gst_msdkdec_finish_task (thiz, task); > > > > > > Why remove the return value checking? > > > Assume that the finish_frame returns error due to flushing, in this case, > > > you are not supposed to send a queued task/frame again. right? > > > > The frame is not sent to downstream eventually in gst_msdkdec_finish_task. > > And I think we have to call gst_msdkdec_finish_task for every task instead > > of returning in the middle of drain, causing memleaks. > > > > What do you think? > > gst_msdkdec_finish_task () will try to push each frame downstream using > gst_video_decoder_finish_frame(). If the first task returns GST_FLOW_ERROR > due to flushing and finish_task() trying to push few more pending frames may > cause undefined behavior or Not, I am not sure?? > > You may ask Slomo or Tim :) See here and the patch in the thread. :) https://bugzilla.gnome.org/show_bug.cgi?id=790312#c18
Review of attachment 368737 [details] [review]: Pushed as commit 8a3630ffd74b40f089c9f9a36dce229839180e92