[FFmpeg-devel] [PATCH V4 1/2] avutil: add ROI data struct and bump version

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Vittorio Giovara
> Sent: Wednesday, January 02, 2019 11:13 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V4 1/2] avutil: add ROI data struct and
> bump version
> 
> On Fri, Dec 28, 2018 at 3:17 AM Guo, Yejun <yejun.guo at intel.com> wrote:
> 
> > The encoders such as libx264 support different QPs offset for
> > different MBs, it makes possible for ROI-based encoding. It makes
> > sense to add support within ffmpeg to generate/accept ROI infos and
> > pass into encoders.
> >
> > Typical usage: After AVFrame is decoded, a ffmpeg filter or user's
> > code generates ROI info for that frame, and the encoder finally does
> > the ROI-based encoding.
> >
> > The ROI info is maintained as side data of AVFrame.
> >
> > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > ---
> >  libavutil/frame.c   |  1 +
> >  libavutil/frame.h   | 23 +++++++++++++++++++++++
> >  libavutil/version.h |  2 +-
> >  3 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c index
> > 34a6210..bebc50e 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -841,6 +841,7 @@ const char *av_frame_side_data_name(enum
> > AVFrameSideDataType type)
> >      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table
> > data";
> >  #endif
> >      case AV_FRAME_DATA_DYNAMIC_HDR_PLUS: return "HDR Dynamic
> Metadata
> > SMPTE2094-40 (HDR10+)";
> > +    case AV_FRAME_DATA_ROIS: return "Regions Of Interest";
> >      }
> >      return NULL;
> >  }
> > diff --git a/libavutil/frame.h b/libavutil/frame.h index
> > 582ac47..3460b01 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -173,6 +173,12 @@ enum AVFrameSideDataType {
> >       * volume transform - application 4 of SMPTE 2094-40:2016 standard.
> >       */
> >      AV_FRAME_DATA_DYNAMIC_HDR_PLUS,
> > +
> > +    /**
> > +     * Regions Of Interest, the data is an array of AVROI type, the
> > + array
> > size
> > +     * is implied by AVFrameSideData::size / sizeof(AVROI).
> > +     */
> > +    AV_FRAME_DATA_ROIS,
> >
> 
> Any chance i could convince you of unfolding this acronym into
> AV_FRAME_REGIONS_OF_INTEREST (and AVRegionOfInterest)?
> When I first read it I thought you were talking about Return of Investments
> or Request of Invention, which were mildly confusing.

thanks, sure, will change to AV_FRAME_DATA_REGIONS_OF_INTEREST and AVRegionOfInterest

> 
> The "AVFrameSideData::size" is a C++-ism, could you please use
> "AVFrameSideData.size" like elsewhere in the header?

ok, will change it.

> 
> You should probably document that sizeof() of this struct is part of the public
> ABI.

thanks for catching this issue, it is not ABI compatible if a new data member is later added into
struct AVRegionOfInterest.  will response in following emails.

> 
> 
> >  };
> >
> >  enum AVActiveFormatDescription {
> > @@ -201,6 +207,23 @@ typedef struct AVFrameSideData {  }
> > AVFrameSideData;
> >
> >  /**
> > + * Structure to hold Region Of Interest.
> > + *
> > + * top/bottom/left/right are coordinates at frame pixel level.
> >
> 
> what does  "pixel level" mean? May I suggest better wording?
> 
>     Number of pixels to discard from the the top/bottom/left/right border of
> the frame to obtain the region of interest of the frame.

thanks., much better than mine, will use it.

> 
> + * They will be extended internally if the codec requires an alignment.
> > + * If the regions overlap, the last value in the list will be used.
> >
> 
> Isn't this encoder dependent too?

yes, and will add to notes explicitly.

> 
> 
> > + *
> > + * qoffset is quant offset, it is encoder dependent.
> 
> + */
> > +typedef struct AVROI {
> > +    size_t top;
> > +    size_t bottom;
> > +    size_t left;
> > +    size_t right;
> > +    int qoffset;
> >
> 
> so int instead of float?

I used float in very early version and got feedback to use int. I'm open to both, I'll change to float since two comments till now ask for float.

> 
> +} AVROI;
> > +
> > +/**
> >   * This structure describes decoded (raw) audio or video data.
> >   *
> >   * AVFrame must be allocated using av_frame_alloc(). Note that this
> > only diff --git a/libavutil/version.h b/libavutil/version.h index
> > f997615..1fcdea9 100644
> > --- a/libavutil/version.h
> > +++ b/libavutil/version.h
> > @@ -79,7 +79,7 @@
> >   */
> >
> --
> Vittorio
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel