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

[Vittorio Giovara]

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;
}
-- 
Vittorio