GNOME Bugzilla – Bug 601576
qtmux feature: moov recovery
Last modified: 2010-02-02 17:37:17 UTC
Created attachment 147487 [details] [review] qtmux enhancements The attached patch was made against gst-plugins-bad 0.10.12 tarball, but uses qtmux from 0.10.14 as its start point. The patch addresses the following needs: * Ability to recover a recording, if the application dies before the moov atom is written to file. This is useful for battery powered devices. Refer to tests/examples/qtmux/qtmuxrecover.c for usage. * By addressing the above (writing sample and codec data to an xml file), the opportunity presented itself to improve on the atom context memory usage. Previously, the whole atom context was stored in memory and could grow in excess of 32MB. During moov construction the patch will not take more than 32KB * Edit list support. By using edit atoms, it is possible to link live sources to qtmux in a running pipeline and maintain audio/video sync. The example in tests/examples/qtmux links/unlinks audiotestsrc on a 3 second timer, playable in quicktime. That patch also includes a faac fix I previously submitted. On first time building do a 'make install' in order to install the header needed by qtmuxrecover.
Hrm, cool stuff (I think?), but this patch is really quite large. Do you think you could - submit a patch that does not include generated files (Makefile.in in particular), preferably against git, especially for such a large patch - open separate bugs for separate issues (the edit list support does not depend on the other stuff, does it?) - not include the faac fix (please just ping the existing bug if we forgot about it) ? This would make reviewing and discussing this *much* easier. I must say the xml serialisation/deserialisation looks quite painful. Is this really the easiest/best way to do this? Not only does it seem awkward and unnecessarily verbose to me, but I'm also not very keen on seeing an external dependency added just for this feature (others might disagree of course).
The (great) majority of this patch is a re-write of the qtmux's atom context, and therefore this should really be considered a re-write of qtmux. Since the edit list portion of this patch is implemented using the new atom context code, it doesn't really make sense to split this into a separate patch. If you like I can file separate reports and reference this report (and patch). On making the suggested patch changes.. I will try, time permitting. On serialization/de-serialization. I don't believe it is painful, in fact I believe the opposite is true, and to enable the first feature I listed (moov recovery), it is necessary. The argument could be made to use something less verbose than xml. I have tried to keep xml tag and property name lengths to a minimum in order to keep xml file lengths to a minimum. The ease and simplicity of parsing xml keeps the code and logic simple without, IMO, too much added overhead.
I like the idea too - Thanks for working on it. But I would rather see this using libxml2 instead of expat, if at all. Could you please attach a small sample recovery file also (to give people an idea). If the data is not too much hierarchical we could maybe use a GKeyfile.
Why does this need to be serialised in xml or some clear-form at all? Can't we just save/dump binary data for recovery purposes? What's the use-case for the recovery btw? Just fixing up the missing atoms? Continuing? (If the latter, why should continued recording after recovery be supported?)
Uhm, yep a binary format would just be fine for me. For me the use-case is to recover a terminated recording. Record and kill the process (mobile device runs out of battery, recording app crashed, ...), the resulting file is not playable. I don't see a need for continued recording though.
The way qtmux currently does it, is to build up the moov atom data in memory. There are two problems with this. First, for lengthy recordings this can get quite large, and secondly, if a device were to suddenly die (e.g a battery powered device) the moov atom data would just disappear making recovery practically impossible. The reason we can't just amend the current code to write out its moov data to a file, and simply paste that at the end of the mdat atom during recover, is due to the way atoms are constructed. Atoms can contain child atoms (moov being the top atom), and each atom contains a header with minimally two items.. type and size. So moov construction can only occur after a recording has finished. I'll attach a sample xml file.. as always with xml, it helps to have it in human readable form.
Created attachment 148138 [details] Sample xml file containing codec and sample data.
Created attachment 149840 [details] [review] Adds moov recovery Patch that adds moov recovery feature (and only that) to qtmux. This patch tries to avoid being intrusive in qtmux code and uses binary data to store the recovery file. It also includes a tool/application to recover files. The important code is in atomsrecovery.[ch]. This is a work in progress and the code needs some polishing and testing and I'm working on that, just wanted to share here so I can have feedback earlier. Things I plan to work on: 1) Improve the support on qtmux by correctly handling error cases. 2) Use GError for returning error messages to the users. 3) Try to come up with a solution for tags handling. Haven't tested when qtmux is on 'faststart' mode yet.
Forgot to mention that atomsrecovery.c has a small doc in the top of it with some details on the recovery file.
Created attachment 149998 [details] [review] improved recovery patch Improved patch, added some error reporting, fixed some FIXME and now it works for faststart mode as well.
Review of attachment 149998 [details] [review]: Good work. I only have some minor nitpicks. ::: gst/qtmux/atomsrecovery.c @@ +84,3 @@ + * IMPORTANT: this is still at a experimental state. + */ + What about including a version here. The version can be bumped and the tool can refuse to recover to load mrf files of wrong version. @@ +211,3 @@ + GST_WRITE_UINT64_BE (data + 26, 0); + } + minor nitpick, the two if(sync) could be joined. @@ +247,3 @@ +} +#endif + remove? @@ +829,3 @@ + buffer = g_malloc0 (size); + offset = 0; + Does this needs to be cleared? Also what is the 1024*1024 - just a decent upper limmit? @@ +881,3 @@ + "Failed to read the ftyp atom from file"); + return FALSE; + } please use data=malloc(moovrf->prefix_size), instead data[1024*1204] here. @@ +924,3 @@ + ATOMS_RECOV_OUTPUT_WRITE_ERROR (err); + return FALSE; + } If data[] is only used here and once (where size is staticaly known) below it can be made small (16 bytes) @@ +936,3 @@ + ATOMS_RECOV_OUTPUT_WRITE_ERROR (err); + return FALSE; + } again use malloc() for data @@ +993,3 @@ + ATOMS_RECOV_OUTPUT_WRITE_ERROR (err); + goto fail; + } again malloc() + free() the buffer. ::: gst/qtmux/atomsrecovery.h @@ +72,3 @@ + gboolean sync; + gboolean do_pts; + guint64 pts_offset; minor nitpick: putting the pts_offset before the two gboolean can save some memory from alignment gaps (can be checked with pahole tool) ::: gst/qtmux/gstqtmux.c @@ +2491,3 @@ break; case PROP_FAST_START_TEMP_FILE: + g_free (qtmux->fast_start_file_path); unrelated change that could be applied right away separately ::: gst/qtmux/qtmoovrecover.c @@ +61,3 @@ + + if (argc != 4 && argc != 5) { + g_print ("Usage: %s moovrecoveryfile mdatfile outputfile [-fs]\n", argv[0]); please make this --fast-start, options args should be -f (single letter) or --long-name (double dash -> long name)
To test I just used this: gst-launch videotestsrc num-buffers=1000 ! x264enc ! qtmux moov-recovery-file=test.mrf ! filesink location=test.broken.mov and to repair ./gst/qtmux/qtmoovrecover test.mrf test.broken.mov test.fixed.mov would be good to change the help output of qtmoovrecover -Usage: ./gst/qtmux/qtmoovrecover moovrecoveryfile mdatfile outputfile [-fs] +Usage: ./gst/qtmux/qtmoovrecover moovrecoveryfile inputfile outputfile [-fs] gstreamer can play the repaired file just fine, but mplayer complains (no idea if for a good reason or not) mplayer: /build/buildd/ffmpeg-0.5+svn20090706/libavformat/mov.c:1999: mov_read_packet: Assertion `sc->ctts_data[sc->ctts_index].duration % sc->time_rate == 0' failed.
Created attachment 152404 [details] [review] Updated patch Updated the patch with Stefan's comments. One thing we still don't handle is the EDTS atom (that qtmux currently only uses to indicate streams that start later than others). But that isn't really common. One solution is to delay the writing of the moov and prefix atoms to the recovery file until all streams get buffers (so we know about stream delays). I think this is fine for the EDTS for the way qtmux uses it currently. Another option is to add a kind of code before each buffer entry, so that we can mix buffer entries with another info that we might be receiving mid stream and might not know right from the beginning (tags, for example).
I've tested in mplayer, vlc, totem and Quicktime (on mac) and had no problems playing recovered files.
Thanks for making the changes. This looks really good now. I think Marks changes broke the patch. Could you please re-attach (hope it rebases for you). Also I think it could be committed now.
Ok, I'll try rebasing it. I'll add the 'marker' before each buffer entry (should make future changes easier) One issue left open is where to put the recover libs/apps for the public usage.
Created attachment 152575 [details] [review] Updated patch [2] Updated patch after Mark patches
We had a discussion on IRC about qtmoovrecover. The consensus is to turn that into an element (part of qtmux plugin). This solves the problem of where to install it and we avoid adding a mini library. The plugin should subclass from GstPipeline and will have the qtmoovrecoder commandline args as gobject properties - recover-file, broken-input, fixed-output, fast-start-mode.
Created attachment 152746 [details] [review] qtmux: Adds moov recovery feature Adds a new property to qtmux that sets a path to a file to write and update data about the moov atom (that is not writen till the end of the file). If the pipeline/app crashes during execution it might be possible to recover the movie using the qtmoovrecover element. qtmoovrecover is an element that is also a pipeline. It is not meant to be used with other elements (it has no pads). It is merely a tool to recover unfinished qtmux files. Fixes #601576 This patch updates the previous one transforming the qtmoovrecover tool into an element, as decided.
Thanks for the patches, Roland, but I committed the binary version of it as human-readability here is not really important, hope it meets your needs/use-cases. Feel free to reopen if it doesn't and we can work on that. There is also the memory usage and edit lists issues that you reported altogether here, but this bug kind of led itself in the moov recovery feature (as that drawn more interest). You can open new bugs for the other features, too. I'm closing this one. Module: gst-plugins-bad Branch: master Commit: e1c1405396d830e7414b141750722c044b40d835 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=e1c1405396d830e7414b141750722c044b40d835 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Sat Dec 12 16:07:15 2009 -0300 qtmux: Adds moov recovery feature Adds a new property to qtmux that sets a path to a file to write and update data about the moov atom (that is not writen till the end of the file). If the pipeline/app crashes during execution it might be possible to recover the movie using the qtmoovrecover element. qtmoovrecover is an element that is also a pipeline. It is not meant to be used with other elements (it has no pads). It is merely a tool/utilitary to recover unfinished qtmux files. Fixes #601576
Comment on attachment 152746 [details] [review] qtmux: Adds moov recovery feature Committed, but with a few minor changes