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

[Reimar Döffinger]

On Sun, Oct 10, 2010 at 12:24:01PM +0200, Nicolas George wrote:
> +  if (sscanf(str, "%d:%d:%lf%n", &a, &b, &d, &end) == 3)
> +    *rts = 3600*a + 60*b + d;
> +  else if (sscanf(str, "%d:%lf%n", &a, &d, &end) == 2)
> +    *rts = 60*a + d;
> +  else if (sscanf(str, "%lf%n", &d, &end) == 1)
> +    *rts = d;

According to the documentation it is wrong to assume whether %n increases
the sscanf value or not.
You'd probably have to rewrite this using strtol or strtod.

> +  return end ? (char *)str + end : NULL;

Returning the parsed length would avoid the cast.
Also I'd suggest to consider making this the extra argument,
and if it is 0 also check that the end is the terminating 0
or otherwise return an error.
Currently our parsing code seems to accept e.g. "2 centuries" as 2 seconds.

> +    forced_key_frames_ts = calloc(sizeof(*forced_key_frames_ts), nts);

valgrind won't like that this is never freed.


> +        if (!(p = parse_timestring(p, &ts))) {

I still don't see the point of munging assignments into ifs.
IMO it is really bad for readability.