After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771478 - qtmux moov-recovery-file option fails by gst-launch-1.0
qtmux moov-recovery-file option fails by gst-launch-1.0
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.8.3
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-15 08:48 UTC by Liene
Modified: 2017-03-16 05:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Recovered video not playable with ffplay (2.49 KB, text/plain)
2017-03-07 16:32 UTC, Daniel
  Details
atomsrecovery: expect more atom types at the headers (3.40 KB, patch)
2017-03-11 22:54 UTC, Thiago Sousa Santos
none Details | Review
qtmux: avoid fallthrough to moovrecovery failure section (781 bytes, patch)
2017-03-11 22:54 UTC, Thiago Sousa Santos
none Details | Review
atomsrecovery: expect more atom types at the headers (3.24 KB, patch)
2017-03-12 05:37 UTC, Thiago Sousa Santos
committed Details | Review
qtmux: avoid fallthrough to moovrecovery failure section (781 bytes, patch)
2017-03-12 05:37 UTC, Thiago Sousa Santos
committed Details | Review
atomsrecovery: also handle extra atoms after 'mdia' in a 'trak' (2.80 KB, patch)
2017-03-12 05:37 UTC, Thiago Sousa Santos
committed Details | Review
qtmux: Join chunks together if they have the same size (4.62 KB, patch)
2017-03-16 05:29 UTC, Thiago Sousa Santos
none Details | Review

Description Liene 2016-09-15 08:48:02 UTC
Hi, I have a problem to recover mp4 file made by gst-launch-1.0: gst-launch-1.0 qtmoovrecover broken-input=broken.mp4 recovery-input=recovery fixed-output=fixed.mp4

Once i start the writing stream, I get this message: GST_WARNING_OBJECT (qtmux, "An error was detected while writing to recover file, moov recovery won't work");  And no other warning.

The same pipeline with gst-launch-0.10 works and there is no such warnings. 

So I inspected the source code and found that gst-plugins-good-1.8.3/gst/isomp4/gstqtmux.c line 2068 where subroutine "fail" executes 100% of time and there moov_recov_file pointer will get NULL. So it will be created and nothing should be written in there. If that subroutine is correct, than I should get at least this warning : GST_WARNING_OBJECT (qtmux, "Failed to write moov recovery file " "headers");. But there is no such warnings in console.
Comment 1 Tim-Philipp Müller 2016-09-15 09:02:12 UTC
You will have to set the GST_DEBUG environment variable to see those warning messages:

 $ GST_DEBUG=*:WARN gst-launch-1.0 ...
Comment 2 Liene 2016-09-15 09:24:47 UTC
Sure, I did it. gst-launch-1.0 throws the warning "An error was detected while writing to recover file, moov recovery won't work" and gst-launch-0.10 throws no warnings. So gst-launch-0.10 works with moov-recovery-file options, but gst-launch-1.0 don't.
Comment 3 Alfonso 2016-11-25 10:10:00 UTC
Hello, we are also having an issue with the qtmoovrecover. However, we believe it's not the qtmoovrecover from gstreamer-1.0, but the mp4mux.

We used this pipeline (both for gstreamer 1.0 and 0.10):

gst-launch-1.0 videotestsrc ! x264enc ! mp4mux  moov-recovery-file=test1-0.mrf ! filesink location=test1-0.mp4

gst-launch-0.10 videotestsrc ! x264enc ! mp4mux  moov-recovery-file=test0-10.mrf ! filesink location=test0-10.mp4

Then we tried to recover each recording with:

gst-launch-1.0 qtmoovrecover broken-input=test1-0.mp4 recovery-input=test1-0.mrf fixed-output=test1-0_recovered.mp4

gst-launch-1.0 qtmoovrecover broken-input=test0-10.mp4 recovery-input=test0-10.mrf fixed-output=test0-10_recovered.mp4


What we found out is that we could recover the 0.10 recording with gstreamer 1.0, but not the 1.0 recording.

