[FFmpeg-devel] cinepakenc: When rd_frame() doesn't use MC, mark pkt as keyframe and reset curframe

tor 2019-08-15 klockan 14:20 -0300 skrev James Almer:
> On 8/15/2019 2:13 PM, Tomas Härdin wrote:
> > tor 2019-08-15 klockan 12:54 -0300 skrev James Almer:
> > > On 8/15/2019 11:34 AM, Tomas Härdin wrote:
> > > > 
> > > > Aside: what is -keyint_min for exactly? It seems to be used as a max in
> > > > msvideo1enc.c, but libx264.c and libxavs.c uses it and gop_size as
> > > > minimum and maximum respectively.. Feels like the relevant encoders
> > > > need to be brought into line (assuming it doesn't break user scripts)
> > > 
> > > Afaik, keyint_min is minimum interval/distance between keyframes. So
> > > yes, if it's being used as max distance then it's wrong, as that's what
> > > gop_size is for.
> > 
> > Make sense. But why is gop_size < keyint_min by default then? 12 vs 25.
> 
> I have no idea.
> 
> > I think the x264 people have complained multiple times about the
> > default gop_size setting even
> 
> The libx264 wrapper changes both to -1 in libavcodec, which means x264's
> own defaults will be used instead. So if they complained then i guess it
> was changed accordingly.

Didn't know encoders could have their own defaults. Updated patch
attached. I added some sanity checks on -keyint_min and -g as well.

I couldn't find rl's contact information, hopefully they are subscribed
to this list.

/Tomas

On 8/15/2019 3:55 PM, Tomas Härdin wrote:
> tor 2019-08-15 klockan 14:20 -0300 skrev James Almer:
>> On 8/15/2019 2:13 PM, Tomas Härdin wrote:
>>> tor 2019-08-15 klockan 12:54 -0300 skrev James Almer:
>>>> On 8/15/2019 11:34 AM, Tomas Härdin wrote:
>>>>> Aside: what is -keyint_min for exactly? It seems to be used as a max in
>>>>> msvideo1enc.c, but libx264.c and libxavs.c uses it and gop_size as
>>>>> minimum and maximum respectively.. Feels like the relevant encoders
>>>>> need to be brought into line (assuming it doesn't break user scripts)
>>>> Afaik, keyint_min is minimum interval/distance between keyframes. So
>>>> yes, if it's being used as max distance then it's wrong, as that's what
>>>> gop_size is for.
>>> Make sense. But why is gop_size < keyint_min by default then? 12 vs 25.
>> I have no idea.
>>
>>> I think the x264 people have complained multiple times about the
>>> default gop_size setting even
>> The libx264 wrapper changes both to -1 in libavcodec, which means x264's
>> own defaults will be used instead. So if they complained then i guess it
>> was changed accordingly.
> Didn't know encoders could have their own defaults. Updated patch
> attached. I added some sanity checks on -keyint_min and -g as well.
> 
> I couldn't find rl's contact information, hopefully they are subscribed
> to this list.
> 
> /Tomas
> 
> 
> 0001-cinepakenc-When-rd_frame-doesn-t-use-MC-mark-pkt-as-.patch
> 
> From 49d7ac02d35acbf3e30555eef215ae3a15c06684 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
> Date: Thu, 15 Aug 2019 16:20:23 +0200
> Subject: [PATCH] cinepakenc: When rd_frame() doesn't use MC, mark pkt as
>  keyframe and reset curframe
> 
> This allows the encoder to make smarter choices for future frames,
> while still keeping keyframe distances within [keyint_min, gop_size].
> Defaults and checks prevent nonsensical keyint_min / gop_size combinations.
> ---
>  libavcodec/cinepakenc.c | 54 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c
> index 93917fafe8..396d3e5273 100644
> --- a/libavcodec/cinepakenc.c
> +++ b/libavcodec/cinepakenc.c
> @@ -112,7 +112,7 @@ typedef struct CinepakEncContext {
>      enum AVPixelFormat pix_fmt;
>      int w, h;
>      int frame_buf_size;
> -    int curframe, keyint;
> +    int curframe, keyint_min, keyint_max;
>      AVLFG randctx;
>      uint64_t lambda;
>      int *codebook_input;
> @@ -168,6 +168,16 @@ static av_cold int cinepak_encode_init(AVCodecContext *avctx)
>          return AVERROR(EINVAL);
>      }
>  
> +    if (avctx->keyint_min <= 0) {
> +        av_log(avctx, AV_LOG_ERROR, "-keyint_min must be >= 1\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (avctx->keyint_min > avctx->gop_size) {
> +        av_log(avctx, AV_LOG_ERROR, "-keyint_min > -g is not allowed. Did you mean -g?\n");

Don't use option names in error messages, and especially not with CLI
formatting. Use keyint_min and gop_size instead, which are the names of
the AVCodecContext fields.

And the suggestion at the end is IMO not really needed.

> +        return AVERROR(EINVAL);
> +    }
> +
>      if (!(s->last_frame = av_frame_alloc()))
>          return AVERROR(ENOMEM);
>      if (!(s->best_frame = av_frame_alloc()))
> @@ -213,7 +223,8 @@ static av_cold int cinepak_encode_init(AVCodecContext *avctx)
>      s->h              = avctx->height;
>      s->frame_buf_size = frame_buf_size;
>      s->curframe       = 0;
> -    s->keyint         = avctx->keyint_min;
> +    s->keyint_min     = avctx->keyint_min;
> +    s->keyint_max     = avctx->gop_size;
>      s->pix_fmt        = avctx->pix_fmt;
>  
>      // set up AVFrames
> @@ -871,7 +882,7 @@ static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
>                      uint8_t *last_data[4], int last_linesize[4],
>                      uint8_t *data[4], int linesize[4],
>                      uint8_t *scratch_data[4], int scratch_linesize[4],
> -                    unsigned char *buf, int64_t *best_score)
> +                    unsigned char *buf, int64_t *best_score, CinepakMode *best_mode)
>  {
>      int64_t score = 0;
>      int best_size = 0;
> @@ -970,6 +981,7 @@ static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
>  
>                  if (best_size == 0 || score < *best_score) {
>                      *best_score = score;
> +                    *best_mode = mode;
>                      best_size = encode_mode(s, h,
>                                              scratch_data, scratch_linesize,
>                                              last_data, last_linesize, &info,
> @@ -1000,13 +1012,15 @@ static int write_cvid_header(CinepakEncContext *s, unsigned char *buf,
>  }
>  
>  static int rd_frame(CinepakEncContext *s, const AVFrame *frame,
> -                    int isakeyframe, unsigned char *buf, int buf_size)
> +                    int isakeyframe, unsigned char *buf, int buf_size,
> +                    int *has_mc_out)
>  {
>      int num_strips, strip, i, y, nexty, size, temp_size, best_size;
>      uint8_t *last_data    [4], *data    [4], *scratch_data    [4];
>      int      last_linesize[4],  linesize[4],  scratch_linesize[4];
>      int64_t best_score = 0, score, score_temp;
>      int best_nstrips;
> +    int has_mc;
>  
>      if (s->pix_fmt == AV_PIX_FMT_RGB24) {
>          int x;
> @@ -1067,9 +1081,11 @@ static int rd_frame(CinepakEncContext *s, const AVFrame *frame,
>      for (num_strips = s->min_strips; num_strips <= s->max_strips && num_strips <= s->h / MB_SIZE; num_strips++) {
>          score = 0;
>          size  = 0;
> +        has_mc = 0;
>  
>          for (y = 0, strip = 1; y < s->h; strip++, y = nexty) {
>              int strip_height;
> +            CinepakMode mode_temp;
>  
>              nexty = strip * s->h / num_strips; // <= s->h
>              // make nexty the next multiple of 4 if not already there
> @@ -1101,16 +1117,21 @@ static int rd_frame(CinepakEncContext *s, const AVFrame *frame,
>                                        last_data, last_linesize, data, linesize,
>                                        scratch_data, scratch_linesize,
>                                        s->frame_buf + size + CVID_HEADER_SIZE,
> -                                      &score_temp)) < 0)
> +                                      &score_temp, &mode_temp)) < 0)
>                  return temp_size;
>  
>              score += score_temp;
>              size += temp_size;
> +
> +            if (mode_temp == MODE_MC) {
> +                has_mc = 1;
> +            }
>          }
>  
>          if (best_score == 0 || score < best_score) {
>              best_score = score;
> -            best_size = size + write_cvid_header(s, s->frame_buf, num_strips, size, isakeyframe);
> +            *has_mc_out = has_mc;
> +            best_size = size + write_cvid_header(s, s->frame_buf, num_strips, size, !has_mc);
>  
>              FFSWAP(AVFrame *, s->best_frame, s->scratch_frame);
>              memcpy(buf, s->frame_buf, best_size);
> @@ -1152,21 +1173,25 @@ static int cinepak_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>                                  const AVFrame *frame, int *got_packet)
>  {
>      CinepakEncContext *s = avctx->priv_data;
> -    int ret;
> +    int ret, has_mc;
>  
>      s->lambda = frame->quality ? frame->quality - 1 : 2 * FF_LAMBDA_SCALE;
>  
>      if ((ret = ff_alloc_packet2(avctx, pkt, s->frame_buf_size, 0)) < 0)
>          return ret;
> -    ret       = rd_frame(s, frame, (s->curframe == 0), pkt->data, s->frame_buf_size);
> +    ret       = rd_frame(s, frame, (s->curframe == 0), pkt->data, s->frame_buf_size, &has_mc);
>      pkt->size = ret;
> -    if (s->curframe == 0)
> +    if (s->curframe == 0 || (!has_mc && s->curframe + 1 >= s->keyint_min)) {
> +        //if the packet doesn't use MODE_MC then we
> +        //mark it as a keyframe and reset curframe
> +        s->curframe = 0;
>          pkt->flags |= AV_PKT_FLAG_KEY;
> +    }
>      *got_packet = 1;
>  
>      FFSWAP(AVFrame *, s->last_frame, s->best_frame);
>  
> -    if (++s->curframe >= s->keyint)
> +    if (++s->curframe >= s->keyint_max)
>          s->curframe = 0;
>  
>      return 0;
> @@ -1194,6 +1219,14 @@ static av_cold int cinepak_encode_end(AVCodecContext *avctx)
>      return 0;
>  }
>  
> +static const AVCodecDefault cinepak_defaults[] = {
> +    //-keyint_min 1 means any frame can be a keyframe,
> +    //2 means there's at least one non-keyframe inbetween, and so on
> +    { "keyint_min",       "1" },
> +    { "g",                "25" },
> +    { NULL },
> +};
> +
>  AVCodec ff_cinepak_encoder = {
>      .name           = "cinepak",
>      .long_name      = NULL_IF_CONFIG_SMALL("Cinepak"),
> @@ -1205,4 +1238,5 @@ AVCodec ff_cinepak_encoder = {
>      .close          = cinepak_encode_end,
>      .pix_fmts       = (const enum AVPixelFormat[]) { AV_PIX_FMT_RGB24, AV_PIX_FMT_GRAY8, AV_PIX_FMT_NONE },
>      .priv_class     = &cinepak_class,
> +    .defaults       = cinepak_defaults,
>  };
> -- 2.20.1
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>