[FFmpeg-devel] [PATCH v2 1/1] avcodec/vpp_qsv: Copy side data from input to output frame

[Anton Khirnov]

Quoting Soft Works (2021-12-03 08:55:19)
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Wednesday, December 1, 2021 11:55 AM
> > To: ffmpeg-devel at ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avcodec/vpp_qsv: Copy side data
> > from input to output frame
> > 
> > Quoting Soft Works (2021-11-30 15:22:38)
> > > Signed-off-by: softworkz <softworkz at hotmail.com>
> > > ---
> > > V2: Add public method av_frame_copy_side_data() instead to copying the
> > implementation.
> > >
> > >  libavfilter/qsvvpp.c         |  5 ++++
> > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++---
> > >  libavutil/frame.c            | 57 ++++++++++++++++++++----------------
> > >  libavutil/frame.h            | 12 ++++++++
> > >  4 files changed, 64 insertions(+), 29 deletions(-)
> > 
> > This should be split: new API in one commit, using it in the second.
> > New API also needs a minor version bump and a doc/APIchanges entry.
> > 
> > > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > > index 1f1f573407..7a19245ea4 100644
> > > --- a/libavutil/frame.c
> > > +++ b/libavutil/frame.c
> > > @@ -296,6 +296,36 @@ int av_frame_get_buffer2(AVFrame *frame, int align)
> > >      }
> > >  }
> > >
> > > +int av_frame_copy_side_data(AVFrame* dst, const AVFrame* src, int
> > force_copy)
> > > +{
> > > +    for (unsigned i = 0; i < src->nb_side_data; i++) {
> > > +        const AVFrameSideData *sd_src = src->side_data[i];
> > > +        AVFrameSideData *sd_dst;
> > > +        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
> > > +            && (src->width != dst->width || src->height != dst->height))
> > > +            continue;
> > 
> > Yuck. I know it's already there, but it really really shouldn't be. This
> > function does not have enough information to make such decisions.
> 
> Agreed. But I don't have enough information to do anything about it.
> 
> This code exists since October 2014. It has become part of ffmpeg 
> behavior and I'm not familiar enough with the subject of this specific 
> side data to make any suggestion. Even when - such change would be
> unrelated and wouldn't belong into this patchset anyway.

I agree that you shouldn't be forced into fixing unrelated issues, but
I'm concerned that this behavior will become hardcoded into this new API
and we won't be able to get rid of it in the future.

How about adding a flag called something like
AV_FRAME_COPY_PROPS_FILTER_SIDE_DATA that will control this behavior?
Then the existing callers of frame_copy_props() can set this flag, so
this patch won't change their behavior.

Then later we can replace this hack with something saner and deprecate
the flag.

I can do this change myself if it's too much work for you, but I'll need
the updated patch.

-- 
Anton Khirnov