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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Friday, December 3, 2021 9:23 AM
> To: FFmpeg development discussions and patches <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-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, 

Well, at times it feels like that..

> 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.

I understand your concern from an abstract point of view. But the point
about which you want to be concerned has already happened 7 years ago
and now there exist more than 200 usages of av_frame_copy_props(), all
of which are possibly expecting the special treatment of PANSCAN.
That makes me wonder, whether there might be any practical use for 
a function av_frame_copy_side_data() that behaves differently?

I think the only way to solve this would be to investigate the situation
from the side of PANSCAN data handling to understand the subject and 
see what could be done.

> 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.

Makes sense by theory, I'm just wondering whether there's a practical 
need for this. But hey, go for it, it's fine with me! :-)

> 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.

That would be great and save me from doing a few more iterations, thanks!

Can you see the V3? Patchwork has it: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=5391


Thanks again,
sw