GNOME Bugzilla – Bug 665355
[basevideodecoder] deletes frame but keeps it in list
Last modified: 2012-02-18 20:14:31 UTC
In gst_base_video_decoder_reset the decoder frees base_video_decoder->current_frame but the frame instance remains in base_video_codec's frames list. Attached patch removes frame from the list. I'm not quite sure however why the frames are kept in base_video_codec's frame list after flush anyway (the codec only clears the list on state change)
Created attachment 202566 [details] [review] Patch
Now I realized that the patch contains fix for another issue which I haven't created bug report for. BaseVideoDecoder doesn't increase codec_data refcount when it keeps reference to it in GstVideoState, but later gets rid of it using gst_buffer_replace, which unrefs the buffer.
The codec_data changes look correct but the frame one doesn't. Look at the code, there are some FIXME comments about handling of the frames list, in theory all frames should be cleared in _reset() for example but this can't be done because the subclass might still hold references to the frames. The solution would be to introduce a new refcounted type for the frames and then clear the frames in _reset() and elsewhere.
Comment on attachment 202566 [details] [review] Patch Could you split the codec_data fix and the other one?
I'll submit the patches later. I mentioned it in first comment that the fix is probably not entirely correct, but at least it fixes the crash and doesn't leave reference to deleted frame in the list. Perhaps the frame could be a GstMiniObject?
Created attachment 202633 [details] [review] Ref codec data
Created attachment 202635 [details] [review] Remove frame from list before freeing it
Created attachment 202647 [details] [review] Make GstFrameState a GstMiniObject
Latest patch does implements counting on GstFrameState, is this better?
The refcounting change is also valid for GStreamer 0.10. Could you provide patches for both versions? While we're at it, GstVideoFrameState should be renamed to GstBaseVideoCodecFrame or something like that. Also, please free all frames when resetting the decoder (and encoder), not only the current frame. Also I'm not sure if it would be better to have a simple, boxed type with refcounting instead of a mini object. Might be easier for bindings
I do remove all frames in gst_base_video_decoder_clear_queues. + g_list_foreach (GST_BASE_VIDEO_CODEC (dec)->frames, + (GFunc) gst_video_frame_state_unref, NULL); + g_list_free (GST_BASE_VIDEO_CODEC (dec)->frames); + GST_BASE_VIDEO_CODEC (dec)->frames = NULL; I don't do it for encoder though.
Created attachment 202859 [details] [review] Make GstFrameState a ref counted boxed type This patch does clear the stored frames in both encoder and decoder
Created attachment 202860 [details] [review] Rename GstVideoFrameState to GstBaseVideoCodecFrame
I'll add a patch for 0.10 if I have time later.
Review of attachment 202635 [details] [review]: superseded by the ref-count patch.
That's your patch adapter for 0.10: commit 70d13bbb328984469a09491808dae1ab2c16443c Author: Matej Knopp <matej.knopp@gmail.com> Date: Mon Dec 5 18:57:01 2011 +0100 basevideo: Make GstVideoFrame a reference counted boxed object ...and also clear all existing frames when resetting the decoder or encoder.
And this is the 0.11 version. The rename patch unfortunately doesn't make sense after looking at the 0.11 changes. It was recently renamed from GstVideoFrame to GstVideoFrameState. commit 4342eea6371ee49eadc19924a304d6f9376da6f9 Author: Matej Knopp <matej.knopp@gmail.com> Date: Mon Dec 5 18:57:01 2011 +0100 basevideo: Make framestate a reference counted boxed object ...and also clear all existing frames when resetting the decoder or encoder.
*** Bug 654529 has been marked as a duplicate of this bug. ***
Well, I did rename it because you suggested it in previous comment ("While we're at it, GstVideoFrameState should be renamed to GstBaseVideoCodecFrame or something like that")
Right, sorry for that. I should've checked the 0.11 changes before