GNOME Bugzilla – Bug 643775
[oggmux] use running time instead of timestamps
Last modified: 2011-04-01 09:05:45 UTC
While theora/vorbis encoders use running time to calculate the granulepos of ogg packets, oggmux was ordering them by timestamps, which could lead to bad ordering in the resulting stream. Making oggmux (and muxers in general) use running_time instead of timestamps might cause some breakages, I wonder if this should only go into 0.11 branch. Anyway, patch follow.
Created attachment 182335 [details] [review] oggmux: Keep track of pad's segments
Created attachment 182336 [details] [review] oggmux: Use running time instead of timestamps Theora and vorbis use running time (which is correct) for calculating the granulepos for their ogg packets. Oggmux, however, used timestamps to order the received buffers. This patch makes it use the running time to compare buffer times and also to timestamp pushed buffers. Some bits of the code still use timestamps, but they are only used to calculate durations, so it should be fine. Someone with more ogg experience should double check this :)
Also/btw/fwiw, it is not clear whether vorbis/theora granulepos/running time handling is The Way To Go. IIRC, they may be the only encoders playing around with running time (and even other granulepos aware do not do so, e.g. speexenc). In particular, it leads to a "non-matching" buffer ts versus granulepos (whatever matching is to mean in this case). As such, base audio encoder approach currently does not consider running_time (see bug #642690). Possibly, the only "right" way here is (as said above) to have all muxers deal with running_time and have oggmux in particular handle the granulepos magic, problem is how to handle the mess^W situation till then.
Review of attachment 182335 [details] [review]: ::: ext/ogg/gstoggmux.c @@ +326,2 @@ switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_NEWSEGMENT:{ You also need to reset (i.e. re-init) the segment on flush-stop ::: ext/ogg/gstoggmux.h @@ +54,3 @@ gboolean have_type; + GstSegment *segment; Just store it as GstSegment instead of GstSegment*, saves one allocation and the possibility to create a memory leak later ;)
Calculating the granulepos from the running time and using the running time for synchronization, in-stream timestamps and downstream buffer timestamps is the way to go. However I'm not sure if we can change this now because users might expect the old behaviour. Also in the case of ogg the timestamp->granulepos conversion and all that should probably happen inside oggmux instead of letting the encoders handle this.
I talked to Wim and yes, we can change the synchronization and everything to running time in 0.10. Pipelines where it makes a difference did not work without changing to the running time anyway
Technically, we should be ordering by granule time. However, that's somewhat orthogonal to the timestamp/running time issue.
commit 1c475f10e1bf76a060c54e227ae3dfd54802c17c Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Apr 1 11:00:38 2011 +0200 oggmux: Store the segment directly inside the pad Also initialize it always in TIME format. We require TIME segments in oggmux anyway and drop newsegment events in other formats and assume an open-ended segment starting at 0. commit fc56c7677328e7ae7fdfa8484dd5c71f655933d0 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Apr 1 10:57:08 2011 +0200 oggmux: Reset the segment on flush-stop events and when going back to READY commit d1c74779f940ea4d261031afaa7b890fc779b6b1 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Thu Mar 3 08:45:15 2011 -0300 oggmux: Use running time instead of timestamps Theora and vorbis use running time (which is correct) for calculating the granulepos for their ogg packets. Oggmux, however, used timestamps to order the received buffers. This patch makes it use the running time to compare buffer times and also to timestamp pushed buffers. Some bits of the code still use timestamps, but they are only used to calculate durations, so it should be fine. https://bugzilla.gnome.org/show_bug.cgi?id=643775 commit c3aae3dc1755b0f9881617e137f6f7c8eda0a5f5 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Wed Feb 16 16:07:49 2011 -0300 oggmux: Keep track of pad's segments https://bugzilla.gnome.org/show_bug.cgi?id=643775