After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 793708 - msdk: dec: memleaks when flushing
msdk: dec: memleaks when flushing
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.13.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 789886
 
 
Reported: 2018-02-22 02:03 UTC by Hyunjun Ko
Modified: 2018-03-08 19:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
msdk: dec: fix leaks when flushing (3.08 KB, patch)
2018-02-22 02:24 UTC, Hyunjun Ko
committed Details | Review

Description Hyunjun Ko 2018-02-22 02:03:15 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.
Comment 1 Hyunjun Ko 2018-02-22 02:24:25 UTC
Created attachment 368737 [details] [review]
msdk: dec: fix leaks when flushing
Comment 2 Hyunjun Ko 2018-02-22 03:43:35 UTC
Note that this patch is based on patches in bug #793413
Comment 3 sreerenj 2018-02-24 00:23:42 UTC
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?
Comment 4 Hyunjun Ko 2018-03-01 08:22:27 UTC
(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?
Comment 5 sreerenj 2018-03-01 21:47:26 UTC
(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 :)
Comment 6 Hyunjun Ko 2018-03-02 05:03:56 UTC
(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
Comment 7 sreerenj 2018-03-08 19:43:10 UTC
Review of attachment 368737 [details] [review]:

Pushed as commit 8a3630ffd74b40f089c9f9a36dce229839180e92