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

[Stefano Sabatini]

On date Friday 2023-07-28 09:44:37 +0200, Stefano Sabatini wrote:
> 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).

I'll push this in a few days unless there are more comments.