On 1/2/2019 2:18 PM, Vittorio Giovara wrote:
> 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;
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.
> }
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
