[FFmpeg-devel] [PATCH] ffmpeg: accept "chapters" as forced key frames.

[Stefano Sabatini]

On date Sunday 2013-01-13 22:49:38 +0100, Nicolas George encoded:
> Allow to force a key frame at the beginning of each chapter.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/ffmpeg.texi |    9 ++++++++-
>  ffmpeg.c        |   52 +++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 53 insertions(+), 8 deletions(-)
> 
> 
> Updated on top of current Git HEAD and removed the risk of overflow in the
> compare function. I will push this version in a few days if nobody has
> anything to add.
> 
> 
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index 0f862c1..de2e795 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -555,9 +555,16 @@ Deprecated see -bsf
>  @item -force_key_frames[:@var{stream_specifier}] @var{time}[, at var{time}...] (@emph{output,per-stream})
>  Force key frames at the specified timestamps, more precisely at the first
>  frames after each specified time.
> +If one of the time is "@code{chapter}[@var{delta}]", it is expanded into the

times

@code{chapters}?

> +time of the beginning of all chapters in the file, shifted by @var{delta}.
>  This option can be useful to ensure that a seek point is present at a
>  chapter mark or any other designated place in the output file.
> -The timestamps must be specified in ascending order.
> +

> +For example, to insert a key frame at 5 minutes plus 0.1 second before the
> +beginning of each chapter:
> + at example
> +-force_key_frames 0:05:00,chapters-0.1

the description is ambiguous, it reads like a key frame is forced "5
minutes + 0.1" before the beginning of each chapter.

> + at end example
>  
>  @item -copyinkf[:@var{stream_specifier}] (@emph{output,per-stream})
>  When doing stream copy, copy also non-key frames found at the
> diff --git a/ffmpeg.c b/ffmpeg.c
> index acaa523..a499f3d 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -1931,19 +1931,25 @@ static InputStream *get_input_stream(OutputStream *ost)
>      return NULL;
>  }
>  
> +static int compare_int64(const void *a, const void *b)
> +{
> +    int64_t va = *(int64_t *)a, vb = *(int64_t *)b;
> +    return va < vb ? -1 : va > vb ? +1 : 0;
> +}
> +
>  static void parse_forced_key_frames(char *kf, OutputStream *ost,
>                                      AVCodecContext *avctx)
>  {
>      char *p;
> -    int n = 1, i;
> -    int64_t t;
> +    int n = 1, i, size, index = 0;
> +    int64_t t, *pts;
>  
>      for (p = kf; *p; p++)
>          if (*p == ',')
>              n++;
> -    ost->forced_kf_count = n;
> -    ost->forced_kf_pts   = av_malloc(sizeof(*ost->forced_kf_pts) * n);
> -    if (!ost->forced_kf_pts) {
> +    size = n;
> +    pts = av_malloc(sizeof(*pts) * size);
> +    if (!pts) {
>          av_log(NULL, AV_LOG_FATAL, "Could not allocate forced key frames array.\n");
>          exit(1);
>      }
> @@ -1955,11 +1961,43 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
>          if (next)
>              *next++ = 0;
>  
> -        t = parse_time_or_die("force_key_frames", p, 1);
> -        ost->forced_kf_pts[i] = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
> +        if (!memcmp(p, "chapters", 8)) {
> +

nit+++: weird empty line, here and below

> +            AVFormatContext *avf = output_files[ost->file_index]->ctx;
> +            int j;
> +
> +            if (avf->nb_chapters > INT_MAX - size ||
> +                !(pts = av_realloc_f(pts, size += avf->nb_chapters - 1,
> +                                     sizeof(*pts)))) {
> +                av_log(NULL, AV_LOG_FATAL,
> +                       "Could not allocate forced key frames array.\n");
> +                exit(1);
> +            }
> +            t = p[8] ? parse_time_or_die("force_key_frames", p + 8, 1) : 0;
> +            t = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
> +
> +            for (j = 0; j < avf->nb_chapters; j++) {
> +                AVChapter *c = avf->chapters[j];
> +                av_assert1(index < size);
> +                pts[index++] = av_rescale_q(c->start, c->time_base,
> +                                            avctx->time_base) + t;
> +            }
> +
> +        } else {
> +
> +            t = parse_time_or_die("force_key_frames", p, 1);
> +            av_assert1(index < size);
> +            pts[index++] = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
> +
> +        }
>  
>          p = next;
>      }
> +
> +    av_assert0(index == size);
> +    qsort(pts, size, sizeof(*pts), compare_int64);
> +    ost->forced_kf_count = size;
> +    ost->forced_kf_pts   = pts;

Alternatively, considering my other patch: we read the chapter times,
and we store the next chapter time in a variable, so the equivalent
syntax for "chapters-0.1" would be:

expr:gte(t, NEXT_CHAPTER_T-0.1)

I wonder how we could combinate both specifications.

A possibility would be to allow to specify more than one
expression/time sequence:
-force_key_frames TIMES -force_key_frames expr:EXPR

or again we could make -force_key_frames accept one expression and one
list, and discard the corresponding previous specification.
-- 
FFmpeg = Fanciful and Frenzy Most Perennial Exciting Governor