[FFmpeg-devel] [PATCH] cinepakenc: When rd_frame() doesn't use MC, mark pkt as keyframe and reset curframe
James Almer
jamrial at gmail.com
Fri Aug 16 05:35:45 EEST 2019
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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list