On Mon, Jun 11, 2018 at 12:05:20PM +0000, Eran Kornblau wrote: > > On Sun, Jun 10, 2018 at 01:20:10PM +0000, Eran Kornblau wrote: > > > > > > > > -----Original Message----- > > > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On > > > > Behalf Of Michael Niedermayer > > > > Sent: Saturday, June 9, 2018 9:17 PM > > > > To: FFmpeg development discussions and patches > > > > <ffmpeg-devel@ffmpeg.org> > > > > Subject: Re: [FFmpeg-devel] qt-faststart bug near 4GB > > > > > > > > > + > > > > > + if (atom.size < atom.header_size) { > > > > > > > > > + printf("atom size %"PRIu64" too small\n", atom.size); > > > > > > > > errors should go to stderr if av_log() is not used i see this is not > > > > introduced by the patch but was that way before so its more a comment > > > > about the code in git than the patch ... > > > > but ideally this should be fixed in a seperate patch either before > > > > or after this one > > > > > > I agree, had the same thought when I wrote the patch, but left it as it > > > was - only the usage error is printed to stderr. > > > Will submit a patch for this after we finalize the discussion on this one. > > > > > > > > > > > some self test for the newly added feature would also be a good idea > > > > > > Since a real MP4 with this problem is going to be large (4GB), I'm > > > thinking about taking a small MP4, and patch some stco offset to > > > UINT_MAX. Naturally, it will break the file, but faststart won't care - > > > it should still upgrade the stco to co64, and we can just compare the > > > cksum/md5sum of the result to some fixed value. > > > What do you think? > > > > hmm, thats probably the most practical, yes > > > > you could also simply compress the file a 4gb file thats 99.99% 0 bytes is > > not large but the problem would be that to test this it would need to be > > decompressed and the space requirements seems too problematic for fate > > clients > > > Ok, took the 'fake offset' approach - created the attached mp4 with - > ffmpeg -f lavfi -i anullsrc=sample_rate=48000 -t 1 faststart-4gb-overflow.mov > python > d = file('faststart-4gb-overflow.mov','rb').read() > p = d.find('stco') > d = d[:(p+12)] + '\xff' * 4 + d[(p+16):] > file('faststart-4gb-overflow.mov','wb').write(d) > > I added a fate test for this under 'mov' (that seemed the closest match...). > For the faststart output, I'm using a temp file, I tried to avoid it with - > qt-faststart input.mov >(md5sum -) > But for some reason, this didn't work from 'make fate' (it did work in bash). > Another option to avoid the temp file, is that I'll add support for passing > '-' > as the qt-faststart output file name, and have it write to stdout. > Since all writes there are sequential (no seeks) it should be easy. > Let me know what you prefer... anyway, current patch + sample file are > attached. > > > > > > > > > Btw, the tests we ran on this change internally are - 1. compared the > > > result of the new version to the previous one on more than 1k files > > > (incl small files and large >4gb files) and verified the result was > > > exactly the same (the edge case this solves is extremely rare, and > > > "normal" files are not expected to change) 2. checked the specific file > > > that had this issue, and verified it was fixed. > > > > > > > > > > > > > also, was the new code tested with a fuzzer ? > > > > > > No, can you provide some guidance on this - is there some fuzzing > > > framework that you're using? > > > > hmm, you can probably add qt-faststart to: > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgoogle%2Foss-fuzz%2Ftree%2Fmaster%2Fprojects%2Fffmpeg&data=02%7C01%7Ceran.kornblau%40kaltura.com%7C439d2ebbc07940dc5cda08d5cf345a9b%7C0c503748de3f4e2597e26819d53a42b6%7C0%7C0%7C636642746144001154&sdata=8N9HpfHJ6atTGmtwHmr0Vuccw39W7RzMM%2BLw%2F4Ptj0g%3D&reserved=0 > > > > this is probably a bit effort though. > > > > using some arbitrary choosen fuzzer, AFL, zzuf or anything else is probably > > simpler. adding it to oss-fuzz has the advantage that google will in the > > future automatically do the fuzzing for qt-faststart in ffmpeg. > > to add it to oss-fuzz you probably would have to submit changes both to the > > oss-fuzz ffmpeg repo as well as to ffmpeg ... > > > Is this mandatory? :) it may be because I'm not familiar with these > frameworks, but indeed sounds like > a significant effort to do it... > > > thx > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > It is what and why we do it that matters, not just one of them. > > > > Thanks! > > Eran
> Makefile | 4 +++- > fate/mov.mak | 9 ++++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > a5668223451eb818958fa6943af0a50bf5c11a70 > 0001-qt-faststart-add-fate-test-for-stco-overflow.patch > From 2ae7cf2839f9bc36cc762da581d16e3eb3adaaec Mon Sep 17 00:00:00 2001 > From: erankor <eran.kornb...@kaltura.com> > Date: Mon, 11 Jun 2018 14:45:11 +0300 > Subject: [PATCH] qt-faststart: add fate test for stco overflow > > verify that the stco atom is upgraded to co64 when the addition of moov > size to the offsets results in an overflow > --- > tests/Makefile | 4 +++- > tests/fate/mov.mak | 9 ++++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) fails on mingw64 + wine make fate-mov-faststart-4gb-overflow V=2 --- - 2018-06-12 23:23:10.487549933 +0200 +++ tests/data/fate/mov-faststart-4gb-overflow 2018-06-12 23:23:10.479161196 +0200 @@ -1 +0,0 @@ -bc875921f151871e787c4b4023269b29 Test mov-faststart-4gb-overflow failed. Look at tests/data/fate/mov-faststart-4gb-overflow.err for details. run-detectors: unable to find an interpreter for tools/qt-faststart.exe md5sum: faststart-4gb-overflow-output.mov: No such file or directory rm: cannot remove 'faststart-4gb-overflow-output.mov': No such file or directory make: *** [fate-mov-faststart-4gb-overflow] Error 1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
