GNOME Bugzilla – Bug 578278
gst-ffmpeg: assign offsets (from upstream) to outgoing buffers
Last modified: 2010-01-05 17:28:55 UTC
gstffmpegdec should assign buffer offsets, so that downstream elements would know the frame/sample numbers.
Created attachment 132288 [details] [review] Assign offsets in ffmpegdec Works with http://bugzilla.gnome.org/show_bug.cgi?id=578052 , since it requires demuxer to assign proper offsets too.
commit 2b25dbe12ee80ddb5c62ebdcf722c8e3fdb3831e Author: LRN <lrn1986 at gmail.com> Date: Fri Apr 10 00:19:50 2009 +0200 ffdec: copy input offsets to output buffers Copy the incomming offsets to the outgoing buffers. Fixes 578278.
Created attachment 138673 [details] [review] Codec frame delay fix and trailing zero-length-frame fix
Reopening with another patch (see the attachment for details).
commit 73d227766a1be096253ebbdfd7fc4df5ddfe8f7b Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Sat Jul 18 18:53:22 2009 +0400 Codec frame delay fix and trailing zero-length frame fix Takes codec frame delay into account (roughly the same way it does for times A special hack to allow trailing frame with timestamp=segment.stop to be dis Fixes bug #578278.
I have a different version of this patch that uses ts_handler to pass offsets from _chain() to _video_frame() and uses separately allocated structures to pass offset (along with timestamp) through reordered_opaque. In theory, that should work even better than delay_offset (which does not take reordering into account). In practice, that does not fix some issues. It's still better than delay_offset though. Also, it includes some potentially unsafe pointer castings (i think them to be safe, but you never know how it would turn out until it hits you). i will attach the patch later today.
Created attachment 139480 [details] [review] Uses ts_handler and reordered_opaque and should be much better.
Created attachment 139483 [details] [review] The same patch, but against current HEAD
Some comments: - Please choose a better name than GstOpaque. That name tells nothing :) - Use GSLice for allocating the GstOpaque objects, that will be faster and result in less memory fragmentation - g_list_append() is slow (always needs to go to the end of the list), better use _prepend() if possible - Use GPOINTER_TO_SIZE(), _TO_UINT() will be 32 bit... and you will see crashes on 64 bit architectures :) Other than that this looks as optimal as it can get
Created attachment 139565 [details] [review] Same thing, but uses GSize and g_slice It uses g_list_append(), because most likely next time it tries to *find* something, it will be the *oldest* item in the list. With _append(), that would be the head of the list. So i'm losing speed on insertion, but gaining it on search. The best approach would be to keep a pointer to the tail of the list, but i'm not sure it's worth the trouble (is it even possible without crufting around g_list?).
commit 54428c186bbb982285daacc749674dc3dca952db Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Thu Jul 30 19:02:12 2009 +0400 ffmpegdec: Assign offsets to outgoing buffers more accurate This now uses ffmpeg functionality to keep random metadata next to the buffers and to get the correct offset for a frame, similar to how timestamps are handled. Fixes bug #578278.
(In reply to comment #5) > commit 73d227766a1be096253ebbdfd7fc4df5ddfe8f7b > Author: Руслан Ижбулатов <lrn1986@gmail.com> > Date: Sat Jul 18 18:53:22 2009 +0400 > > Codec frame delay fix and trailing zero-length frame fix > > Takes codec frame delay into account (roughly the same way it does for > times > A special hack to allow trailing frame with timestamp=segment.stop to be > dis > > Fixes bug #578278. After a number of versions of this bug/patch, the above 'hack' still seems to be in there. But that 'hack' actually breaks/overrides normal segment semantics, and would have some nonlinear-filtering pipeline (e.g. entrans, gnonlin) end up with different (more) output than it would before. Similarly, video stream may end up with more frames than there is corresponding audio data properly clipped to the same segment. [the hack considers a buffers starting at segment->stop to be part of the segment, and fiddles with such buffers' start and duration, etc. Normal segment clipping does not consider it to be part of segment] So, unless there is some very good reason to have 'special hacks' breaking standard behaviour, I feel like reverting that part.