[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 Michael Niedermayer
> Sent: Thursday, January 03, 2019 6:07 AM
> 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 Wed, Jan 02, 2019 at 09:50:24PM +0100, Vittorio Giovara wrote:
> > On Wed, Jan 2, 2019 at 6:45 PM James Almer <jamrial at gmail.com> wrote:
> >
> > > 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:
> > > >>
> > > >
> > > > 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.
> > >
> >
> > Hmm you're right, but embedding an entire array is pretty bad too,
> > especially because it locks the struct size...
> > Do you have any alternative ideas for this?
> 
> the array element size could be added in addition to the number of entries.
> that way elements can be added after the array and also the individual
> elements can be extended
> 
> int rois_nb;
> int roi_size;
> AVRegionOfInterest rois[rois_nb];

nice direction, and I just have a little adjustment.

>From user's perspective, the buffer size in this method is not straightforward when it calls
av_frame_new_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST, size) to prepare for ROI buffer.

the size calculation looks like:
size =  sizeof(rois_nb) + sizeof(roi_size) + rois_nb * sizeof(AVRegionOfInterest).

and, the ROI array starts from:  AVFrameSideData.data + sizeof(rois_nb) + sizeof(roi_size).


My change is to move roi_size into struct AVRegionOfInterest as self_size, 
and remove rois_nb (can be calculated with AVFrameSideData.size / AVRegionOfInterest.self_size).

>From user's perspective, the code looks like:
    size_t nb_rois = 2;
    AVFrameSideData *sd = av_frame_new_side_data(in, AV_FRAME_DATA_REGIONS_OF_INTEREST, nb_rois * sizeof(AVRegionOfInterest));
    if (!sd) {
        av_frame_free(&in);
        return AVERROR(ENOMEM);
    }
    AVRegionOfInterest* rois = (AVRegionOfInterest*)sd->data;
    rois[0].self_size = sizeof(*rois);
    rois[0].top = 0;
    rois[0].left = 0;
    rois[0].bottom = in->height;
    rois[0].right = in->width/4;
    rois[0].qoffset = -15;
    rois[1].self_size = sizeof(*rois);
    rois[1].top = 0;
    rois[1].left = in->width*3/4;
    rois[1].bottom = in->height;
    rois[1].right = in->width;
    rois[1].qoffset = -15;

The advantage is the code is straightforward.
The disadvantage is that we have to always do 'rois[i].self_size = sizeof(*rois);' and there is a little memory waste. 

I personally prefer this method, will send out the new patch.

> 
> Alternativly some "int version" could be used but we dont use that style
> elsewhere
> 
> 
> [...]
> --
> Michael     GnuPG fingerprint:
> 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Republics decline into democracies and democracies degenerate into
> despotisms. -- Aristotle