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

Stefano Sabatini stefasab at gmail.com
Fri Dec 14 00:56:42 CET 2012


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-ffmpeg-implement-force_key_frames-expression-evaluti.patch
Type: text/x-diff
Size: 7604 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121214/e1821ec6/attachment.bin>


More information about the ffmpeg-devel mailing list