GNOME Bugzilla – Bug 652195
matroskademux: seeking in non-finalized matroska files does not work correctly
Last modified: 2011-12-21 18:31:13 UTC
Created attachment 189549 [details] [review] Patch for improving seeking in not-yet-finalized files Create the following pipeline. filesrc ! matroskademux ! queue ! ffdec_h264 ! xvimagesink Put the pipeline in PAUSED state. Do a seek a bit into a matroska file and then put the pipeline in playing state. This works fine with a matroska file that is finalized, i.e. finished. But if you try the same with a matroska file that is not finalized, e.g. still being recorded, then seeking is only possible in the segment defined by the buffers queued in the queue. When playing a file that is still being recorded the duration is not yet written to the matroska file header, and the demuxer will update it's segment's duration whenever it reads a new frame from the file. In the case of the pipline above it means that the duration will be what the 'max-size-time' property of the queue is set to. Because that's how much the demuxer has read so far. Then, when creating the seeksegment, gst_segment_set_seek() will CLAMP the start and stop variables so that they do not exceed the actual duration of the segment. I have attached a attached a patch to the demuxer that I think solves the problem. Could you please take a look at it? /Branko
Created attachment 190041 [details] [review] Avoid looping when searching for clusters in a file still being recorded
I'm not so sure about the seeking patch. Not that I think it's wrong, I just don't really think it improves behaviour that much. Isn't there something more clever we can do in the demuxer, like scan backwards from file end to get a timecode and figure out the duration? Check upstream for size changes? etc.
Created attachment 196504 [details] test prog Attached a small C-program that shows the problem. It sets up a pipeline as described above, pauses, seeks from 5-10 secs, and then goes to play. Works fine with a finalized file. With a non-finalized file, however, it only works with the seek patch.
Created attachment 203953 [details] [review] Scan file for time code of the last block in the last cluster New way of handling files with unknown duration. Before the seek is done a check is made to see if the file size has changed since the previous seek. If it has then the file is scanned backwards to find the last cluster. The time code of the last block in the last cluster will be set as new duration.
Created attachment 203954 [details] [review] Adds function for scanning byte reader backwards Make sit possible to scan the byte reader data backwards
Created attachment 204040 [details] [review] matroskademux: do not consider duration of non-finalized file While it is slightly painful saying so given the effort that has been put into the previous patches, it seems those patches introduce a whole lot of code and complexity for a play/seek-file-being-created (corner) case More importantly, however, I believe the present patch also supports this corner case in a much easier and clearer way, namely simply invalidate the (currently determined) duration in the segment (to avoid it messing with requested seek position). This is also not really a hack, since the duration being invalidated is not really that valid/informative anyway (and only temporary). Likewise, the duration being determined (by lots of code seeking/scanning to the end) is only "marginal" in that it will be outdated the moment it is determined (as the file is still growing in this case). In particular, e.g. truncating a requested segment.stop position to an updated (but already outdated) duration would not be quite right either, since the file could still grow that position. And moreover, performing a seek near the end will have the duration updated anyway (when streaming resumes at that point).
Well, my first attempt was to set the duration of the seeksegment to -1, i.e. to invalidate the duration. Patch is included in my first post above. That solved the problem. Those few lines, perhaps combined with checking whether the file size has changed, would solve the problem with ~350 lines less of code than my last patch. Would that be OK?
It would be OK in my book; the hackish-level of the first version has been somewhat mitigated by doing the size check (in view of Comment #2). And as said, doing a whole lot of clever stuff which will only yield outdated results (given the situation of a moving target) seems marginally beneficial (to say the least). But that may all be just me ... ;)
Couldn't agree more ;-) Sorry for my previous post. Didn't see that you had already done what I suggested. However, I think we need to change the patch slightly. It doesn't work with a non-finalized file that doesn't grow, for example a file that is the result of a recording being interrupted. In that case only the first seek will result in the duration being invalidated. The following seeks the file size will not have changed, and the current duration of the segment will be used to clamp the seek. We either have to invalidate the duration every time (for non-finalized files only, of course), or save the duration in the segment.
Hm, yes, how to detect/define non-finalized; probably checking whether file has a valid recorded duration ? So, what about: * if duration is ever updated by _parse_block whatever => assume file does not have a valid duration => invalidate when seeking * otherwise, do not mess with duration
Created attachment 204046 [details] [review] Always invalidate duration for non-finalized files Last (?) patch. If the segment duration is ever updated in the gst_matroska_demux_parse_blockgroup_or_simpleblock function the file is considered non-finalized, and the duration is always invalidated when seeking.
OK, thanks, committed: --- master ---- commit a7d6690f92e6c739d3b3e28d99d1f7840f8c59d9 Author: Branko Subasic <branko@axis.com> Date: Wed Dec 21 17:43:10 2011 +0100 matroskademux: do not consider duration of non-finalized file ... to avoid it clamping requested seek position. Non-finalized file case, determined by whether _parse_blockgroup_or_simpleblock ever updates the segment duration. Fixes #652195. ---- 0.10 ---- commit 49b228fe33fbe6360a30563277a2c222bc729cf7 Author: Branko Subasic <branko@axis.com> Date: Wed Dec 21 17:43:10 2011 +0100 matroskademux: do not consider duration of non-finalized file ... to avoid it clamping requested seek position. Non-finalized file case, determined by whether _parse_blockgroup_or_simpleblock ever updates the segment duration. Fixes #652195.
Great, thanks for taking the time.