[FFmpeg-devel] [PATCH 2/5] avfilter/vf_addroi: realloc the buf and append new ROI
lance.lmwang at gmail.com
lance.lmwang at gmail.com
Thu Sep 30 13:38:27 EEST 2021
On Thu, Sep 30, 2021 at 04:58:07AM +0200, Andreas Rheinhardt wrote:
> 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, ®ion);
> > + 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, ®ion);
> > + 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 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".
--
Thanks,
Limin Wang
More information about the ffmpeg-devel
mailing list