[FFmpeg-devel] [PATCH] Implement the function cmdutils.c:parse_int_or_die

[D. Hugh Redelmeier]

| From: Stefano Sabatini <stefano.sabatini-lala at poste.it>

| According to the Gnu libc manual, both INFINITY and the macro isfinite()
| are part of C99, and FFmpeg is implemented with C99, so I think it's
| just OK to use them.
|
| On the other end NaN is a Gnu extension, so we can't rely on that,
| while (strangely enough) the isnan() macro seems to be part of ISO
| C99.
| 
| I'm OK to use the isnan() check, for what regards the acceptance of
| infinite values see below.

I don't know what the coding standards are for the ffmpeg project.

Some of this is specified in C99 as part of the optional Annex F for
support of IEC 60559 (formerly known as IEC 559:1989).

You can test whether the implementation includes this optional
support by using the macro __STDC_IEC_559__.

isnan() is specified to exist whether IEC 60559 is supported or not.
I don't remember whether it was in C before this standard.

Note that in 5.2.4.2.2 the standard says:

    An implementation may give zero and non-numeric values (such as
    infinities and NaNs) a sign or may leave them unsigned. Wherever
    such values are unsigned, any requirement in this International
    Standard to retrieve the sign shall produce an unspecified sign,
    and any requirement to set the sign shall be ignored.

So the idea of comparing with -INFINITY seems to not be sanctioned by
the base standard.  It is nailed down in IEC 60559 (but the original
proposal for IEEE 754, and the 8087 FPU, implemented a mode where
there was just one infinity -- the projectively extended real number
system rather than the affinely extended one.).

I think that it is perfectly legal for all of these to be false in a
conforming C implementation:
	5 < INFINITY
	5 > INFINITY
	5 < -INFINITY
	5 > -INFINITY

Notice also that INFINITY is considered a non-numeric value.  That
matches my intuition.  I don't think that infinities can safely
treated as normal values without careful consideration of each bit of
code that might touch them.

| > I have NOT looked at the codebase that these patches are intended for.
| > How frequently will parse_double_or_die be called with interesting
| > bounds (i.e. other than [-INFINITY, INFINITY])?  My hunch is that it
| > would be uncommon (based on no data!).  I know that the range check
| > makes sense for integers but it might not for floats.
| 
| I have no strong opinion on this, but from a purely mathematically
| point of view if we're saying that a double may be e.g. say <= inf
| then we should accept also the inf value, so I think the function
| shouldn't exit if the value is "inf", and we could have a variable
| which for some strange reason accept the inf or -inf value.
| 
| Eventually we could add a flag to tell parse_double_or_die() to accept
| only finite values; another idea could be to have a flag for
| inclusivity/exclusivity in the range check.

I still have not looked at the code base.

Portable code does not use INFINITY.  Not all FPUs support it.  That
is why C99 made support optional.

Does ffmpeg code expect to be able to use INFINITY?

If you don't know the answer, and you are at all familiar with the
code, I would bet that there would be places where the code misbehaves
if given INFINITY.  Here's why: the chance of code accidentally
working with INFINITY is not high.  If you are at all familiar with
the code, and don't know, then there is a high probablility that at
least some of the coders didn't consider infinity.

The problem is that INFINITY is not like a normal floating point
value.  For example, with INFINITY (and with NaNs):
	(x < y)
is not the same as
	!(x >= y)

Summary: life is simpler if you can prevent INFINITY or NaN showing up
in input.  Furthermore, I suspect users don't have any reason to
specify them in input.

The one obvious use is to provide ranges that are unbounded -- you've
used that in your patch.  But I don't actually imagine that any or
very many calls to parse_double_or_die would specify interesting
ranges.  So leave the ranges out.  If I'm wrong, you can add another
function that takes ranges.  This code does not use INFINITY.

double parse_double_or_die(const char *context, const char *numstr)
{
    double d;
    char *tail;

    /* errno could have been set by previous library calls, so reset it */
    errno = 0;
    d = strtod(numstr, &tail);
    if (*tail != '\0') {
        fprintf(stderr, "Expected double for %s but found: %s\n", context, numstr);
        exit(1);
    } else if (errno == ERANGE || isnan(d)) {
        fprintf(stderr, "The value for %s is not a proper double float number: %s\n",
                context, numstr);
        exit(1);
    }
    return d;
}

double parse_bounded_double_or_die(const char *context, const char *numstr, double min, double max)
{
    double d = parse_double_or_die(context,numstr);

    if (d < min | d > max) {
        fprintf(stderr, "The value for %s must be >= %f and <= %f but you used: %s\n",
                context, min, max, numstr);
        exit(1)
    }
    return d;
}