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

From: Limin Wang <lance.lmwang@gmail.com>

It is simpler and more efficient compared to the current code.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_addroi.c | 98 +++++++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 61 deletions(-)

lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> It is simpler and more efficient compared to the current code.
> 
> Signed-off-by: Limin Wang <lance.lmwang@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

On Thu, Sep 30, 2021 at 04:58:07AM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > It is simpler and more efficient compared to the current code.
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@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.
It's very confusing for the self_size. I prefer to add a descriptor header
with (nb_roi, noi_offset, noi_size), then store roi elements after the
header, then it's not necessary to store self_size for every roi.
But it'll break with old version compatiablity. 

> b) The new code meanwhile is happy to create arrays of
> AVRegionOfInterest with different sizes.
I haven't consider the size is different if it'll be changed. But the old
code will not work if the size is changed I think.

> 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).

It make sense, please ignore the patch, I think it's difficult to make
different version happy. 

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