[FFmpeg-devel] [PATCH] fix possible crash if vpre before vcodec

[Michael Niedermayer]

On Fri, Feb 27, 2009 at 02:39:56PM +0000, Robert Swain wrote:
> 2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
> > On Fri, Feb 27, 2009 at 02:54:58PM +0100, Reimar D?ffinger wrote:
> >> On Fri, Feb 27, 2009 at 01:33:02PM +0000, Robert Swain wrote:
> >> > 2009/2/27 Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> >> > > On Fri, Feb 27, 2009 at 12:36:27PM +0000, Robert Swain wrote:
> >> > >> 2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
> >> > >> > On Fri, Feb 27, 2009 at 11:25:26AM +0100, Reimar D?ffinger wrote:
> >> > >> >> I recently tried the following command
> >> > >> >> ffmpeg -i baldursgate-logo.mve -vpre hq -vcodec libx264 -b 500k -an out.mp4
> >> > >> >> and wondered why it would not work.
> >> > >> >> strace showed the following:
> >> > >> >> open("/home/reimar/.ffmpeg/hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> > >> >> open("/home/reimar/.ffmpeg/(null)-hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> > >> >> open("/usr/local/share/ffmpeg/hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> > >> >> open("/usr/local/share/ffmpeg/(null)-hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> > >> >> obviously ffmpeg tries to open the file already during option parsing (this could probably be considered
> >> > >> >> the real bug). When -vcodec is set only afterwards it passes NULL to
> >> > >> >> snprintf %s argument which may (and will) crash with some
> >> > >> >> implementations.
> >> > >> >> I propose a patch that at least avoids the crash, but e.g. this:
> >> > >> >> -vcodec blub -vpre hq -vcodec libx264
> >> > >> >> will still have the counter-intuitive behaviour of reading
> >> > >> >> blub-hq.ffpreset but encoding with libx264.
> >> > >> >
> >> > >> > would it not make more sense to warn the user that the codec is not set
> >> > >> > before -*pre instead of silently skiping searching for the file?
> >> > >>
> >> > >> I agree. I prefer command lines to error out on options that don't
> >> > >> work as intended than to continue on. But a warning at least is
> >> > >> absolutely necessary.
> >> > >>
> >> > >> Also, it should be documented that the preset options should come
> >> > >> after the codec name.
> >> > >
> >> > > Well, the warning still would not work right, as the example
> >> > > -vcodec blub -vpre hq -vcodec libx264
> >> > > above shows, that's why I didn't consider it particularly useful.
> >> >
> >> > Maybe the non-explicit codec idea wasn't so good after all. Maybe we
> >> > should suggest people use -vpre libx264-hq?
> >>
> >> I think it was a good idea, just the implementation is not optimal.
> >> I don't know FFmpeg command line parsing well, but in MPlayer I'd just
> >> store the -?pre string somewhere and open the file after parsing all the
> >> other options...
> >
> > can be done, add a static char, store it there
> > then call the parsing function from new_video_stream() and others
> > but beware of chains of bitstream filters ...
> >
> > [i hope ive not missed some detail ...]
> 
> Would that work for multiple output files?

it should but i might have missed something ...


> 
> ffmpeg -i blah ... -vpre hq -vcodec libx264 ... oufile1.mp4 -vpre blah
> -vcodec foo ... outfile2.avi
> 
> What would be the behaviour if -vpre isn't specified for the second
> output file?

it likely will be reset in new_video_stream()


> For the matter, what is the behaviour for other options
> in this case? I never knew.

i suspect some will be kept some will be reset ;)

patches to improve this and document it are welcome

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090227/fa384d09/attachment.pgp>