GNOME Bugzilla – Bug 790696
matroskademux: seek without index may end up on cluster with no key-frame
Last modified: 2018-08-27 21:13:44 UTC
Created attachment 364170 [details] Test-application and example matroska-file. Hi, We have found an issue when seeking in a h264-matroske file that has very long GOP-length and no cluster index. The extra long GOP-lengt causes the matroska file to contain clusters that doesn't have a key-frame. The reason to there being no index is due to that the seek is done in an ongoing recording where the file, not yet has been completed and written to disc. When seeking in the matroska file you sometimes end up on a cluster that doesn't have a key-frame and this causes the matroska file to not being able to be correctly decoded. I have tried to make a small application (seek_app.c) that shows the issue, and yes it is a total rip off of your basic tutorial 6. I have attached a matroska file (h264_two_keyframes_cues_removed.mkv) that has two clusters including key-frames. From mkvinfo tool: Cluster at 456 Cluster timecode: 0.000s at 468 SimpleBlock (key, track number 1, 1 frame(s), timecode 0.000 = 00:00:00.000) at 471 Cluster at 81574 Cluster timecode: 114.932s at 81586 SimpleBlock (key, track number 1, 1 frame(s), timecode 114.932 = 00:01:54.932) at 81595 There is no index in the file, to simulate an ongoing recording. The test-application works something like this: The seek is started after 10 seconds #define TIME_SEEK_START 10 I have tested doing three different seeks at 40, 130 and 160 seconds: #define TIME_SEEK_POS 40 #define TIME_SEEK_POS 130 #define TIME_SEEK_POS 160 If a seek is done to TIME_SEEK_POS 40 the seek will find a cluster at offset 72483 with time 0:00:33.466000000. The cluster at offset 72483 does not contain any key-frame. The seek continues at 0:00:33.466000000 and the video can not be decoded. The seek should have found cluster at offset 456 with time 0:00:00.000000000 that contains a key-frame. matroska-demux.c:2137:gst_matroska_demux_search_pos:<matroskademux0> found cluster at offset 72483 with time 0:00:33.466000000 matroska-demux.c:2216:gst_matroska_demux_search_pos:<matroskademux0> simulated index entry; time 0:00:33.466000000, pos 72439 If a seek is done to TIME_SEEK_POS 130 the seek will find a cluster at offset 81574 with time 0:01:54.932000000. The cluster at offset 81574 contains a key-frame. The seek continues at 0:01:54.932000000 and the video can be decoded. matroska-demux.c:2137:gst_matroska_demux_search_pos:<matroskademux0> found cluster at offset 81574 with time 0:01:54.932000000 matroska-demux.c:2216:gst_matroska_demux_search_pos:<matroskademux0> simulated index entry; time 0:01:54.932000000, pos 81530 matroska-demux.c:2415:gst_matroska_demux_handle_seek_event:<matroskademux0> seek to key unit, adjusting segment start from 0:02:10.000000000 to 0:01:54.932000000 If a seek is done to TIME_SEEK_POS 160 the seek will find a cluster at offset 133909 with time 0:02:27.932000000. The cluster at offset 133909 does not contain any key-frame. The seek continues at 0:02:27.932000000 and the video can not be decoded. The seek should have found cluster at offset 81574 with time 0:01:54.932000000 that contains a key-frame. matroska-demux.c:2137:gst_matroska_demux_search_pos:<matroskademux0> found cluster at offset 133909 with time 0:02:27.932000000 matroska-demux.c:2216:gst_matroska_demux_search_pos:<matroskademux0> simulated index entry; time 0:02:27.932000000, pos 133865 matroska-demux.c:2415:gst_matroska_demux_handle_seek_event:<matroskademux0> seek to key unit, adjusting segment start from 0:02:40.000000000 to 0:02:27.932000000 We have a local patch that works on our code, but it does not work on the latest Gstreamer code (1.13.x), for gst-plugins-good (matroska-demux.c). We are working on making the patch work but we are not there yet, so any good input, on making a patch, is greatly appreciated. Regards /Mats
What version of gst-plugins-good is your original patch for ? You can still attach it here as a reference.
Created attachment 364183 [details] [review] Patch used in our local code. This was also aimed for upstreams code. We are using gst-plugins-good version 1.10.4. I have added the patch that was intended for the upstream code, but it is identical to the patch that we are using in our code. Apart from: GST_DEBUG_OBJECT (demux, "undershot target"); - /* ok if close enough */ - if (GST_CLOCK_DIFF (cluster_time, time) < 5 * GST_SECOND) { - GST_DEBUG_OBJECT (demux, "target close enough"); - prev_cluster_time = cluster_time; - prev_cluster_offset = cluster_offset; - break; + if (has_keyframe) { + /* ok if close enough */ + if (GST_CLOCK_DIFF (cluster_time, time) < 5 * GST_SECOND) { + GST_DEBUG_OBJECT (demux, "target close enough"); + prev_cluster_time = cluster_time; + prev_cluster_offset = cluster_offset; + prev_has_keyframe = has_keyframe; + break; + } I hope that is ok. The patch used for our are actually two separate patches. I can add them if that is something that you think is something you can use.
Hi, Have you been able to reproduce the issue using the matroska-file and test-app? Regards /Mats
Created attachment 364663 [details] [review] Updated patch that works on 1.13.x upstream code. New patch that is reworked to for upstream 1.13.x code.
Hi, Has anyone had a chance to look at the patch yet? Cheers.
Hi, Has anyone had a chance to look at the patch yet? Regards /Mats
Hi, Is there anyone in the GStreamer community that is going to the Hackfest in Lund 4/5 - 6/5? Maybe there we could find some time to discuss this issue? Regards /Mats
According to the registration list, a big part of the GStreamer developers is coming to the hackfest.
That's great news. What I would like to know if anyone is up to discussing this issue. It doesn't seem polite just blindsiding any guy.
Sure thing, find me at the hackfest. I think the issue is clear, it's just that someone needs to make time to review the patch.
It's still on my list, but I haven't gotten a chance to merge it during the hackfest, sorry. One thing I need to check (I don't remember right now) is if there is some kind of limit for the cluster backtracking/scanning - do we want to keep scanning back to the beginning of the file in the worst case, or abort after say 5 clusters or so?
There is no limit to the backtracking. The backtracking goes all the way to the start of the file if that is needed. Priority is to find a I-frame and to be able to decode the movie-track.
Hi, Any news on this bug? Regards /Mats
Created attachment 373340 [details] [review] matroskademux: extract cluster prevsize if available
Created attachment 373341 [details] [review] matroskademux: try to ensure keyframe when seeking without index
Created attachment 373342 [details] [review] matroskademux: figure out if we have prev_size when starting up
Created attachment 373343 [details] [review] matroskademux: no need to search for keyframes for intra-only streams
Created attachment 373344 [details] [review] matroskademux: set limit how much to backtrack to find a keyframe
Created attachment 373345 [details] [review] matroskademux: make max backtrack distance for keyframe search configurable
Created attachment 373346 [details] [review] matroskademux: implement keyframe search also without cluster prev size
I have implemented this slightly differently based on some of your original code. Would be great if you could test it to see how it works for you. I'm not convinced we can really use the bisect loop to backtrack and look for a keyframe as well, but in any case I found it difficult to keep track of all the logic and the global state and how it interacts here in all the scenarios, so I have decided to do it differently in a more verbose manner: by first finding the right cluster via bisecting and then backtracking cluster by cluster in a separate loop. We should also use cluster prev_size if available, since this is much more efficient. We already keep track of which video tracks are intra-only, so should re-use the existing logic for that instead. I have also added a limit to the backtracking. I realise this does not work for you like that, but we need to have this work by default with a variety of different input files, and it's entirely possible that we have files where only the first cluster starts with a keyframe and otherwise keyframes are in the middle of clusters. We can't just scan back to the very beginning every time in that case. But the max-backtrack-distance is configurable via a property, so hopefully that works for your use case. You can hook up to the pipeline's "deep-element-added" property to catch matroskademux and configure it if you don't have direct access to the instance.
Patches are also here for easier cherry-picking fwiw: https://cgit.freedesktop.org/~tpm/gst-plugins-good/log/?h=matroskademux-scan-clusters-for-keyframe-2
Thank you for the patches. Will test them ASAP. Sorry for being dense, but I am not able to cherry-pick form the link you attached. Can not find the "matroskademux-scan-clusters-for-keyframe-2"-branch. The link seems to be on another git.
Is the intention that all patches should be applied, or is the intention to pick the ones that "we" need?
> Sorry for being dense, but I am not able to cherry-pick form the link you > attached. Can not find the > "matroskademux-scan-clusters-for-keyframe-2"-branch. The link seems to be on > another git. This should work: $ git remote add tpm-fdo git://people.freedesktop.org/~tpm/gst-plugins-good (I'm sure there's an http url too, but don't know it right now) $ git fetch tpm-fdo $ gitk tpm-fdo/matroskademux-scan-clusters-for-keyframe-2 & > Is the intention that all patches should be applied, or is the > intention to pick the ones that "we" need? You probably don't need the two first ones ("WIP: gdkpixbufsink-test") and the last one ("matroskademux: implement keyframe search also without cluster prev size") is also optional for you, but shouldn't do any harm. I also forgot to mention that I ran into some issues when testing the initial patch: when scrubbing/seeking towards the end of the file (beyond start of last cluster? don't remember) I could easily trigger fatal seeking errors (though I'm sure they could be fixed of course). I could only trigger it with a certain sequence of seeks though.
Thank you for the tip. Worked just fine once I got the proxies sorted out.
Hi, Now I have tested this fix (I used all seven patches) and it seems all good to me. What happens from now on? Should I wait for this fix to be available on your master branch and then download it from there? BR
I will merge it soon then, thanks for testing!
commit 9d6621a30dad77b69afd24e6b25792a2dcf78da9 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 15 12:53:34 2018 +0100 matroskademux: implement keyframe search also without cluster prev size If we have cluster prev size (GStreamer muxer will write it by default), we can go back to the previous cluster efficiently, but if we don't then just search backwards until we find a cluster ebml identifier, like we do when searching for clusters in the bisection loop. commit 2d6efbbae20a2f53bed19eb76643b45ee4a8756a Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 15 12:14:24 2018 +0100 matroskademux: make max backtrack distance for keyframe search configurable Add property instead of hardcoding it in the code. In some scenarios such as CCTV variable fps and extra long GOPs are used to minimise storage space, for example. In those cases there might not be any keyframes for many minutes, so provide a property to override the max allowed distance. https://bugzilla.gnome.org/show_bug.cgi?id=790696 commit 2990f0730ae5428aed890d3d754b987be69aa175 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 15 11:49:57 2018 +0100 matroskademux: set limit how much to backtrack to find a keyframe If we seek without an index and land on a cluster that starts with a delta frame. https://bugzilla.gnome.org/show_bug.cgi?id=790696 commit ffb4533137405585d526f365ba204bd2fc023f67 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 15 11:25:21 2018 +0100 matroskademux: no need to search for keyframes for intra-only streams If the video streams are all I-frame only then we don't need to look for a cluster with a keyframe, we can just assume there will be one. https://bugzilla.gnome.org/show_bug.cgi?id=790696 commit 694631520ddedabc2bd7a4a65691989249b8143e Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 15 01:10:32 2018 +0100 matroskademux: figure out if we have prev_size when starting up This is useful to know in case someone initiates a seek or direction change before we reach the second cluster. commit 43ce85f794f7599e81483c75c0e31147189755e3 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 8 12:37:54 2018 +0100 matroskademux: try to ensure keyframe when seeking without index When seeking in pull mode without an index (because there is no index or the file is still being written to) we bisect to find the right cluster to jump to. However, it's possible the cluster we found doesn't start with a keyframe, which leads to decoding errors, so if we know that the found cluster starts with a delta frame try to scan back to previous clusters until we find one that starts with a keyframe or we are back at the beginning. Theoretically it's possible that all clusters but the first one do not start with a keyframe and the keyframes are in the middle of clusters, but this is extremely unusual, so we will cover this case with a basic sanity check. This problem is especially problematic with content recorded with dynamic GOP and FPS, where long GOP lengths and low FPS may cause a large set of clusters to lack key frames. Playback would then be started on a non-keyframe cluster, and the large number of such frames would make the content impossible to decode fo a long stretch of time. Based on patch by: Mats Lindestam <matslm@axis.com> https://bugzilla.gnome.org/show_bug.cgi?id=790696 commit 93ddea2a70991f24f2b4de81d2d9beb0136c9a5e Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Jan 18 10:27:38 2017 +0000 matroskademux: extract cluster prevsize if available This is useful for reverse playback/trickmodes without an index, and will also be useful in the seek handler if we need to scan back to find a cluster that starts with a keyframe. https://bugzilla.gnome.org/show_bug.cgi?id=790696