GNOME Bugzilla – Bug 432612
[matroskamux] doesn't handle segments correctly
Last modified: 2011-10-11 13:04:26 UTC
Hi, Matroskamuxer converts the buffer of timestamps directly to the muxed stream time. Which leads to some issues when you feed it a stream that doesn't start at timestamp 0. Most notably, some frames can get timestamps which don't fit inside the files duration.. I'll attach a patch that handles this very naively, which makes stuff work for my application. But as i'm not sure how muxers should handle segments, it's probably wrong and there could be some regressions when you mixing multiple streams. Is there any documentation about how muxers are supposed to handle segments?
Created attachment 86848 [details] [review] very naive patch :)
IMO, muxers should do as little as possible (time) transformations behind the (hidden) scenes, but put streams into container as faithfully as possible (and out of them for demuxer). Rather, gstreamer's pipeline architecture should be exploited to make sure the muxer's input is as "clean" as possible. For example, rather than having a "naive patch" (or another one for that matter) (again) duplicate segment handling code in yet another element, it seems that the same effect (basically) can be achieved by using identity single-segment=true in the pipeline somewhere before matroskamux. And if the effect that one wishes to achieve in a particular case may be different, than it would seem "cleaner" to put this in custom element (before muxer), or patch identity, or some other element, etc [for example, for my general transcoding app, I have some element that (re)computes timestamps purely based on frame or sample count in stead of segments; should clearly not be in a muxer either]. In particular, pre-transforming a "strange" stream into a "nice" stream would settle this for any subsequent muxer, rather than trying to have all of them prepared for as many possible cases (with approximately duplicated code).
I wonder it would make sense for muxers to keep track of the incoming newsegment event and then use gst_segment_to_stream_time().
They should use the running_time even so that you can mux multiple segments one after another, like how a sink would play them.
@Mark: It would indeed be nice to have a simpler timestamp handling on muxers level, but I don't like the idea of forcing people to add elements before muxers everytime, doesn't look intuitive to me. I think this is one of the things that could be done/handled at the GstPad level once we get tracking of GstSegments into it, or by a BaseMux class.
*** Bug 631850 has been marked as a duplicate of this bug. ***
Created attachment 181042 [details] [review] set buffer timestamps to normalized buffer running time This patch will set all buffer timestamps to normalized buffer running time.
Review of attachment 181042 [details] [review]: As discussed on IRC using the running time for synchronization is the way to go. But no normalization of the running time should be done (to force it to start at 0) and the downstream buffers should have the timestamps stored inside matroska, i.e. the running time, instead of the upstream timestamps. Also I'm not sure if we can change this behaviour now, people might assume the current behaviour. But at least for 0.11 this should definitely be done
I guess most pipelines will not be affected by the change, since most src elements timestamp from 0 so running time equals buffer time, no?
No, for live pipelines this is not necessarily true or for pipelines that are running at a different playback rate or if you're concatenating two different streams for example.
I talked to Wim and yes, we can change this in 0.10. Pipelines where it makes a difference did not work without changing to the running time anyway
commit dd19a7edad076535c917c3f5f3a6cda19804d6cf Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Mar 22 19:36:21 2011 +0100 matroskamux: use running time for synchronization Fixes #432612.
This doesn't seem to work correctly. Fix coming...
Now this should work as advertised: commit 26d0812543dfc8ed9be018e69b860c39d9e0bded matroskamux: fix segment handling, so we actually use running time gst_matroska_mux_best_pad adjusts the buffer timestamp to running time using the segment stored in the pad's collect data. However, the event handler didn't pass the newsegment event on to collectpads' handler, so this segment was never updated at all. Re-fixes bug #432612.