[FFmpeg-devel] [PATCH 3/3] Make opt_default(), return an error code rather than exit from the program.

[Michael Niedermayer]

On Mon, Nov 08, 2010 at 01:22:43PM +0100, Stefano Sabatini wrote:
> On date Sunday 2010-11-07 17:35:43 +0100, Stefano Sabatini encoded:
> > On date Sunday 2010-11-07 17:13:27 +0100, Michael Niedermayer encoded:
> > > On Sun, Nov 07, 2010 at 04:32:45PM +0100, Stefano Sabatini wrote:
> > > > On date Sunday 2010-11-07 16:18:10 +0100, Michael Niedermayer encoded:
> > > > > On Sun, Nov 07, 2010 at 10:42:12AM +0100, Stefano Sabatini wrote:
> > > > > > On date Sunday 2010-11-07 01:04:49 +0100, Michael Niedermayer encoded:
> > > > > > > On Sun, Nov 07, 2010 at 12:53:35AM +0100, Stefano Sabatini wrote:
> > > > > > > > On date Sunday 2010-11-07 00:38:48 +0100, Michael Niedermayer encoded:
> > > > > > > > > On Sat, Nov 06, 2010 at 03:02:57PM +0100, Stefano Sabatini wrote:
> > > > > > > > > > This allows the program to nicely handle the error, for example
> > > > > > > > > > providing detailed error explanation to the user or cleaning up the
> > > > > > > > > > allocated resources of the program.
> > > > > > > > > 
> > > > > > > > > there is plenty of code that does not check its return value
> > > > > > > > > and i dont think adding checks everywhere is a good idea
> > > > > > > > 
> > > > > > > > In general returning meaningful values *is* a good idea, but I'm
> > > > > > > > not going to add them to all the FFmpeg codebase, but *this patch* is
> > > > > > > > required by the previous patches in this series, to make ffmpeg prints
> > > > > > > > something useful after opt_default() is called, rather than just
> > > > > > > > abort.
> > > > > > > 
> > > > > > > grep for opt_default and you will see there are alot of cases where the
> > > > > > > return value is not checked and not exiting anymore will lead to actual
> > > > > > > bugs
> > > > > > 
> > > > > > All but one of those uses are in opt_target() where the value is fixed
> > > > > 
> > > > > until someone changes it ...
> > > > > anyway, i dont strongly care about the return vs exit but i strongly object
> > > > > to adding 20 checks in all uses of opt_default()
> > > > 
> > > > opt_default_or_die(opt, arg)?
> > > 
> > > pass the line number and filename as argument to opt_default if you want it
> > > printed before exit()
> > 
> > opt_default() doesn't know nothing about the external context where it
> > is called, so it makes sense to handle the error in the callee
> > function (and it is not possible to change the signature of
> > opt_default).
> 
> Anyway I hope you agree with the fact that the current feedback sucks.

i agree with that of course


> 
> Without the patches:
> $ ffmpeg -i slow.flv -fpre ffpresets/libx264-wrong.ffpreset -y x.mp4
> [...]
> Unrecognized option 'foo'
> 
> (opt_default() exits, without giving the application a chance to say
> something useful about the context of the failure)
> 
> Patched:
> $ ffmpeg -i slow.flv -fpre ffpresets/libx264-wrong.ffpreset -y x.mp4
> [...]
> Unrecognized option 'foo'
> ffpresets/libx264-wrong.ffpreset:3: Invalid option or argument: 'foo=bar', parsed as 'foo' = 'bar'
> 
> I believe the second output is *much* more useful (especially
> considering that with the -*pre options it isn't even immediate which
> is the file from which the options are read).
> 

> And opt_target() should be removed in favor of soft-presets.

no objection here if its not worse in useability.
but before this is done this changeset is ugly

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

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101108/690993bf/attachment.pgp>