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

[Reimar Döffinger]

On Sun, Oct 17, 2010 at 07:41:54PM +0200, Nicolas George wrote:
> + * @param[in]  endchar  return an error of the next character after the

[in] is a bit pointless for non-pointer arguments (what else could they be
after all?).

> +Force key frames at the specified timestamps, more precisely at the first
> +frames after each specified time.

"first frame", not "frames"

> +This option can be useful to ensure that a seek point is present at a

"can be used" instead of "useful" I'd suggest.

> diff --git a/cfg-mencoder.h b/cfg-mencoder.h
> index ade5047..bb0f50a 100644
> --- a/cfg-mencoder.h
> +++ b/cfg-mencoder.h
> @@ -220,6 +220,8 @@ const m_option_t mencoder_opts[]={
>      // info header strings
>      {"info", info_conf, CONF_TYPE_SUBCONFIG, CONF_GLOBAL, 0, 0, NULL},
>  
> +    {"force-key-frames", parse_forced_key_frames, CONF_TYPE_FUNC_PARAM, CONF_GLOBAL, 0, 0, NULL},
> +
>  #ifdef CONFIG_MP3LAME
>      {"lameopts", lameopts_conf, CONF_TYPE_SUBCONFIG, CONF_GLOBAL, 0, 0, NULL},
>  #endif
> diff --git a/libmpcodecs/ve.c b/libmpcodecs/ve.c
> index f71f932..194348f 100644
> --- a/libmpcodecs/ve.c
> +++ b/libmpcodecs/ve.c
> @@ -26,6 +26,7 @@
>  #include "img_format.h"
>  #include "mp_image.h"
>  #include "vf.h"
> +#include "ve.h"
>  
>  extern const vf_info_t ve_info_lavc;
>  extern const vf_info_t ve_info_vfw;
> @@ -73,3 +74,47 @@ vf_instance_t* vf_open_encoder(vf_instance_t* next, const char *name, char *args
>      char* vf_args[] = { "_oldargs_", args, NULL };
>      return vf_open_plugin(encoder_list,next,name,vf_args);
>  }
> +
> +static double *forced_key_frames_ts;
> +static int forced_key_frames_number;
> +static int forced_key_frames_idx;
> +
> +int parse_forced_key_frames(const m_option_t *opt, char *arg)
> +{
> +    double ts;
> +    char *p;
> +    int nts = 1, idx = 0, len;
> +
> +    for (p = arg; *p; p++)
> +        nts += *p == ',';
> +    free(forced_key_frames_ts);
> +    forced_key_frames_ts = calloc(sizeof(*forced_key_frames_ts), nts);
> +    p = arg;
> +    while (1) {
> +        len = parse_timestring(p, &ts, ',');
> +        if (!len) {
> +            mp_msg(MSGT_CFGPARSER, MSGL_ERR,
> +                   "Option force-key-frames: invalid time: '%s'\n", p);
> +            return M_OPT_INVALID;
> +        }
> +        forced_key_frames_ts[idx++] = ts;
> +        if (!p[len])
> +            break;
> +        p += len + 1;
> +    }
> +    forced_key_frames_number = idx;
> +    forced_key_frames_idx = 0;
> +    return 0;
> +}

Possibly a bit silly, since you'd have to validate the values separately,
but have you considered using CONF_TYPE_STRING_LIST instead and doing
the parsing inside is_forced_key_frame?
The validating could also include checking the values are actually
in increasing order.

> +int is_forced_key_frame(double pts)
> +{
> +    if (forced_key_frames_idx < forced_key_frames_number &&
> +        pts >= forced_key_frames_ts[forced_key_frames_idx]) {
> +        forced_key_frames_idx++;
> +        if (forced_key_frames_idx >= forced_key_frames_number)
> +            free(forced_key_frames_ts);

Maybe also set forced_key_frames_number to 0 just in case?