> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Thursday, January 03, 2019 6:07 AM > To: FFmpeg development discussions and patches <ffmpeg- > de...@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 <jamr...@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.giov...@gmail.com> > > > > wrote: > > > > > > > >> > > > >> > > > >> On Fri, Dec 28, 2018 at 3:17 AM Guo, Yejun <yejun....@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel