GNOME Bugzilla – Bug 730995
omxvideodec: Memory leak with interlaced h264 streams
Last modified: 2014-07-23 08:11:35 UTC
Following pipeline playin an h264 interlaced stream causes gstomxdec to 'leak' memory: gst-launch-1.0 filesrc location=arte_hd-01-extract.ts ! tsdemux ! h264parse ! omxh264dec ! fakesink The memory leaks occurs because each frame handle by gstomxdec contains a field. Fields are then given to omx component but the OMX component returns a complete frame leading gstomxdec to finish one input frame and keep the other. So, the gstvideodecoder frame list keep growing. It also causes a performance issue. I attached the video stream used in the above pipeline. And I wrote a hack which remove frames from list which have a timestamp less than the frame output by the omx component. However it may be not the best solution.
Created attachment 277544 [details] Interlaced h264 video stream You can also found a bigger extract here: http://www.darkosphere.fr/tmp/arte_hd-01-extract-2.ts
Created attachment 277545 [details] [review] omxvideodec-hack: release frames with old timestamp to avoid memory issue It is just the hack I use to avoid the issue.
Review of attachment 277545 [details] [review]: I agree it's mostly a hack, but we didn't find anything better yet, hence this is what we do in libav to fix the same issue. Please, have a look at livav implementation to double check (iirc it was a bit more complex). ::: omx/gstomxvideodec.c @@ +1179,3 @@ + GstVideoCodecFrame *tmp = l->data; + + if (tmp->pts < timestamp) I'm not sure but can we cleanup <= here ?
(In reply to comment #3) > Review of attachment 277545 [details] [review]: > > I agree it's mostly a hack, but we didn't find anything better yet, hence this > is what we do in libav to fix the same issue. Please, have a look at livav > implementation to double check (iirc it was a bit more complex). I look at gstlibav and you're right. there is the same behavior of cleaning frames. The method is a little different, first it marks all incoming frames as decode only. Then, when a frame is decoded, it is unmarked and all frames marked DECODE_ONLY between the beginning of the list and the current frame are discarded. If you prefer this way, I can do it. > ::: omx/gstomxvideodec.c > @@ +1179,3 @@ > + GstVideoCodecFrame *tmp = l->data; > + > + if (tmp->pts < timestamp) > > I'm not sure but can we cleanup <= here ? I call drop_earlier_frame() before the current frame is finished, so cleanup with '<=' will also release the frame we want to output.
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 277545 [details] [review] [details]: > > > > I agree it's mostly a hack, but we didn't find anything better yet, hence this > > is what we do in libav to fix the same issue. Please, have a look at livav > > implementation to double check (iirc it was a bit more complex). > > I look at gstlibav and you're right. there is the same behavior of cleaning > frames. The method is a little different, first it marks all incoming frames as > decode only. Then, when a frame is decoded, it is unmarked and all frames > marked DECODE_ONLY between the beginning of the list and the current frame are > discarded. > > If you prefer this way, I can do it. I see, so the list order is the order in which frames came in. Re-ordering happens in the decoder. But libav will allocate output buffers for them, even though they have been re-ordered. In fact, it only marks one of the field. This is pretty clever, so we can drop unneeded frames much sooner. I don't think we can do the same here, as the API is different. My only concern is that we rely on correct PTS, hopefully it's always right, though even if it was wrong, I think the result will be ok, and better than leaking. In the long term, it would be much nicer if the parsers could give a little more information, so we could group the fields in the base class. Though it would mean a frame with multiple input buffers, which would be an API break. > > > ::: omx/gstomxvideodec.c > > @@ +1179,3 @@ > > + GstVideoCodecFrame *tmp = l->data; > > + > > + if (tmp->pts < timestamp) > > > > I'm not sure but can we cleanup <= here ? > > I call drop_earlier_frame() before the current frame is finished, so cleanup > with '<=' will also release the frame we want to output. Thanks, it's clear now.
(In reply to comment #5) > (In reply to comment #4) > I see, so the list order is the order in which frames came in. Re-ordering > happens in the decoder. But libav will allocate output buffers for them, even > though they have been re-ordered. In fact, it only marks one of the field. This > is pretty clever, so we can drop unneeded frames much sooner. I don't think we > can do the same here, as the API is different. You're right, we can do this here because frames could be re-ordered by decoder and could be output by OMX component in display order without allocating output frame before. So in case the stream is out of order, with this method, we will drop frames which could be output later. e.g: frame 1 (display order 3) --> +--------+ --> frame 3 frame 2 (display order 2) --> | OMXDec | --> frame 2 frame 3 (display order 1) --> +--------+ --> frame 1 List : frame 1 -> frame 2 -> frame 3 In this exemple, with the above method frame 1 and 2 will be released but they shouldn't. > My only concern is that we rely on correct PTS, hopefully it's always right, > though even if it was wrong, I think the result will be ok, and better than > leaking. Yes, it could be better than leaking. Also I think we should check for valid PTS or not and if not just remove invalid timestamped frames. I will update the patch to add some comments and make minor modifications.
Created attachment 277712 [details] [review] omxvideodec: release frames with old PTS to avoid memory issue Updated patch with comments and a way to handle frame with invalid timestamp. Also this patch assume the facts that the OMX components won't mess up with timestamp. Frame list will continue to grow up if we give valid timestamp to component and if it returns only null timestamp.
Any OMX expert to double check this patch ?
The patch looks correct, but arguably there should be one *frame* per input buffer, and not one *field*. I think we have a bug for h264parse about that already.
commit ad969ffda34f671252d115fbb3293fae43f1b0e6 Author: Aurélien Zanelli <aurelien.zanelli@parrot.com> Date: Fri May 30 15:29:15 2014 +0200 omxvideodec: release frames with old PTS to avoid memory issue Interlaced stream could make the decoder use two input frames to produce one output frame causing the gstvideodecoder frame list to grow. Assuming the video decoder output frame in display order rather than in decoding order, this commit add a way to release frames with PTS less than current output frame. https://bugzilla.gnome.org/show_bug.cgi?id=730995
(In reply to comment #9) > The patch looks correct, but arguably there should be one *frame* per input > buffer, and not one *field*. I think we have a bug for h264parse about that > already. For the reference: https://bugzilla.gnome.org/show_bug.cgi?id=704214
(In reply to comment #9) > The patch looks correct, but arguably there should be one *frame* per input > buffer, and not one *field*. I think we have a bug for h264parse about that > already. For h264, it will also depends on the alignment: if it's au, it will provide full access unit and if it's nal, it will provide each nal separately so an incoming frame in omxh264dec will either contains a full au or just a nal which won't always contains a full frame. In my case, stream AU contains only one field. Also I think current behavior could be interesting to reduce latency since we can begin to decode before having a full frame.