GNOME Bugzilla – Bug 735509
oggdemux: should accumulate segment.base
Last modified: 2014-09-01 09:21:46 UTC
In gst_ogg_demux_handle_page(): segment.base = chain->begin_time; This seems wrong as segment.base should be accumulated. This break a WIP patch I'm working on implementing segment looping in PUSH mode: the running time in the sink is always too late after the first loop and so buffers aren't rendered.
Bug #734659 is a similar issue with qtdemux. The proposed patch solves this by assigning the result of gst_segment_to_running_time (position) to base. Would that be a proper fix here as well? This does solve my problem when looping.
Actually base = gst_segment_to_running_time (segment.position) isn't the proper fix to my problem. In push mode, position is always equal to the length of my stream after the first loop iteration. So after the first iteration gst_segment_to_running_time (position) returns 2 * len and so we have to wait len seconds before the sample can be played during the second iteration. This does work, as start is always equal to 0 we now set base to len after the first loop iteration and the sample is played right away as expected. segment.base = gst_segment_to_running_time (&ogg->segment, GST_FORMAT_TIME, start); Is that the proper way to do this?
That's correct though. After the first iteration it should be length of the segment, after the second 2 times the length of the segment, etc. If it doesn't work then, then the running times of the buffers in the segment are wrong.
Yeah I agree segment.base should be 0, len, 2 * len, etc. if I do: segment.base = gst_segment_to_running_time (&ogg->segment, GST_FORMAT_TIME, ogg->segment.position); I end up with: 0 (position = 0 and ogg->segment.base = 0) 2 * len (position = len and ogg->segment.base = len) so the second sample is delayed. But if I do: segment.base = gst_segment_to_running_time (&ogg->segment, GST_FORMAT_TIME, start); I have: 0 (start = 0 and ogg->segment.base = 0) len (start = 0 and ogg->segment.base = len) 2 * len (start = 0 and ogg->segment.base = 2 * len) as expected. Actually, an even easier fix would be just to *not* change segment.base at all and just keep ogg->segment.base. Would that be ok?
Where does it get the base=len from? That sounds like something already does the right thing somewhere in oggdemux
Could you clarify whether upstream operates in BYTE or TIME format in your case ?
(In reply to comment #5) > Where does it get the base=len from? That sounds like something already does > the right thing somewhere in oggdemux In pull mode this is done by gst_segment_do_seek() which is called in gst_ogg_demux_perform_seek_pull(). gst_segment_do_seek() wasn't called in push mode because of a bug in my WIP patch which I now fixed. So yeah, I think we can just stop overriding segment.base and rely on ogg->segment.base as it already contain the right value.
(In reply to comment #6) > Could you clarify whether upstream operates in BYTE or TIME format in your case > ? We receive BYTE segments from upstream (oggdemux doesn't handle TIME segments atm).
Created attachment 284814 [details] [review] oggdemux: don't override segment.base When doing non flushing seeks ogg->segment.base already contains the proper base value (thanks to gst_segment_do_seek()), accumulating time as expected. Segment is copied from ogg->segment so there is no need to override it.
Hum this code was introduced in http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=02c6766c8a2910c0ae1d2ff4da1201af174b6d23 to fix bug #706569 which my patch re-break. :\
(In reply to comment #10) > Hum this code was introduced in > http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=02c6766c8a2910c0ae1d2ff4da1201af174b6d23 > to fix bug #706569 which my patch re-break. :\ So we have to find a way to fulfil these 2 goals: - Segment.base has to match the current position when playing multi chains files or they are not playback because they are "too late" in the sink (bug #706569) - We don't want to override the Segment.base when not-flush seeking as gst_segment_do_seek() do the right thing and set the proper base value. Any suggestion? Looking at the code I'm not sure what a proper way would be.
Review of attachment 284814 [details] [review]: ::: ext/ogg/gstoggdemux.c @@ -4223,3 @@ segment.stop = chain->segment_stop; segment.time = chain->begin_time; - segment.base = chain->begin_time; Maybe this should be segment.base += chain->begin_time?
Created attachment 284988 [details] [review] oggdemux: accumulate base time Base time should be accumulated so non flushing seeks have the expected base. Not accumulating result in segments appearing as "too late" and so are not played by the sink.
Comment on attachment 284988 [details] [review] oggdemux: accumulate base time Good, now it all makes sense :)
Comment on attachment 284988 [details] [review] oggdemux: accumulate base time commit 3c8d3465bf0ec29fe04296c153de2081837fbbe6 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Fri Aug 29 14:00:06 2014 +0200 oggdemux: accumulate base time Base time should be accumulated so non flushing seeks have the expected base. Not accumulating result in segments appearing as "too late" and so are not played by the sink. https://bugzilla.gnome.org/show_bug.cgi?id=735509