[FFmpeg-devel] [PATCH 2/5] avfilter/vf_addroi: realloc the buf and append new ROI

Andreas Rheinhardt andreas.rheinhardt at outlook.com
Thu Sep 30 05:58:07 EEST 2021


lance.lmwang at gmail.com:
> From: Limin Wang <lance.lmwang at gmail.com>
> 
> It is simpler and more efficient compared to the current code.
> 
> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> ---
>  libavfilter/vf_addroi.c | 98 +++++++++++++++++++------------------------------
>  1 file changed, 37 insertions(+), 61 deletions(-)
> 
> diff --git a/libavfilter/vf_addroi.c b/libavfilter/vf_addroi.c
> index 5f9ec21..f521a96 100644
> --- a/libavfilter/vf_addroi.c
> +++ b/libavfilter/vf_addroi.c
> @@ -91,14 +91,40 @@ static int addroi_config_input(AVFilterLink *inlink)
>      return 0;
>  }
>  
> +static int addroi_append_roi(AVBufferRef **pbuf, AVRegionOfInterest *region)
> +{
> +    AVBufferRef *buf = *pbuf;
> +    uint32_t old_size = buf ? buf->size : 0;
> +    int ret;
> +    AVRegionOfInterest *roi;
> +
> +    ret = av_buffer_realloc(pbuf, old_size + sizeof(*region));
> +    if (ret < 0)
> +        return ret;
> +    buf = *pbuf;
> +
> +    roi = (AVRegionOfInterest *)(buf->data + old_size);
> +    memcpy(roi, region, sizeof(*region));
> +
> +    return 0;
> +}
> +
>  static int addroi_filter_frame(AVFilterLink *inlink, AVFrame *frame)
>  {
>      AVFilterContext *avctx = inlink->dst;
>      AVFilterLink  *outlink = avctx->outputs[0];
>      AddROIContext     *ctx = avctx->priv;
> -    AVRegionOfInterest *roi;
> +    AVRegionOfInterest region = (AVRegionOfInterest) {
> +        .self_size = sizeof(AVRegionOfInterest),
> +        .top       = ctx->region[Y],
> +        .bottom    = ctx->region[Y] + ctx->region[H],
> +        .left      = ctx->region[X],
> +        .right     = ctx->region[X] + ctx->region[W],
> +        .qoffset   = ctx->qoffset,
> +    };
>      AVFrameSideData *sd;
>      int err;
> +    AVBufferRef *buf = NULL;
>  
>      if (ctx->clear) {
>          av_frame_remove_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
> @@ -107,73 +133,23 @@ static int addroi_filter_frame(AVFilterLink *inlink, AVFrame *frame)
>          sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
>      }
>      if (sd) {
> -        const AVRegionOfInterest *old_roi;
> -        uint32_t old_roi_size;
> -        AVBufferRef *roi_ref;
> -        int nb_roi, i;
> -
> -        old_roi = (const AVRegionOfInterest*)sd->data;
> -        old_roi_size = old_roi->self_size;
> -        av_assert0(old_roi_size && sd->size % old_roi_size == 0);
> -        nb_roi = sd->size / old_roi_size + 1;
> -
> -        roi_ref = av_buffer_alloc(sizeof(*roi) * nb_roi);
> -        if (!roi_ref) {
> -            err = AVERROR(ENOMEM);
> +        buf = sd->buf;
> +        err = addroi_append_roi(&buf, &region);
> +        if (err < 0)
>              goto fail;
> -        }
> -        roi = (AVRegionOfInterest*)roi_ref->data;
> -
> -        for (i = 0; i < nb_roi - 1; i++) {
> -            old_roi = (const AVRegionOfInterest*)
> -                (sd->data + old_roi_size * i);
> -
> -            roi[i] = (AVRegionOfInterest) {
> -                .self_size = sizeof(*roi),
> -                .top       = old_roi->top,
> -                .bottom    = old_roi->bottom,
> -                .left      = old_roi->left,
> -                .right     = old_roi->right,
> -                .qoffset   = old_roi->qoffset,
> -            };
> -        }
> -
> -        roi[nb_roi - 1] = (AVRegionOfInterest) {
> -            .self_size = sizeof(*roi),
> -            .top       = ctx->region[Y],
> -            .bottom    = ctx->region[Y] + ctx->region[H],
> -            .left      = ctx->region[X],
> -            .right     = ctx->region[X] + ctx->region[W],
> -            .qoffset   = ctx->qoffset,
> -        };
> -
> -        av_frame_remove_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
> -
> -        sd = av_frame_new_side_data_from_buf(frame,
> -                                             AV_FRAME_DATA_REGIONS_OF_INTEREST,
> -                                             roi_ref);
> -        if (!sd) {
> -            av_buffer_unref(&roi_ref);
> -            err = AVERROR(ENOMEM);
> -            goto fail;
> -        }
>  
> +        sd->data = buf->data;
> +        sd->size = buf->size;
>      } else {
> -        sd = av_frame_new_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST,
> -                                    sizeof(AVRegionOfInterest));
> +        err = addroi_append_roi(&buf, &region);
> +        if (err < 0)
> +            goto fail;
> +        sd = av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST, buf);
>          if (!sd) {
> +            av_buffer_unref(&buf);
>              err = AVERROR(ENOMEM);
>              goto fail;
>          }
> -        roi = (AVRegionOfInterest*)sd->data;
> -        *roi = (AVRegionOfInterest) {
> -            .self_size = sizeof(*roi),
> -            .top       = ctx->region[Y],
> -            .bottom    = ctx->region[Y] + ctx->region[H],
> -            .left      = ctx->region[X],
> -            .right     = ctx->region[X] + ctx->region[W],
> -            .qoffset   = ctx->qoffset,
> -        };
>      }
>  
>      return ff_filter_frame(outlink, frame);
> 

1. a) The old code is based upon the assumption that the self_size of
all the AVRegionOfInterest elements is the same. I don't see this
requirement documented anywhere.
b) The new code meanwhile is happy to create arrays of
AVRegionOfInterest with different sizes.
2. The new code has alignment issues in case the required alignment of
AVRegionOfInterest increases if the already existing side data has been
added by old software with the old alignment.
3. a) Imagine AVRegionOfInterest changes from a POD to a structure with
allocated subelements. The old code would properly free and discard
them; the new code wouldn't: In case the underlying buffer is not marked
as reallocatable, the free function of the old buffer would be called,
but the (then dangling) pointers would nevertheless be retained. That
the behaviour here depends upon internals of the AVBuffer API is a red
flag for me.
b) If libavfilter itself is so new that it knows that AVRegionOfInterest
to no longer be a POD and if this filter gets an option to set these
subelements, then the new approach here does no longer work at all any
more: One would have to wrap the old free function into the new free
function (i.e. override the AVBuffer's free function and make the new
free function call the old free function) to free the old entries (whose
version might actually be newer than what libavfilter knows about, hence
the need to use the old free function for freeing the old entries). This
is just not possible with the AVBuffer API. (The AVBuffer's free
function currently does not even get the size of the buffer, so one
would have to abuse the opaque to pass the number of elements to it.)

My conclusions are: Keep the code as-is, but require that all
AVRegionOfInterest entries in an array must always be from the same
version. Notice that one can't really traverse a list in which this is
not so because of alignment: If one stores the elements contiguously,
there will be issues if they require different alignment; if one honours
alignment and adds padding in between these elements, then one can't
traverse the list at all any more, because one does not know where to
look for self_size of the next entry at all any more (one would need to
know that entry's padding in order to know where to look for it).

- Andreas


More information about the ffmpeg-devel mailing list