Afterwards, we tried recovering both files with gstreamer 0.10 and the result was the same (0.10 recording recovered, 1.0 recording not recovered)
Comment 4 Daniel 2017-03-07 16:24:09 UTC
Hi, in addition to Alfonso info. I made some tests using the script to create an uninstalled setup environment (https://github.com/GStreamer/gstreamer/blob/master/scripts/create-uninstalled-setup.sh) using Ubuntu 16.04

In this tests I find out that the bug it's between branch 1.4.5 and 1.6.4. Since the element mp4mux it's in gst-plugins-good I made a git bisect of this plugin.

Apparently, the commit who starts the bug is: https://github.com/GStreamer/gst-plugins-good/commit/1d058c7d8a3c3fdb5876e97f9706dedccff29e5f

However, while I could now recover the video, it was still not playable. I continued the git bisect and found that the recovery issues started with this commit: https://github.com/GStreamer/gst-plugins-good/commit/6321cdedb3b9bb1d7f89309c6718bd2c588d727e
Comment 5 Daniel 2017-03-07 16:32:26 UTC
Created attachment 347406 [details]
Recovered video not playable with ffplay
Comment 6 Alfonso 2017-03-10 11:22:43 UTC
Hello everyone! Is there something else we can do to help fixing this bug?
Comment 7 Thiago Sousa Santos 2017-03-11 22:54:01 UTC
Created attachment 347729 [details] [review]
atomsrecovery: expect more atom types at the headers

Skip more atoms at the header until it finds the 'mdat' to continue the
moov recovery
Comment 8 Thiago Sousa Santos 2017-03-11 22:54:10 UTC
Created attachment 347730 [details] [review]
qtmux: avoid fallthrough to moovrecovery failure section

Return before that to preserve our successfull results, otherwise no
moov recovery information would be written
Comment 9 Thiago Sousa Santos 2017-03-11 22:55:00 UTC
There is still a 3rd fix needed related to how the trak atom is read from the recovery file and rewritten in the output. Still working on this patch.
Comment 10 Thiago Sousa Santos 2017-03-12 05:37:39 UTC
Created attachment 347735 [details] [review]
atomsrecovery: expect more atom types at the headers

Skip more atoms at the header until it finds the 'mdat' to continue the
moov recovery
Comment 11 Thiago Sousa Santos 2017-03-12 05:37:43 UTC
Created attachment 347736 [details] [review]
qtmux: avoid fallthrough to moovrecovery failure section

Return before that to preserve our successfull results, otherwise no
moov recovery information would be written
Comment 12 Thiago Sousa Santos 2017-03-12 05:37:48 UTC
Created attachment 347737 [details] [review]
atomsrecovery: also handle extra atoms after 'mdia' in a 'trak'

Take into account the atoms at the end of the 'trak' atom when
recovering it. So that its size (already computed and added in the trak
size) isn't making offsets wrong.
Comment 13 Thiago Sousa Santos 2017-03-12 05:39:14 UTC
Final versions of the patches attached. Let me know how they work for you, please.
Comment 14 Thiago Sousa Santos 2017-03-15 03:40:35 UTC
commit b434ba86f1acecbc1b61fb7832aa6c5432766295
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Sat Mar 11 21:20:40 2017 -0800

    atomsrecovery: also handle extra atoms after 'mdia' in a 'trak'
    
    Take into account the atoms at the end of the 'trak' atom when
    recovering it. So that its size (already computed and added in the trak
    size) isn't making offsets wrong.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=771478

commit 7e39dec391c37ded3fb5bb7d24c60de6634647f4
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Sat Mar 11 12:56:33 2017 -0800

    qtmux: avoid fallthrough to moovrecovery failure section
    
    Return before that to preserve our successfull results, otherwise no
    moov recovery information would be written
    
    https://bugzilla.gnome.org/show_bug.cgi?id=771478

commit 4d9b17ad77ef078ceff753f41db0e61338d0e20c
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Sat Mar 11 12:27:28 2017 -0800

    atomsrecovery: expect more atom types at the headers
    
    Skip more atoms at the header until it finds the 'mdat' to continue the
    moov recovery
    
    https://bugzilla.gnome.org/show_bug.cgi?id=771478
Comment 15 Daniel 2017-03-15 11:00:02 UTC
Hello, I applied the patches to master branch in my test environment and it works fine. Also I test the patches with branch 1.8 without problems, but I didn't test it in previous versions. Will it be patched in previous versions?

Thanks!
Comment 16 Sebastian Dröge (slomo) 2017-03-15 11:06:31 UTC
Should be backported to 1.10 and included in 1.10.5, yes. Thiago, can you do that?
Comment 17 Thiago Sousa Santos 2017-03-15 14:55:18 UTC
I tested with 1.10 but didn't get a smooth playback, still looking into the reason.
Comment 18 Thiago Sousa Santos 2017-03-15 16:28:59 UTC
Doing a quick look around, it works if I also cherry-pick https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=c2225781bbafff8f70866cf06ad757befa81aa8b into 1.10 as well.

Will check what is the minimum fix that makes it work unless someone thinks this patch is safe enough to go into 1.10.
Comment 19 Thiago Sousa Santos 2017-03-16 05:29:09 UTC
Created attachment 348062 [details] [review]
qtmux: Join chunks together if they have the same size

This is the piece of the original patch that fixes it for 1.10.

Should I push this or the original version?
Comment 20 Sebastian Dröge (slomo) 2017-03-16 05:48:10 UTC
Better this version as it does not add new features, and it does not involve the addition of the new "find best pad" code.