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...
this is not mandatory but trying with some basic fuzzer seems like a good idea look at the examples in the manpage of zzuf for example, its very easy to use [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
