Message ID | 1632964440-24060-2-git-send-email-lance.lmwang@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/5] avfilter/vf_showinfo: minor adjustment for the dump format of ROI | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
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, ®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. 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, ®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-request@ffmpeg.org with subject "unsubscribe".
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);