[MPlayer-dev-eng] [PATCH] -force-key-frames option

[Reimar Döffinger]

On Sun, Oct 17, 2010 at 03:52:12PM +0200, Nicolas George wrote:
> Le sextidi 26 vendémiaire, an CCXIX, Reimar Döffinger a écrit :
> > I don't really like that a simple "string in, value out" becomes
> > a founction with 3 arguments and one return value.
> > Maybe there really is no truly better way, but I don't
> > really like it, taht is all.
> 
> The problem is that the "value out" is a float, and float is not a
> sympathetic type to report additional information, especially errors.
> 
> I think the "-1E100 means error" was ugly and should be avoided, especially
> if the function is to be used in several places.
> 
> The obvious choice would be NAN, but it is a portability risk. Although, now
> that I look closer into it, I see that NAN is used in libav* and mplayer is
> already linked with isnan. But still, I do not like it a lot.

In the context of MPlayer/FFmpeg MP_NOPTS_VALUE probably would be a more obvious
choice.
However, we might want to allow parsing this as a special value, thus I did
not suggest it.

> > Also I'd find more explicit error case helpful, e.g.:
> 
> I am not sure what you are suggesting. Always store a value in the return
> argument, even in case of error?

That, but also an explicit return.
It just seems bad for readability to have the initialization to 0 for len and
then rely on sscanf not changing it in the error case.
I think it's e.g. a bit obfuscated what you'd have to do to make -1 the error
return value.
Or if you wanted to add a feature where an empty string would translate to time
0 (or above-mentioned MP_NOPTS_VALUE) but still have an error for incorrect termintation.
I made a mistake in the example code I gave though.

> -  int a, b;
> +  int a, b, len = 0;
>    double d;
> -  if (sscanf(str, "%d:%d:%lf", &a, &b, &d) == 3)
> -    return 3600*a + 60*b + d;
> -  else if (sscanf(str, "%d:%lf", &a, &d) == 2)
> -    return 60*a + d;
> -  else if (sscanf(str, "%lf", &d) == 1)
> -    return d;
> -  return -1e100;
> +  if (sscanf(str, "%d:%d:%lf%n", &a, &b, &d, &len) >= 3)
> +    *time = 3600*a + 60*b + d;
> +  else if (sscanf(str, "%d:%lf%n", &a, &d, &len) >= 2)
> +    *time = 60*a + d;
> +  else if (sscanf(str, "%lf%n", &d, &len) >= 1)
> +    *time = d;
> +  else
> +    *time = 0; /* dummy value */
> +  if (str[len] && str[len] != endchar)
> +    len = 0;
> +  return len;

So that would make my overall suggestion:
1) do not initialize len
2) initialize "*time = 0; /* ensure initialization for error cases */"
3) do
else if (sscanf...)
  ...
else
  return 0; /* unsupported time format */
if (str[len] && str[len] != endchar)
  return 0; /* incorrect termination */
return len;

> @@ -1268,8 +1272,7 @@ static int parse_time(const m_option_t* opt,const char *name, const char *param,
>    if (param == NULL || strlen(param) == 0)
>      return M_OPT_MISSING_PARAM;
>  
> -  time = parse_timestring(param);
> -  if (time == -1e100) {
> +  if (!parse_timestring(param, &time, 0)) {
>      mp_msg(MSGT_CFGPARSER, MSGL_ERR, "Option %s: invalid time: '%s'\n",
>             name,param);
>      return M_OPT_INVALID;
> @@ -1327,7 +1330,7 @@ static int parse_time_size(const m_option_t* opt,const char *name, const char *p
>  
>    /* End at time parsing. This has to be last because the parsing accepts
>     * even a number followed by garbage */
> -  if ((end_at = parse_timestring(param)) == -1e100) {
> +  if (!parse_timestring(param, &end_at, 0)) {

I assume you checked those variables are actually double.

> +/**
> + * Parse a string as a timestamp.
> + *
> + * @return  Number of chars in the timestamp.
> + */
> +int parse_timestring(const char *str, double *time, char endchar);

At least endchar should have a short description.
Otherwise enough bike-shedding, just go ahead with something.