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

Submitted by Jun Zhao on March 10, 2019, 1:16 p.m.

Details

Message ID 1552223806-32587-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao March 10, 2019, 1:16 p.m.
From: Jun Zhao <barryjzhao@tencent.com>

support regions of interest sidedata

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavfilter/vf_showinfo.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

Comments

Guo, Yejun March 11, 2019, 2:48 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Jun Zhao

> Sent: Sunday, March 10, 2019 9:17 PM

> To: ffmpeg-devel@ffmpeg.org

> Cc: Jun Zhao <barryjzhao@tencent.com>

> Subject: [FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of

> interest sidedata

> 

> From: Jun Zhao <barryjzhao@tencent.com>

> 

> support regions of interest sidedata

> 

> Signed-off-by: Jun Zhao <barryjzhao@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.

> +

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

> +

> +        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@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
mypopy@gmail.com March 11, 2019, 3:04 a.m.
On Mon, Mar 11, 2019 at 10:51 AM Guo, Yejun <yejun.guo@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> > Of Jun Zhao
> > Sent: Sunday, March 10, 2019 9:17 PM
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Jun Zhao <barryjzhao@tencent.com>
> > Subject: [FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of
> > interest sidedata
> >
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > support regions of interest sidedata
> >
> > Signed-off-by: Jun Zhao <barryjzhao@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.
>
> > +
> > +    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
> > +
> > +        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
> >
Guo, Yejun March 11, 2019, 3:20 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of mypopy@gmail.com

> Sent: Monday, March 11, 2019 11:05 AM

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Cc: Jun Zhao <barryjzhao@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@intel.com>

> wrote:

> >

> >

> >

> > > -----Original Message-----

> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > > Of Jun Zhao

> > > Sent: Sunday, March 10, 2019 9:17 PM

> > > To: ffmpeg-devel@ffmpeg.org

> > > Cc: Jun Zhao <barryjzhao@tencent.com>

> > > Subject: [FFmpeg-devel] [PATCH V2] lavfi/showinfo: support regions of

> > > interest sidedata

> > >

> > > From: Jun Zhao <barryjzhao@tencent.com>

> > >

> > > support regions of interest sidedata

> > >

> > > Signed-off-by: Jun Zhao <barryjzhao@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@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

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;
+    }
+
+    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");
+
+        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);