[FFmpeg-devel] first experience with ffmpeg

[Stefano Sabatini]

On date Friday 2008-02-15 10:27:28 -0500, D. Hugh Redelmeier encoded:
> | From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> 
> Thanks for your reply.
> 
> | On date Friday 2008-02-15 02:09:19 -0500, D. Hugh Redelmeier encoded:
> 
> | > ===> the ffmpeg program should diagnose malformed arguments to -v.
> 
> | This happens everytime we use atoi() in ffmpeg.c. When the string is
> | unparsable as a string ffmpeg uses the value 0 without to warn the
> | user a blink.
> |  
> | What about the attached patch (I think there could be some trouble
> | from the conversion from long int to int)?
> | 
> | Suggested log: replace atoi() in ffmpeg.c with the parse_int_or_die()
> | function.
> 
> I think that this is a very good start.
> 
> Have you tested it?  Your function parse_int_or_die seems to have a
> bug.  The result of strtol ought to be stored in i but it is instead
> discarded.

Doh!! that's right, and also I forgot to switch on my brain that day :S.
 
> The function knows that an integer numeral is desired, but it does not
> know what the numeral is for.  So it's diagnostic cannot explain the
> context to the user.  I recommend a const char * argument just to
> provide this context.
> 
>   fprintf(stderr, "expected integer for %s but found: %s\n", ctxt, instr);

This is a good idea, nonteheless I think this should be done in a more
general way. That is we should be able to do it passing the name
of the option directly in cmdutils.c:parse_options(), that means that the
parse_arg_function should take as an argument the name of the option
which is going to be setted.

Since this is a quite radical change for the moment I'd rather leave
this for the Great FFmpeg.c Rewrite.

> I would recommend checking for overflow/underflow tool.  This may mean
> that you need a separate function for parsing unsigned values.
> 
> Here's a modified version of your function.  Untested:
> 
> #include <errno.h>
> 
> static long int parse_int_or_die(const char* intstr, const char *ctxt)
> {
>     long int i;
>     char *tail;
> 
>     errno = 0;
>     i = strtol(intstr, &tail, 10);
>     if (*tail != '\0') {
>         fprintf(stderr, "Expected integer for %s but found \"%s\"\n", ctxt, instr);
>         exit(1);
>     } else if (errno == ERANGE) {
>         fprintf(stderr, "Integer for %s is too big: %s\n", ctxt, intstr);
>         exit(1);
>     }
>     return i;
> }

Good catch, I also think that is better to have a function which
returns an *int* which is what is expected from the functions which
actually use atoi(). Eventually we could add other functions for every
different type (e.g. parse_long_int_or_die, parse_uint_or_die ...).

Also I think it's better to implement such a function in cmdutils.c
rather than in ffmpeg.c, in this way this function would be usable by
ffplay.c and ffserver.c too.

I'm going to create a new thread for that patch.

[...]

Regards. 
-- 
Stefano Sabatini
Linux user number 337176 (see http://counter.li.org)