[FFmpeg-devel] [PATCH] ffmpeg: implement -force_key_frames_expr option

Michael Niedermayer michaelni at gmx.at
Fri Dec 14 03:15:22 CET 2012


On Fri, Dec 14, 2012 at 12:56:42AM +0100, Stefano Sabatini wrote:
> On date Thursday 2012-12-13 16:53:57 +0100, Nicolas George encoded:
> > Le tridi 23 frimaire, an CCXXI, Stefano Sabatini a écrit :
> > > ---
> > >  doc/ffmpeg.texi |   20 ++++++++++++++++++++
> > >  ffmpeg.c        |   44 ++++++++++++++++++++++++++++++++++++++++++--
> > >  ffmpeg.h        |   15 +++++++++++++++
> > >  ffmpeg_opt.c    |    7 +++++++
> > >  4 files changed, 84 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> > > index b8f6930..ff14e04 100644
> > > --- a/doc/ffmpeg.texi
> > > +++ b/doc/ffmpeg.texi
> > > @@ -551,12 +551,32 @@ Force video tag/fourcc. This is an alias for @code{-tag:v}.
> > >  Show QP histogram
> > >  @item -vbsf @var{bitstream_filter}
> > >  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.
> > >  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.
> > > +See also the @option{force_key_frames_expr} option.
> > > +
> > > + at item -force_key_frames_expr[:@var{stream_specifier}] @var{expr} (@emph{output,per-stream})
> > > +Force key frames at the time specified by the expression in
> > 
> > ... at the times specified by the expression. The expression will be
> > evaluated repeatedly to get the time for each forced keyframe.
> > 
> > > + at var{expr}, more precisely at the first frames after each specified
> > > +time. The expression can contain the following constants:
> > > +
> > > + at table @option
> > > + at item n
> > > +The count of the next match, starting from 0.
> > 
> > Number of already forced keyframes.
> 
> Changed.
> 
> > > + at item prev_t
> > > +The time of the last forced key frame, it is @code{NAN} when no
> >                                         ^
> > (value of the last evaluation of the expression)
> 
> No, this is not the same thing.
> 
> > 
> > > +keyframe was still forced.
> > 
> > ... was forced yet.
> > 
> > > + at end table
> > > +
> > > +For example you can specify the expression @code{n*5} to force a key
> > > +frame every 5 seconds, or @code{prev_t+5} to specify a key frame after
> > > +the last one forced.
> > > +See also the @option{force_key_frames} option.
> > 
> > Please add a note that forcing too many keyframes is very harmful for the
> > lookahead algorithms (at least for x264): using fixed-GOP options or similar
> > would be more efficient.
> 
> Done.
>  
> > >  @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 956f5b6..7985051 100644
> > > --- a/ffmpeg.c
> > > +++ b/ffmpeg.c
> > > @@ -109,6 +109,13 @@ const int program_birth_year = 2000;
> > >  
> > >  static FILE *vstats_file;
> > >  
> > > +const char *const forced_keyframes_const_names[] = {
> > > +    "n",
> > > +    "prev_t",
> > 
> > > +    "next_t",
> > 
> > Undocumented?
> 
> Changed to a context variable.
> 
> > 
> > > +    NULL
> > > +};
> > > +
> > >  static void do_video_stats(OutputStream *ost, int frame_size);
> > >  static int64_t getutime(void);
> > >  
> > > @@ -435,6 +442,8 @@ static void exit_program(void)
> > >          avcodec_free_frame(&output_streams[i]->filtered_frame);
> > >  
> > >          av_freep(&output_streams[i]->forced_keyframes);
> > > +        av_freep(&output_streams[i]->forced_keyframes_expr);
> > > +        av_expr_free(output_streams[i]->forced_keyframes_pexpr);
> > >          av_freep(&output_streams[i]->avfilter);
> > >          av_freep(&output_streams[i]->logfile_prefix);
> > >          av_freep(&output_streams[i]);
> > > @@ -872,8 +881,9 @@ static void do_video_out(AVFormatContext *s,
> > >          video_size += pkt.size;
> > >          write_frame(s, &pkt, ost);
> > >      } else {
> > > -        int got_packet;
> > > +        int got_packet, forced_keyframe = 0;
> > >          AVFrame big_picture;
> > > +        double pts_time;
> > >  
> > >          big_picture = *in_picture;
> > >          /* better than nothing: use input picture interlaced
> > > @@ -897,11 +907,26 @@ static void do_video_out(AVFormatContext *s,
> > >          big_picture.quality = ost->st->codec->global_quality;
> > >          if (!enc->me_threshold)
> > >              big_picture.pict_type = 0;
> > > +
> > > +        pts_time = big_picture.pts != AV_NOPTS_VALUE ?
> > > +            big_picture.pts * av_q2d(enc->time_base) : NAN;
> > 
> > Indentation is unusual.
> 
> It is what emacs likes (and please let's not fight over silly
> indentation rules).
> 
> > 
> > >          if (ost->forced_kf_index < ost->forced_kf_count &&
> > >              big_picture.pts >= ost->forced_kf_pts[ost->forced_kf_index]) {
> > > -            big_picture.pict_type = AV_PICTURE_TYPE_I;
> > >              ost->forced_kf_index++;
> > > +            forced_keyframe = 1;
> > > +        } else if (pts_time != AV_NOPTS_VALUE &&
> > > +                   pts_time >= ost->forced_keyframes_expr_const_values[FKF_NEXT_T]) {
> > > +            ost->forced_keyframes_expr_const_values[FKF_N] += 1;
> > > +            ost->forced_keyframes_expr_const_values[FKF_PREV_T] = pts_time;
> > > +            ost->forced_keyframes_expr_const_values[FKF_NEXT_T] =
> > > +                av_expr_eval(ost->forced_keyframes_pexpr, ost->forced_keyframes_expr_const_values, NULL);
> > > +            forced_keyframe = 1;
> > > +        }
> > > +        if (forced_keyframe) {
> > > +            big_picture.pict_type = AV_PICTURE_TYPE_I;
> > > +            av_log(NULL, AV_LOG_DEBUG, "Forced keyframe at time %f\n", pts_time);
> > >          }
> > > +
> > >          update_benchmark(NULL);
> > >          ret = avcodec_encode_video2(enc, &pkt, &big_picture, &got_packet);
> > >          update_benchmark("encode_video %d.%d", ost->file_index, ost->index);
> > > @@ -2226,9 +2251,24 @@ static int transcode_init(void)
> > >                      codec->bits_per_raw_sample = frame_bits_per_raw_sample;
> > >                  }
> > >  
> > > +                if (ost->forced_keyframes && ost->forced_keyframes_expr) {
> > > +                    av_log(NULL, AV_LOG_ERROR,
> > > +                           "forced_keyframes and forced_keyframes_expr are incompatible, select just one\n");
> > > +                    return AVERROR(EINVAL);
> > > +                }
> > >                  if (ost->forced_keyframes)
> > >                      parse_forced_key_frames(ost->forced_keyframes, ost,
> > >                                              ost->st->codec);
> > > +                if (ost->forced_keyframes_expr) {
> > > +                    ret = av_expr_parse(&ost->forced_keyframes_pexpr, ost->forced_keyframes_expr,
> > > +                                        forced_keyframes_const_names, NULL, NULL, NULL, NULL, 0, NULL);
> > > +                    if (ret < 0)
> > > +                        return ret;
> > > +                    ost->forced_keyframes_expr_const_values[FKF_N] = 0;
> > > +                    ost->forced_keyframes_expr_const_values[FKF_PREV_T] = NAN;
> > > +                    ost->forced_keyframes_expr_const_values[FKF_NEXT_T] =
> > > +                        av_expr_eval(ost->forced_keyframes_pexpr, ost->forced_keyframes_expr_const_values, NULL);
> > > +                }
> > 
> 
> > Possible alternate implementation: -force_key_frames expr:prev_t+5.
> > 
> > It has the advantage of not requiring all the code for new options, you just
> > have to add a test in parse_forced_key_frames(); that would make the code
> > much simpler I believe. And you do not have to worry about the user giving
> > incompatible options.
> 
> Changed this way, it is slightly simpler. I don't know if it would
> make sense to specify both expression and list (in this case it would
> also make sense to split the options), probably not.
> 
> [...]
> 
> Patch updated.
> -- 
> FFmpeg = Friendly and Fucking Martial Powered Efficient Guru

>  doc/ffmpeg.texi |   38 ++++++++++++++++++++++++++++++++------
>  ffmpeg.c        |   45 ++++++++++++++++++++++++++++++++++++++++-----
>  ffmpeg.h        |   12 ++++++++++++
>  3 files changed, 84 insertions(+), 11 deletions(-)
> fd3db7ff91b99d41766f00dc9d65b4117801db31  0003-ffmpeg-implement-force_key_frames-expression-evaluti.patch
> From 36786e76ccad1a89037542d4596a04f373215c81 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Sun, 9 Dec 2012 18:40:22 +0100
> Subject: [PATCH] ffmpeg: implement -force_key_frames expression evalution
> 
> ---
>  doc/ffmpeg.texi |   38 ++++++++++++++++++++++++++++++++------
>  ffmpeg.c        |   45 ++++++++++++++++++++++++++++++++++++++++-----
>  ffmpeg.h        |   12 ++++++++++++
>  3 files changed, 84 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index b8f6930..0915025 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -551,12 +551,38 @@ Force video tag/fourcc. This is an alias for @code{-tag:v}.
>  Show QP histogram
>  @item -vbsf @var{bitstream_filter}
>  Deprecated see -bsf
> - at 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.
> -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.
> +
> + at item -force_key_frames[:@var{stream_specifier}] (@var{time}[, at var{time}...])|expr:@var{expr} (@emph{output,per-stream})
> +Force key frames at the specified timestamps, more precisely at the
> +first frames after each specified time.
> +
> +If a list of times is specified as argument, the 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.
> +
> +If the argument is prefixed with @code{expr:}, the string @var{expr}
> +is interpreted like an expression and is evaluated repeatedly to get
> +the time of the next keyframe to force.
> +
> +The expression in @var{expr} can contain the following constants:
> + at table @option
> + at item n
> +the number of already forced keyframes
> + at item prev_t
> +the time of the last forced key frame, it is @code{NAN} when no
> +keyframe was forced yet
> + at end table

implementing this in ffmpeg.c makes the expression evaluation only
available to ffmpeg and not to other user applications, also the
expression evaluator only has a small number of input variables
available from the application.
in libavcodec it would have a much larger amount of information
available, like the scene change score or various rate control
parameters

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121214/01b48432/attachment.asc>


More information about the ffmpeg-devel mailing list