From b532ce16a52dfc7e1a806ed559aab5d77735dcd8 Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 14 Jan 2014 12:05:46 +0000 Subject: [PATCH] oggdemux: fix broken seeking reading the whole file A change in gst_ogg_demux_do_seek caused oggdemux to wait for a page for each of the streams, including a skeleton stream if one was present. Since Skeleton only has header pages, that was never going to end well. Also, the code was skipping CMML streams when looking for pages, so would also have broken on CMML streams. Thus, we change the code to disregard Skeleton streams, as well as discontinuous streams (such as CMML and Kate). While it may be desirable to consider Kate streams too (in order to avoid losing a subtitle starting near the seek point), this may be a performance drag when seeking where no subtitles are. Maybe one could add a "give up" threshold for such discontinuous streams, so we'd get any page if there is one, but do not end up reading preposterous amounts of data otherwise. In any case, it is important that the code that determines the amount of streams to look pages for remains consistent with the "early out" conditions of the code that actually parses the incoming pages, lest we never decrease the pending counter to zero. This fixes seeking on a file with a skeleton track reading all the file on each seek. https://bugzilla.gnome.org/show_bug.cgi?id=719615 --- ext/ogg/gstoggdemux.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 65ac060b94..b90ac58a2d 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -2981,11 +2981,40 @@ gst_ogg_demux_do_seek (GstOggDemux * ogg, GstSegment * segment, &best, FALSE, 0)) goto seek_error; - /* second step: find pages for all streams, we use the keyframe_granule to keep - * track of which ones we saw. If we have seen a page for each stream we can - * calculate the positions of each keyframe. */ - GST_DEBUG_OBJECT (ogg, "find keyframes"); + /* second step: find pages for all relevant streams. We use the + * keyframe_granule to keep track of which ones we saw. If we have + * seen a page for each stream we can calculate the positions of + * each keyframe. + * Relevant streams are defined as those streams which are not + * Skeleton (which only has header pages). Discontinuous streams + * such as Kate and CMML are currently excluded, as they could + * cause performance issues if there are few pages in the area. + * TODO: We might want to include them on a flag, if we want to + * not miss a subtitle (Kate has repeat packets for this purpose, + * but a stream does not have to use them). */ pending = chain->streams->len; + for (i = 0; i < chain->streams->len; i++) { + GstOggPad *pad = g_array_index (chain->streams, GstOggPad *, i); + if (!pad) { + GST_WARNING_OBJECT (ogg, "No pad for serialno %08x", pad->map.serialno); + pending--; + continue; + } + if (pad->map.is_skeleton) { + GST_DEBUG_OBJECT (ogg, "Not finding pages for Skeleton stream %08x", + pad->map.serialno); + pending--; + continue; + } + if (pad->map.is_sparse) { + GST_DEBUG_OBJECT (ogg, "Not finding pages for sparse stream %08x (%s)", + pad->map.serialno, gst_ogg_stream_get_media_type (&pad->map)); + pending--; + continue; + } + } + GST_DEBUG_OBJECT (ogg, "find keyframes for %d/%d streams", pending, + chain->streams->len); /* figure out where the keyframes are */ keytarget = target; @@ -3010,7 +3039,7 @@ gst_ogg_demux_do_seek (GstOggDemux * ogg, GstSegment * segment, if (pad == NULL) continue; - if (pad->map.is_skeleton || pad->map.is_cmml) + if (pad->map.is_skeleton || pad->map.is_sparse) goto next; granulepos = ogg_page_granulepos (&og);