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

[James Almer]

On 1/2/2019 2:18 PM, Vittorio Giovara wrote:
> On Wed, Jan 2, 2019 at 4:13 PM Vittorio Giovara <vittorio.giovara at gmail.com>
> wrote:
> 
>>
>>
>> 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.
>>
>> 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;

This will go south as soon as you start copying, referencing and freeing
frames and/or frame side data.

All side data types need to be a contiguous array of bytes in memory
with no pointers.

> }
>