[FFmpeg-devel] [PATCH] lavu: add AVVideoHint API

[Stefano Sabatini]

On date Wednesday 2023-07-26 10:52:57 +0000, Carotti, Elias wrote:
> On Mon, 2023-07-24 at 01:27 +0200, Stefano Sabatini wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> > 
> > 
> <snip>
> 
> > > 
> > > sorry to nitpick, but if we have this information in the single
> > > rect
> > > should not we use that instead?
> > > 
> > > Basically I see two options:
> > > 1. generic VideoHint, storing single hint type and rects
> > > 
> > > for example we might have type=changed, and the rects storing only
> > > the
> > > changed rects
> > > 
> > > If we want to extend this then we need to add a new type. The
> > > problem
> > > might be if we want to store more than one VideoHint in the side
> > > data
> > > (it it possible to store more than one AVVideoHint, each one with a
> > > different type, as side data?). Suppose for example we want to add
> > > a
> > > new hint type, e.g. ROI, then we can add a new AVHideoHint with
> > > type=roi - but I don't know if we can have several frame hint side
> > > data (each one with different individual types).
> > > 
> > > 2. generic VideoHint, storing rects each one containing the hint
> > > type
> > > 
> > > for example we might have a list of rects, each one with
> > > type=changed|constant|roi. This would allow one to store different
> > > kind of hints in the same AVVideoHint structure, but at the same
> > > time
> > > would enable inconsistent data (for example we might have changed
> > > and
> > > unchanged rects in the same list of rects)
> > > 
> 

> I don't think it allowed such inconsistency the way it was made.
> Basically, the semantics was that the Hint type could be either
> CONSTANT or CHANGED and that was the basis to interpret the rects with
> type DAMAGE. 

Yes, but then there is no guarantee that you'll have all rects with
the same subtype, and you need to iterate and check each one to
check the subtype (so you might have mixed constant/changed and
unclear behavior to apply in this case).

> > > In each case, storing the type in the generic AVVideHint and in
> > > each
> > > rect might lead to inconsistent states (e.g. you might have generic
> > > type=damage, and have type=roi and type=changed in the rects), in
> > > this
> > > case having the type in both the general structure and in each
> > > rects
> > > seems redundant and leads to possible data inconsistencies.
> > > 
> > > ...
> > > 
> > 
> > > I would rather go to solution 1, but I'm not sure if having more
> > > than
> > > one hint in the side data (with different subtypes) is possible and
> > > wanted. If this is the case, probably the best solution would be to
> > > store more than one AVVideoHint (each one with an individual type
> > > and
> > > list of rects) in the single side data object.
> > 
> > Elaborating on this.
> > 
> > 1. Ideally we might want to extend the av_frame_get_side_data API to
> > be able to store more than one entry per side data, by adding a
> > unique
> > label to the side data, but this requires probably more effort and
> > extends the scope even further.
> > 
> > 2. Alternatively, we might extend the hint API like this:
> > 
> > AVVideoHint *av_video_hint_create_side_data(AVFrame *frame,
> >                                             size_t nb_rects);
> > 
> > AVVideoHint *av_video_hint_extend_side_data(AVFrame *frame,
> >                                             size_t nb_rects);
> > 
> > by marking the continuation on the previous hint (but this would
> > complicate deserialization as you need then to iterate to get all the
> > side data).
> > 
> > Or we could make a variadic function like this:
> > AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t
> > nb_rects, ...);
> > 
> > Alternatively, we might store in AVVideoHint more than one hint, but
> > this somehow conflict with the general design.
> > 
> > ...
> > 
> > As a low effort approach, I would keep the initial design of a single
> > hint type per side-data (dropping the possibility of having more than
> > one hint type in the same side data), which should be fixed
> > generically by implementing 1.
> 
> Agreed.
> For clarity I am attaching again the patch with Anton's changes just
> rebased on the current master branch.
> Anton, is this still ok to be merged?

I'm fine with the current patch, the behavior can then be extended
by extending side data with multiple-label for the same type.

Anton, if you are fine with it I would merge this one (and maybe get
included in the next release if we are not too late already).

Thanks.