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

[Robert Swain]

2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
> On Fri, Feb 27, 2009 at 03:06:42PM +0000, Robert Swain wrote:
>> 2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
>> > On Fri, Feb 27, 2009 at 03:10:13PM +0100, Michael Niedermayer wrote:
>> >> 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 ...]
>> >
>> > one ive missed ...
>> >
>> > currently -vpre balh -some thing
>> > allows things to be overridden from vpre but if vpre is set later
>> > this would no longer work
>>
>> Well-spotted. This functionality is very useful for applying
>> constraints to quality presets and for overriding things for specific
>> circumstances. Back to the drawing board.
>
> simply tell the user to put vcodec first ...

I think this is probably the best option. We need to avoid the crash
but maybe a warning note here that -*pre should come after -*codec but
before overriding coding options. Non-existent files are handled in
opt_preset() already.

Regards,
Rob