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 784258 - qtmoovrecover can't recover video
qtmoovrecover can't recover video
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-06-27 19:00 UTC by Gregoire
Modified: 2017-10-25 06:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
isomp4: atomsrecovery: handle common and large atom headers (2.11 KB, patch)
2017-07-06 05:25 UTC, Thiago Sousa Santos
committed Details | Review
atomsrecovery: read from mdat only what is on headers (4.55 KB, patch)
2017-07-09 05:15 UTC, Thiago Sousa Santos
committed Details | Review

Description Gregoire 2017-06-27 19:00:04 UTC
This bug seems to be a regression because I had this recovering feature working in the past - though it was not on the exact same video.

I have two videos that have been encoded with gstreamer mp4mux moov-recovery-file. One reaches the EOS, the other is stopped abruptly.

Both videos can't be recovered: http://gentil.com/tmp/qtmoovrecover.zip
Comment 1 Gregoire 2017-06-27 19:04:10 UTC
Here is a simpler way to show case the bug:


gst-launch-1.0 videotestsrc ! x264enc ! qtmux moov-recovery-file=test.mrf ! filesink location=test.mov

gst-launch-1.0 qtmoovrecover recovery-input=test.mrf broken-input=test.mov fixed-output=testo.mov
Comment 2 Thiago Sousa Santos 2017-06-30 02:45:52 UTC
What version have you used? Git master?

Recently (early March) some fixes for qtmoovrecover were merged and it seems to be working for me after that.
Comment 3 Gregoire 2017-07-01 19:24:07 UTC
Progress but still not working for me.

More precisely:

- I have Ubuntu 16.04 with 1.8.3.

- I have downloaded the Ubuntu gstreamer1.0-plugins-good sources.

- I have applied 4 patches from https://cgit.freedesktop.org/gstreamer/gst-plugins-good/log/gst/isomp4:

2017-03-27	atomsrecovery: Error out when fseek() fails instead of silently ignoring
ea4e9fc2d4904117255c80c0b832253fb31d4f7a

2017-03-15	atomsrecovery: also handle extra atoms after 'mdia' in a 'trak'
b434ba86f1acecbc1b61fb7832aa6c5432766295

2017-03-15	qtmux: avoid fallthrough to moovrecovery failure section
7e39dec391c37ded3fb5bb7d24c60de6634647f4

2017-03-15	atomsrecovery: expect more atom types at the headers
4d9b17ad77ef078ceff753f41db0e61338d0e20c


Strictly speaking, I don't have git master but I truly believe that with those 4 patches I have the same thing as git master.

The two command lines I wrote at "2017-06-27 19:04:10 UTC" are now working!

But the files provided at "2017-06-27 19:00:04 UTC" can't still be recovered though there is no more error message.

What's the problem?
Comment 4 Thiago Sousa Santos 2017-07-02 00:28:12 UTC
Without 7e39dec391c37ded3fb5bb7d24c60de6634647f4 on qtmux it is unlikely that those files have enough information to be recovered with this tool. The .mrf should not have information about the position of the samples in the .mp4 so it won't be able to reconstruct the headers.

How important is recovering those files? Or are you just trying to understand why it won't work with those 2 files?
Comment 5 Gregoire 2017-07-02 00:44:11 UTC
OK, I start to understand. 7e39dec391c37ded3fb5bb7d24c60de6634647f4 is for the "failing" encoder side. So I need to commit that to my embedded device. Very painful because I'm going to have to recompile everything just for one small missing "return". Is there a way to push this pasth to ubuntu 16.04 1.8.3 package?


No. Those files were just samples to test the features before release. Luckily, I can control both the encoder embedded device and the PC on which I do the recovery.


I'm still confused about one point: I'm using mp4mux not qtmux. Is there any difference?
Comment 6 Nicolas Dufresne (ndufresne) 2017-07-02 14:12:20 UTC
Its the same element with a small behaviour change.
Comment 7 Gregoire 2017-07-05 18:46:25 UTC
It starts working but still not perfect.

I have both on encoder-device and PC-fixer gstreamer 1.8.3 with the four patches mentioned in this thread.

I can recover a video and play it but the output of mp4info is not very clean.

Here are the files of this test: http://gentil.com/tmp/qtmoovrecover2.zip

20160530-071745.mp4: ORIGINAL BROKEN FILE
a.mp4: FIXED FILE


mp4info 20160530-071745.mp4 
mp4info version -r
20160530-071745.mp4:
ReadChildAtoms: "20160530-071745.mp4": In atom  missing child atom moov
FindIntegerProperty: no such property - moov.mvhd.modificationTime (src/mp4file.cpp,746)
mp4info: can't open 20160530-071745.mp4


