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 > > 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://github.com/google/oss-fuzz/tree/master/projects/ffmpeg 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 ... thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
