On Thu, Sep 30, 2021 at 04:58:07AM +0200, Andreas Rheinhardt wrote: > lance.lmw...@gmail.com: > > From: Limin Wang <lance.lmw...@gmail.com> > > > > It is simpler and more efficient compared to the current code. > > > > Signed-off-by: Limin Wang <lance.lmw...@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". -- Thanks, Limin Wang _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".