Re: [FFmpeg-devel] [PATCH] avcodec/dnxhdenc: retry increasing qscale to not overflow max_bits

From: "Martin Storsjö" <martin@martin.st>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avcodec/dnxhdenc: retry increasing qscale to not overflow max_bits
Date: Sat, 5 Mar 2022 21:18:59 +0200 (EET)
Message-ID: <9e7bfc61-9758-5dca-dd26-63be0e994b1@martin.st> (raw)
In-Reply-To: <20220305101544.507071-1-onemda@gmail.com>

On Sat, 5 Mar 2022, Paul B Mahol wrote:

> Increase mb_bits type from uint16_t to uint32_t to fix possible oveflows
> in bit size calculations.
>
> Update fate test that needs change.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
> libavcodec/dnxhdenc.c     | 8 +++++---
> libavcodec/dnxhdenc.h     | 2 +-
> tests/ref/lavf/mxf_opatom | 4 ++--
> 3 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
> index 374fec499b..9bdecec284 100644
> --- a/libavcodec/dnxhdenc.c
> +++ b/libavcodec/dnxhdenc.c
> @@ -924,7 +924,7 @@ static void dnxhd_setup_threads_slices(DNXHDEncContext *ctx)
>             unsigned mb = mb_y * ctx->m.mb_width + mb_x;
>             ctx->slice_size[mb_y] += ctx->mb_bits[mb];
>         }
> -        ctx->slice_size[mb_y]   = (ctx->slice_size[mb_y] + 31) & ~31;
> +        ctx->slice_size[mb_y]   = (ctx->slice_size[mb_y] + 31U) & ~31U;
>         ctx->slice_size[mb_y] >>= 3;
>         thread_size = ctx->slice_size[mb_y];
>         offset += thread_size;
> @@ -1220,17 +1220,19 @@ static int dnxhd_encode_fast(AVCodecContext *avctx, DNXHDEncContext *ctx)
>             avctx->execute2(avctx, dnxhd_mb_var_thread,
>                             NULL, NULL, ctx->m.mb_height);
>         radix_sort(ctx->mb_cmp, ctx->mb_cmp_tmp, ctx->m.mb_num);
> +retry:
>         for (x = 0; x < ctx->m.mb_num && max_bits > ctx->frame_bits; x++) {
>             int mb = ctx->mb_cmp[x].mb;
>             int rc = (ctx->qscale * ctx->m.mb_num ) + mb;
>             max_bits -= ctx->mb_rc[rc].bits -
>                         ctx->mb_rc[rc + ctx->m.mb_num].bits;
> -            ctx->mb_qscale[mb] = ctx->qscale + 1;
> +            if (ctx->mb_qscale[mb] < 255)
> +                ctx->mb_qscale[mb]++;
>             ctx->mb_bits[mb]   = ctx->mb_rc[rc + ctx->m.mb_num].bits;
>         }
>
>         if (max_bits > ctx->frame_bits)
> -            return AVERROR(EINVAL);
> +            goto retry;
>     }
>     return 0;
> }
> diff --git a/libavcodec/dnxhdenc.h b/libavcodec/dnxhdenc.h
> index 30ae8c15e3..7726a3915f 100644
> --- a/libavcodec/dnxhdenc.h
> +++ b/libavcodec/dnxhdenc.h
> @@ -100,7 +100,7 @@ typedef struct DNXHDEncContext {
>     unsigned qscale;
>     unsigned lambda;
>
> -    uint16_t *mb_bits;
> +    uint32_t *mb_bits;
>     uint8_t  *mb_qscale;
>
>     RCCMPEntry *mb_cmp;
> diff --git a/tests/ref/lavf/mxf_opatom b/tests/ref/lavf/mxf_opatom
> index e34cf2559e..359422ce9a 100644
> --- a/tests/ref/lavf/mxf_opatom
> +++ b/tests/ref/lavf/mxf_opatom
> @@ -1,3 +1,3 @@
> -aab6397829bd90f0c77a3f9fde53bb9c *tests/data/lavf/lavf.mxf_opatom
> +b1e32792b835ac51b0b1d5dcb9497e4e *tests/data/lavf/lavf.mxf_opatom
> 4717625 tests/data/lavf/lavf.mxf_opatom
> -tests/data/lavf/lavf.mxf_opatom CRC=0xf55aa22a
> +tests/data/lavf/lavf.mxf_opatom CRC=0xb13ba2f8
> -- 
> 2.33.0

Thanks! I'm not very familiar with the codebase so I can't say for sure, 
but the reasoning sounds sensible, and if it fixes the test, I'm all for 
it, so please go ahead!

// Martin

_______________________________________________
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".