mp4info a.mp4 
mp4info version -r
a.mp4:
ReadAtom: "a.mp4": invalid atom size, extends outside parent atom - skipping to end of "" "�:��" 754660230 vs 18767154
ReadAtom: "a.mp4": atom type �:�� is suspect
Track	Type	Info
1	video	H264 Baseline@4.2, 13.510 secs, 9716 kbps, 1920x1440 @ 29.829756 fps
2	audio	MPEG-1 Audio (11172-3), 13.740 secs, 0 kbps, 44100 Hz
ReadAtom: "a.mp4": invalid atom size, extends outside parent atom - skipping to end of "" "�:��" 754660230 vs 18767154
ReadAtom: "a.mp4": atom type �:�� is suspect



Is it possible to have a really clean-up version of the fixed video?
Comment 8 Thiago Sousa Santos 2017-07-06 05:25:42 UTC
Created attachment 355002 [details] [review]
isomp4: atomsrecovery: handle common and large atom headers

Do not assume all files are large files. Check and use the short or
extended atom size field only if needed.
Comment 9 Gregoire 2017-07-06 06:53:05 UTC
I have applied the patch but I'm still getting a similar output for mp4info on the provided example. Am I missing something?
Comment 10 Thiago Sousa Santos 2017-07-06 07:01:19 UTC
Seems to work here with git master after applying this patch:

$ mp4info testo.mov 
mp4info version -r
testo.mov:
ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom
Track	Type	Info
1	video	H264 Unknown Profile f4@1.3, 30.500 secs, 1498 kbps, 320x240 @ 30.000000 fps
ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom
Comment 11 Gregoire 2017-07-06 07:27:28 UTC
I have tried again and no luck. Upgrading to git master triggers way too many things... A diff between 1.8.3+5_patches and git_master shows no difference.

Any idea?
Comment 12 Thiago Sousa Santos 2017-07-06 08:16:06 UTC
Hard to tell. Maybe something is different in qtmux in 1.8.

Can you share the .mov, the .mrf and the recovered .mov files after those patches to see if we can identify what is different?

Additionally if you could try a setup with everything git master just to confirm this fix indeed works for master.

Thanks.
Comment 13 Gregoire 2017-07-06 08:32:25 UTC
Yes, tomorrow morning I will try a cerbero. But I don't want to mess my whole Ubuntu machine and overwriting all gstreamer.

My command line is:
gst-launch-1.0 qtmoovrecover recovery-input=20160530-071745.mrf broken-input=20160530-071745.mp4 fixed-output=a.mp4

