[FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of interest sidedata

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of mypopy at gmail.com
> Sent: Monday, March 11, 2019 11:05 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Jun Zhao <barryjzhao at tencent.com>
> Subject: Re: [FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of
> interest sidedata
> 
> On Mon, Mar 11, 2019 at 10:51 AM Guo, Yejun <yejun.guo at intel.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> > > Of Jun Zhao
> > > Sent: Sunday, March 10, 2019 9:17 PM
> > > To: ffmpeg-devel at ffmpeg.org
> > > Cc: Jun Zhao <barryjzhao at tencent.com>
> > > Subject: [FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of
> > > interest sidedata
> > >
> > > From: Jun Zhao <barryjzhao at tencent.com>
> > >
> > > support regions of interest sidedata
> > >
> > > Signed-off-by: Jun Zhao <barryjzhao at tencent.com>
> > > ---
> > >  libavfilter/vf_showinfo.c |   33
> +++++++++++++++++++++++++++++++++
> > >  1 files changed, 33 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> > > index e41c330..d0e140f 100644
> > > --- a/libavfilter/vf_showinfo.c
> > > +++ b/libavfilter/vf_showinfo.c
> > > @@ -111,6 +111,36 @@ static void dump_stereo3d(AVFilterContext *ctx,
> > > AVFrameSideData *sd)
> > >          av_log(ctx, AV_LOG_INFO, " (inverted)");
> > >  }
> > >
> > > +static void dump_roi(AVFilterContext *ctx, AVFrameSideData *sd)
> > > +{
> > > +    AVRegionOfInterest *roi;
> > > +    int nb_rois;
> > > +
> > > +    if (sd->size < sizeof(*roi)) {
> > > +        av_log(ctx, AV_LOG_INFO, "invalid data");
> > > +        return;
> > > +    }
> >
> > it is possible.
> It's a surprised behavior for me, how to check the corrupted side data
> for this case? I don't like the self_size field in this case in fact.

it is for ABI compatible, just imaging that we expand the struct, while the
user is using an old version and there is only one roi in the sd buffer. 
For this case, sd->size is less than sizeof(*roi).

> >
> > > +
> > > +    roi = (AVRegionOfInterest *)sd->data;
> > > +    if (roi->self_size == 0 || sd->size % roi->self_size != 0) {
> > > +        av_log(ctx, AV_LOG_INFO, "invalid ROI side data");
> > > +        return;
> > > +    }
> > > +    nb_rois = sd->size / roi->self_size;
> > > +
> > > +    av_log(ctx, AV_LOG_INFO, "Regions Of Interest(RoI) information: ");
> > > +    for (int index = 0; index < nb_rois; index++) {
> > > +        av_log(ctx, AV_LOG_INFO, "index: %d, region: (%d %d)/(%d %d),
> qp
> > > offset: %d/%d",
> > > +               index, roi->left, roi->top, roi->right, roi->bottom, roi-
> >qoffset.num,
> > > roi->qoffset.den);
> > > +
> > > +        if (roi->self_size == 0)
> > > +            av_log(ctx, AV_LOG_INFO,
> > > +                   "AVRegionOfInterest.self_size must be set to
> > > sizeof(AVRegionOfInterest).\n");
> >
> > we can skip this since the first roi->self_size is tested.
> >
> If we have N ROIs, we just need to check the first roi entry for
> roi->self_size? It's the other surprised behavior for me

forgot to mention the next line together.

let's save the first roi->self_size and so the next line will be:
roi = (AVRegionOfInterest *)((char *)roi + self_size);

The self_size here is to indicates the struct size when the user uses it.
It is actually enough to just set the first roi.


btw, there is an issue in my original code pushed, and Mark Thompson has given
better code which has not been pushed yet, except saving the first roi->self_size.
(I just realized this, and will comment MarkT again).

> > > +
> > > +        roi = (AVRegionOfInterest *)((char *)roi + roi->self_size);
> > > +    }
> > > +}
> > > +
> > >  static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
> > >  {
> > >      const char *color_range_str     = av_color_range_name(frame-
> > > >color_range);
> > > @@ -246,6 +276,9 @@ static int filter_frame(AVFilterLink *inlink,
> AVFrame
> > > *frame)
> > >          case AV_FRAME_DATA_AFD:
> > >              av_log(ctx, AV_LOG_INFO, "afd: value of %"PRIu8, sd->data[0]);
> > >              break;
> > > +        case AV_FRAME_DATA_REGIONS_OF_INTEREST:
> > > +            dump_roi(ctx, sd);
> > > +            break;
> > >          default:
> > >              av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
> > > bytes)",
> > >                     sd->type, sd->size);
> > > --
> > > 1.7.1
> > >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel