[FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

[James Almer]

On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote:
> AVCodecParameters.extradata is supposed to be allocated with
> av_malloc(); av_realloc() and its wrappers do not guarantee
> the proper alignment. Therefore parse the extradata twice:
> Once to check its validity and to determine the eventual size
> and a second time to actually write the new extradata.

Why would av_malloc alignment be needed for extradata?
I see the doxy says "Must be allocated with av_malloc()" but I'm fairly 
sure that was meant to be "Must be allocated with av_malloc() family of 
functions", like its AVCodecContext counterpart. The idea is that 
library users don't use normal malloc as extradata will be freed with 
av_free(), and not because it will be accessed by SIMD code.

> 
> (Of course, not reallocating the buffer is beneficial in itself.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>   libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++----------------
>   1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
> index a695cba370..f5424e95b8 100644
> --- a/libavcodec/bsf/hevc_mp4toannexb.c
> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
> @@ -38,13 +38,11 @@ typedef struct HEVCBSFContext {
>   } HEVCBSFContext;
>   
>   static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
> -                                             uint8_t **new_extradatap,
> +                                             uint8_t *new_extradata,
>                                                size_t *new_extradata_sizep)
>   {
>       int num_arrays = bytestream2_get_byte(gb);
> -    uint8_t *new_extradata = NULL;
>       size_t new_extradata_size = 0;
> -    int ret;
>   
>       for (int i = 0; i < num_arrays; i++) {
>           int type = bytestream2_get_byte(gb) & 0x3f;
> @@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
>                 type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
>               av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
>                      type);
> -            ret = AVERROR_INVALIDDATA;
> -            goto fail;
> +            return AVERROR_INVALIDDATA;
>           }
>   
>           for (int j = 0; j < cnt; j++) {
> @@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
>               if (!nalu_len ||
>                   nalu_len > bytestream2_get_bytes_left(gb) ||
>                   4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
> -                ret = AVERROR_INVALIDDATA;
> -                goto fail;
> +                return AVERROR_INVALIDDATA;
>               }
> -            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE);
> -            if (ret < 0)
> -                goto fail;
> -
> -            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> -            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
> +            if (new_extradata) {
> +                AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> +                bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len);
> +            } else
> +                bytestream2_skipu(gb, nalu_len);
>               new_extradata_size += 4 + nalu_len;
> -            memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>           }
>       }
> -    *new_extradatap = new_extradata;
>       *new_extradata_sizep = new_extradata_size;
>   
>       return 0;
> -fail:
> -    av_freep(&new_extradata);
> -    return ret;
>   }
>   
>   static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> @@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>       bytestream2_skip(&gb, 21);
>       length_size = (bytestream2_get_byte(&gb) & 3) + 1;
>   
> -    ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
> -                                            &new_extradata_size);
> -    if (ret < 0)
> -        return ret;
> +    while (1) {
> +        GetByteContext gb_bak = gb;
> +        ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
> +                                                &new_extradata_size);
> +        if (ret < 0)
> +            return ret;
> +        if (new_extradata || !new_extradata_size)
> +            break;
> +        new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!new_extradata)
> +            return AVERROR(ENOMEM);
> +        memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +        gb = gb_bak;
> +    }
>   
>       av_freep(&ctx->par_out->extradata);
>       ctx->par_out->extradata      = new_extradata;