File is there: http://gentil.com/tmp/a.zip
Comment 14 Gregoire 2017-07-06 17:39:26 UTC
It's not working for me with a clean Cerbero + applying the patch. Am I doing something wrong? Can you send your atomsrecovery.c file? Mine: https://pastebin.com/ZWAp0rjH
Comment 15 Thiago Sousa Santos 2017-07-07 03:48:36 UTC
(In reply to Gregoire from comment #13)
> Yes, tomorrow morning I will try a cerbero. But I don't want to mess my
> whole Ubuntu machine and overwriting all gstreamer.
> 
> My command line is:
> gst-launch-1.0 qtmoovrecover recovery-input=20160530-071745.mrf
> broken-input=20160530-071745.mp4 fixed-output=a.mp4
> 
> File is there: http://gentil.com/tmp/a.zip

Would you mind sharing the original unrecovered .mov and .mrf files so I can check them? Maybe your generated files have something different from the ones from videotestsrc.

Also, you can use gst-uninstalled script (gstreamer/scripts) to set up an uninstalled environment to try master without having to install to your system.
Comment 16 Thiago Sousa Santos 2017-07-07 04:16:30 UTC
Never mind, found the files that cause the issue.
Comment 17 Thiago Sousa Santos 2017-07-07 04:42:42 UTC
The mdat is longer than its size by 2133036 bytes. Can you share the exact pipeline you are using to create these files? I'm guessing it might be related to some parameter you are using.
Comment 18 Gregoire 2017-07-07 05:29:30 UTC
The pipeline is:

appsrc name=source do-timestamp=true format=3 caps=video/x-raw,format=I420,width=1920,height=1440,framerate=30/1 ! nxvideoenc bitrate=10000000 ! queue ! mp4mux.video_0 alsasrc do-timestamp=true ! lamemp3enc ! mp4mux.audio_0 mp4mux moov-recovery-file=video.mrf name=mp4mux ! queue ! filesink location=video.mp4

nxvideoenc is a hardware-based encoder provided by Samsung/Nexell.

The device runs the Gstreamer stack 1.8.3 from Ubuntu 16.04LTS on ARM-64bit.
Comment 19 Thiago Sousa Santos 2017-07-08 20:47:10 UTC
unfortunately I couldn't reproduce it by going back to 1.8.3. Also I don't have nxenc or an ARM64 platform to test. Let me put some information here in case it helps you on anyone else debugging this further.

The recovery element takes 2 inputs, one is the .mov and another is the .mrf. In the .mov there is only the stream data, just a massive blob. It starts with a "ftyp" and maybe some minor atoms and then there should be the 'mdat' atom that has all the data inside it. A quick read on the quicktime format spec should give a basic idea of how these files are structured.

To recover this file, the .mrf file actually stores header information, which is the size, position and timing information for every sample that was stored to the .mov file. By combining those two it is possible to create a full mov/mp4 file that has the full headers and the data.

In your case, the mdat file seems to be longer than what the mrf headers have stored and it will cause some extra random binary data to be placed at the end of the file. There is one way to improve this by limiting the mdat output to only write to disk what is actually in the mrf headers but that would only hide the real problem that is why that extra data is in the end on the first place. Maybe by adding some extra debgugging to your setup you can figure out where does that come from and why there isn't headers in the mrf for that data.
Comment 20 Gregoire 2017-07-08 21:23:21 UTC
First, I appreciate your efforts.

Please correct me if I'm wrong: from what I have understood, the problem is more during the encoding stage where the mdat is longer than the mrf headers or said differently, the mrf headers are shorter than the mdat. Perhaps there is an issue to write the mrf headers to file.

What I'm confused about is that the mdat file is written at the beginning of the file, so how can the system know how long is going to be the mdat or the mrf headers?

Anyway, the investigation is more on the encoding stage rather than during the pure recovery.

Could you point in the code where I should hack/patch the recovery phase so that the mdat is trunk-ed to the size of the mrf headers?
Comment 21 Thiago Sousa Santos 2017-07-08 21:56:24 UTC
(In reply to Gregoire from comment #20)
> First, I appreciate your efforts.
> 
> Please correct me if I'm wrong: from what I have understood, the problem is
> more during the encoding stage where the mdat is longer than the mrf headers
> or said differently, the mrf headers are shorter than the mdat. Perhaps
> there is an issue to write the mrf headers to file.

Yes, it looks to be on the encoding side.

> 
> What I'm confused about is that the mdat file is written at the beginning of
> the file, so how can the system know how long is going to be the mdat or the
> mrf headers?

It doesn't know, but for every sample that it puts in the mdat it writes about this sample on the mrf (size, offset, timing information, stream to which it belongs). The same information is kept in memory to write the final headers in case the file is successfully finished. The mrf is an extra step that is done, if enabled.

> 
> Anyway, the investigation is more on the encoding stage rather than during
> the pure recovery.
> 
> Could you point in the code where I should hack/patch the recovery phase so
> that the mdat is trunk-ed to the size of the mrf headers?

The mdat is written by the pipeline (filesink), so mp4mux writes to it by pushing buffers downstream. The mrf is written by the "atoms_recov_write_trak_samples" function. Look in qtmux code for "moov_recov_file". Try to track the data added to the mrf and correlate with the buffers pushed downstream. You might find that some extra data is being pushed or some data is not being sent to the mrf.

Good luck!
Comment 22 Gregoire 2017-07-08 23:59:05 UTC
That' Good luck!' sounds like 'I don't want to hear about you ever ;-)' hahaha

Thanks for the clarification. It helps. I understand but I think there are still things are not completely correct (independently of my situation):

- can you please commit your latest patch to the tree, that would simplify the testing. This patch definitely helps.

- I have done again a clean git master cerbero + the patch in this bug report, I still get a mild error:

git clone git://anongit.freedesktop.org/gstreamer/cerbero
./cerbero-uninstalled bootstrap
./cerbero-uninstalled package gstreamer-1.0
./cerbero-uninstalled buildone gst-plugins-good-1.0 (with patch)
./cerbero-uninstalled shell
gst-launch-1.0 videotestsrc ! x264enc ! qtmux moov-recovery-file=test.mrf ! filesink location=test.mov
CONTROL-C
gst-launch-1.0 qtmoovrecover recovery-input=test.mrf broken-input=test.mov fixed-output=testo.mov
mp4info testo.mov 

As you can see, it still reports something incorrect.

mp4info version -r
testo.mov:
ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom
Track	Type	Info
1	video	H264 Unknown Profile f4@1.3, 68.533 secs, 1500 kbps, 320x240 @ 30.000146 fps
ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom

- My question in my previous post was more to hack/fix the problem in the recovery stage rather than in the encoding stage. In the atomsrecovery.c file, where is the logic that (could/would/should) compare the size of the mdat in the broken file and the size of the headers in the mrf file?
Comment 23 Thiago Sousa Santos 2017-07-09 05:13:45 UTC
(In reply to Gregoire from comment #22)
> 
> - can you please commit your latest patch to the tree, that would simplify
> the testing. This patch definitely helps.

Yeah, will merge it soon.

> 
> - I have done again a clean git master cerbero + the patch in this bug
> report, I still get a mild error:
> 
> git clone git://anongit.freedesktop.org/gstreamer/cerbero
> ./cerbero-uninstalled bootstrap
> ./cerbero-uninstalled package gstreamer-1.0
> ./cerbero-uninstalled buildone gst-plugins-good-1.0 (with patch)
> ./cerbero-uninstalled shell
> gst-launch-1.0 videotestsrc ! x264enc ! qtmux moov-recovery-file=test.mrf !
> filesink location=test.mov
> CONTROL-C
> gst-launch-1.0 qtmoovrecover recovery-input=test.mrf broken-input=test.mov
> fixed-output=testo.mov
> mp4info testo.mov 
> 
> As you can see, it still reports something incorrect.
> 
> mp4info version -r
> testo.mov:
> ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom
> Track	Type	Info
> 1	video	H264 Unknown Profile f4@1.3, 68.533 secs, 1500 kbps, 320x240 @
> 30.000146 fps
> ReadChildAtoms: "testo.mov": In avc1 atom, extra 4 bytes at end of atom

Those extra 4 bytes are actually intentional AFAIK. Some players out there expect that extra stuff. It is harmless.

> 
> - My question in my previous post was more to hack/fix the problem in the
> recovery stage rather than in the encoding stage. In the atomsrecovery.c
> file, where is the logic that (could/would/should) compare the size of the
> mdat in the broken file and the size of the headers in the mrf file?

At the end of moov_recov_write_file, in atomsrecovery.c there is a piece of code for reading the mdat atom and writing it to the output. It just reads chunks from 1 file and writes it to the other file. Look for FOURCC_mdat and that should be the place. It writes the size of the mdat, then the mdat identifier and then reads the data from the file into the output. It doesn't check for the size of the mdat it intends to write and adds the rest of the file at the end. This will appear as bogus data to the players.
Comment 24 Thiago Sousa Santos 2017-07-09 05:15:01 UTC
Created attachment 355202 [details] [review]
atomsrecovery: read from mdat only what is on headers

Something like this should work. I didn't test this much, yet. Let me know if it works for you. Should do some more testing on the week.
Comment 25 Gregoire 2017-07-09 08:44:17 UTC
Just a very quick test: yes, it works. The file is clean from the mp4info point of view with the example provided above. I need to test more next week and understand the implications. It would still be better to find the problem during the encoding phase.

Anyway, definitely many thanks for this second patch!
Comment 26 Gregoire 2017-07-22 17:04:01 UTC
So, the two patches are working well for me.

I still think that the second patch would make sense to be committed because the point of this plugin is to recover and if for any reason the sizes don't match, it still needs to be recovered properly.

That being said, I'm trying to investigate the original problem of size mismatch. I have noticed one thing: if there is audio, the bitrate of the audio portion is 0kbps.

videoonly-repaired.mp4:
Track	Type	Info
1	video	H264 Baseline@4.2, 3.213 secs, 9978 kbps, 1920x1440 @ 31.123561 fps

audio_video-repaired.mp4:
Track	Type	Info
1	video	H264 Baseline@4.2, 2.890 secs, 9740 kbps, 1920x1440 @ 30.103806 fps
2	audio	MPEG-1 Audio (11172-3), 3.108 secs, 0 kbps, 44100 Hz


Please download the files here: http://gentil.com/tmp/qtmoovrecover3.zip

My point is that the size mismatch problem is perhaps due to audio. Can you test with audio too?
Comment 27 Thiago Sousa Santos 2017-10-25 06:05:52 UTC
commit 923b83a48cce999abd10f869dd721880aee19b0f
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Sat Jul 8 22:11:49 2017 -0700

    atomsrecovery: read from mdat only what is on headers
    
    It is possible that the mdat has more data than what was stored in the
    headers file. If we put that to the output the file will have bogus data
    at the end and some players will complain.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=784258

commit 69605b6c61bde5b6b1bfdefaf7ca66802b0c5158
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Wed Jul 5 22:23:21 2017 -0700

    isomp4: atomsrecovery: handle common and large atom headers
    
    Do not assume all files are large files. Check and use the short or
    extended atom size field only if needed.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=784258