On Wed, Jan 2, 2019 at 4:13 PM Vittorio Giovara <vittorio.giov...@gmail.com> wrote:
> > > On Fri, Dec 28, 2018 at 3:17 AM Guo, Yejun <yejun....@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....@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. > > The "AVFrameSideData::size" is a C++-ism, could you please use > "AVFrameSideData.size" like elsewhere in the header? > > You should probably document that sizeof() of this struct is part of the > public ABI. > After thinking some more about this, it's probably a bad idea to embed an array in a side data. Not only it constrains the structure to only change at major bumps, but it seems very reminiscent of binary side data which is/should be not used for newer side data. As a matter of fact side data normally hosts either structs or simple types like ints or enums only. Instead of this, why not creating a structure hosting the number of elements and a pointer? Something like AVRegionOfInterest { size_t top/left/right/bottom } AVRegionOfInterestSet { int rois_nb; AVRegionOfInterest *rois; } -- Vittorio _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel