diff mbox series

[FFmpeg-devel,v3,2/2] avcodec/vpp_qsv: Copy side data from input to output frame

Message ID DM8P223MB0365CD2B520BB9B09046AADFBA6A9@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM
State New
Headers show
Series [FFmpeg-devel,v3,1/2] avutil/frame: Add av_frame_copy_side_data() function
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Soft Works Dec. 3, 2021, 7:58 a.m. UTC
Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/qsvvpp.c         |  5 +++++
 libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Anton Khirnov Dec. 7, 2021, 8:03 a.m. UTC | #1
Quoting Soft Works (2021-12-03 08:58:31)
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  libavfilter/qsvvpp.c         |  5 +++++
>  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> index d1218355c7..b291216292 100644
> --- a/libavfilter/qsvvpp.c
> +++ b/libavfilter/qsvvpp.c
> @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
>                  return AVERROR(EAGAIN);
>              break;
>          }
> +
> +        ret = av_frame_copy_side_data(out_frame->frame, in_frame->frame, 0);
> +        if (ret < 0)
> +            return ret;
> +
>          out_frame->frame->pts = av_rescale_q(out_frame->surface.Data.TimeStamp,
>                                               default_tb, outlink->time_base);
>  
> diff --git a/libavfilter/vf_overlay_qsv.c b/libavfilter/vf_overlay_qsv.c
> index 7e76b39aa9..02518e020c 100644
> --- a/libavfilter/vf_overlay_qsv.c
> +++ b/libavfilter/vf_overlay_qsv.c
> @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
>  {
>      AVFilterContext  *ctx = fs->parent;
>      QSVOverlayContext  *s = fs->opaque;
> +    AVFrame       *frame0 = NULL;
>      AVFrame        *frame = NULL;
> -    int               ret = 0, i;
> +    int               ret = 0;
>  
> -    for (i = 0; i < ctx->nb_inputs; i++) {
> +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
>          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> -        if (ret == 0)
> -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);
> +
> +        if (ret == 0) {
> +            AVFrame *temp;
> +
> +            if (i == 0)
> +                frame0 = frame;
> +            else
> +                ret = av_frame_copy_side_data(frame, frame0, 0);
> +
> +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);

I don't quite understand the ownership semantics here. This function
does not free frame, so I assume ff_qsvvpp_filter_frame() takes
ownership of it. That would mean you're not allowed to keep a pointer to
it and access it later, because it might have already been freed.
Soft Works Dec. 7, 2021, 8:55 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Tuesday, December 7, 2021 9:04 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> from input to output frame
> 
> Quoting Soft Works (2021-12-03 08:58:31)
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  libavfilter/qsvvpp.c         |  5 +++++
> >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > index d1218355c7..b291216292 100644
> > --- a/libavfilter/qsvvpp.c
> > +++ b/libavfilter/qsvvpp.c
> > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> AVFilterLink *inlink, AVFrame *picr
> >                  return AVERROR(EAGAIN);
> >              break;
> >          }
> > +
> > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame->frame,
> 0);
> > +        if (ret < 0)
> > +            return ret;
> > +
> >          out_frame->frame->pts = av_rescale_q(out_frame-
> >surface.Data.TimeStamp,
> >                                               default_tb, outlink-
> >time_base);
> >
> > diff --git a/libavfilter/vf_overlay_qsv.c b/libavfilter/vf_overlay_qsv.c
> > index 7e76b39aa9..02518e020c 100644
> > --- a/libavfilter/vf_overlay_qsv.c
> > +++ b/libavfilter/vf_overlay_qsv.c
> > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> >  {
> >      AVFilterContext  *ctx = fs->parent;
> >      QSVOverlayContext  *s = fs->opaque;
> > +    AVFrame       *frame0 = NULL;
> >      AVFrame        *frame = NULL;
> > -    int               ret = 0, i;
> > +    int               ret = 0;
> >
> > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > -        if (ret == 0)
> > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);
> > +
> > +        if (ret == 0) {
> > +            AVFrame *temp;
> > +
> > +            if (i == 0)
> > +                frame0 = frame;
> > +            else
> > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > +
> > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx-
> >inputs[i], frame);
> 
> I don't quite understand the ownership semantics here. This function
> does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> ownership of it. That would mean you're not allowed to keep a pointer to
> it and access it later, because it might have already been freed.

The filter is using framesync, which is taking care of the ownership.
ff_qsvvpp_filter_frame() clones or copies the frame, depending on case.
Other than with the normal overlay filter, the frame from input0 is
not used for output. But the regular overlay filter has established the
convention that side data from input0 is being kept at the output.

This patch is making sure that overlay_qsv behaves in the same way.
The problem is that (due to the async nature of processing in qsvvpp)
it can't always take the side data from the input0-frame, because there's
no output frame at this stage. So we copy the side data to the frame from
input1, to make sure that the "right" side data is copied (transferred) in
either case.

Thanks for looking at this,
softworkz
Anton Khirnov Dec. 7, 2021, 11:50 a.m. UTC | #3
Quoting Soft Works (2021-12-07 09:55:37)
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Tuesday, December 7, 2021 9:04 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> > from input to output frame
> > 
> > Quoting Soft Works (2021-12-03 08:58:31)
> > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > ---
> > >  libavfilter/qsvvpp.c         |  5 +++++
> > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > index d1218355c7..b291216292 100644
> > > --- a/libavfilter/qsvvpp.c
> > > +++ b/libavfilter/qsvvpp.c
> > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > AVFilterLink *inlink, AVFrame *picr
> > >                  return AVERROR(EAGAIN);
> > >              break;
> > >          }
> > > +
> > > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame->frame,
> > 0);
> > > +        if (ret < 0)
> > > +            return ret;
> > > +
> > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > >surface.Data.TimeStamp,
> > >                                               default_tb, outlink-
> > >time_base);
> > >
> > > diff --git a/libavfilter/vf_overlay_qsv.c b/libavfilter/vf_overlay_qsv.c
> > > index 7e76b39aa9..02518e020c 100644
> > > --- a/libavfilter/vf_overlay_qsv.c
> > > +++ b/libavfilter/vf_overlay_qsv.c
> > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > >  {
> > >      AVFilterContext  *ctx = fs->parent;
> > >      QSVOverlayContext  *s = fs->opaque;
> > > +    AVFrame       *frame0 = NULL;
> > >      AVFrame        *frame = NULL;
> > > -    int               ret = 0, i;
> > > +    int               ret = 0;
> > >
> > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > -        if (ret == 0)
> > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);
> > > +
> > > +        if (ret == 0) {
> > > +            AVFrame *temp;
> > > +
> > > +            if (i == 0)
> > > +                frame0 = frame;
> > > +            else
> > > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > > +
> > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx-
> > >inputs[i], frame);
> > 
> > I don't quite understand the ownership semantics here. This function
> > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > ownership of it. That would mean you're not allowed to keep a pointer to
> > it and access it later, because it might have already been freed.
> 
> The filter is using framesync, which is taking care of the ownership.
> ff_qsvvpp_filter_frame() clones or copies the frame, depending on case.
> Other than with the normal overlay filter, the frame from input0 is
> not used for output. But the regular overlay filter has established the
> convention that side data from input0 is being kept at the output.

Okay, if you're sure that framesync guarantees the frame remaining valid
then I have no objections.

But note that temp is unused and should be removed.
Soft Works Dec. 7, 2021, 12:39 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Tuesday, December 7, 2021 12:51 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> from input to output frame
> 
> Quoting Soft Works (2021-12-07 09:55:37)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Tuesday, December 7, 2021 9:04 AM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> data
> > > from input to output frame
> > >
> > > Quoting Soft Works (2021-12-03 08:58:31)
> > > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > > ---
> > > >  libavfilter/qsvvpp.c         |  5 +++++
> > > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > > index d1218355c7..b291216292 100644
> > > > --- a/libavfilter/qsvvpp.c
> > > > +++ b/libavfilter/qsvvpp.c
> > > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > > AVFilterLink *inlink, AVFrame *picr
> > > >                  return AVERROR(EAGAIN);
> > > >              break;
> > > >          }
> > > > +
> > > > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame-
> >frame,
> > > 0);
> > > > +        if (ret < 0)
> > > > +            return ret;
> > > > +
> > > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > >surface.Data.TimeStamp,
> > > >                                               default_tb, outlink-
> > > >time_base);
> > > >
> > > > diff --git a/libavfilter/vf_overlay_qsv.c
> b/libavfilter/vf_overlay_qsv.c
> > > > index 7e76b39aa9..02518e020c 100644
> > > > --- a/libavfilter/vf_overlay_qsv.c
> > > > +++ b/libavfilter/vf_overlay_qsv.c
> > > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > > >  {
> > > >      AVFilterContext  *ctx = fs->parent;
> > > >      QSVOverlayContext  *s = fs->opaque;
> > > > +    AVFrame       *frame0 = NULL;
> > > >      AVFrame        *frame = NULL;
> > > > -    int               ret = 0, i;
> > > > +    int               ret = 0;
> > > >
> > > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > > -        if (ret == 0)
> > > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i],
> frame);
> > > > +
> > > > +        if (ret == 0) {
> > > > +            AVFrame *temp;
> > > > +
> > > > +            if (i == 0)
> > > > +                frame0 = frame;
> > > > +            else
> > > > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > > > +
> > > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx-
> > > >inputs[i], frame);
> > >
> > > I don't quite understand the ownership semantics here. This function
> > > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > > ownership of it. That would mean you're not allowed to keep a pointer to
> > > it and access it later, because it might have already been freed.
> >
> > The filter is using framesync, which is taking care of the ownership.
> > ff_qsvvpp_filter_frame() clones or copies the frame, depending on case.
> > Other than with the normal overlay filter, the frame from input0 is
> > not used for output. But the regular overlay filter has established the
> > convention that side data from input0 is being kept at the output.
> 
> Okay, if you're sure that framesync guarantees the frame remaining valid
> then I have no objections.
> 
> But note that temp is unused and should be removed.

OK, thanks. Let's see what Haihao says, he's closest to the subject at the
moment.

softworkz
Xiang, Haihao Dec. 9, 2021, 8:35 a.m. UTC | #5
On Tue, 2021-12-07 at 12:39 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > Khirnov
> > Sent: Tuesday, December 7, 2021 12:51 PM
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> > from input to output frame
> > 
> > Quoting Soft Works (2021-12-07 09:55:37)
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > > Khirnov
> > > > Sent: Tuesday, December 7, 2021 9:04 AM
> > > > To: ffmpeg-devel@ffmpeg.org
> > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> > 
> > data
> > > > from input to output frame
> > > > 
> > > > Quoting Soft Works (2021-12-03 08:58:31)
> > > > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > > > ---
> > > > >  libavfilter/qsvvpp.c         |  5 +++++
> > > > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > > > index d1218355c7..b291216292 100644
> > > > > --- a/libavfilter/qsvvpp.c
> > > > > +++ b/libavfilter/qsvvpp.c
> > > > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > > > 
> > > > AVFilterLink *inlink, AVFrame *picr
> > > > >                  return AVERROR(EAGAIN);
> > > > >              break;
> > > > >          }
> > > > > +
> > > > > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame-
> > > 
> > > frame,
> > > > 0);
> > > > > +        if (ret < 0)
> > > > > +            return ret;
> > > > > +
> > > > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > > > surface.Data.TimeStamp,
> > > > >                                               default_tb, outlink-
> > > > > time_base);
> > > > > 
> > > > > diff --git a/libavfilter/vf_overlay_qsv.c
> > 
> > b/libavfilter/vf_overlay_qsv.c
> > > > > index 7e76b39aa9..02518e020c 100644
> > > > > --- a/libavfilter/vf_overlay_qsv.c
> > > > > +++ b/libavfilter/vf_overlay_qsv.c
> > > > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > > > >  {
> > > > >      AVFilterContext  *ctx = fs->parent;
> > > > >      QSVOverlayContext  *s = fs->opaque;
> > > > > +    AVFrame       *frame0 = NULL;
> > > > >      AVFrame        *frame = NULL;
> > > > > -    int               ret = 0, i;
> > > > > +    int               ret = 0;
> > > > > 
> > > > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > > > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > > > -        if (ret == 0)
> > > > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i],
> > 
> > frame);
> > > > > +
> > > > > +        if (ret == 0) {
> > > > > +            AVFrame *temp;
> > > > > +
> > > > > +            if (i == 0)
> > > > > +                frame0 = frame;
> > > > > +            else
> > > > > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > > > > +
> > > > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx-
> > > > > inputs[i], frame);
> > > > 
> > > > I don't quite understand the ownership semantics here. This function
> > > > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > > > ownership of it. That would mean you're not allowed to keep a pointer to
> > > > it and access it later, because it might have already been freed.
> > > 
> > > The filter is using framesync, which is taking care of the ownership.
> > > ff_qsvvpp_filter_frame() clones or copies the frame, depending on case.
> > > Other than with the normal overlay filter, the frame from input0 is
> > > not used for output. But the regular overlay filter has established the
> > > convention that side data from input0 is being kept at the output.
> > 
> > Okay, if you're sure that framesync guarantees the frame remaining valid
> > then I have no objections.
> > 
> > But note that temp is unused and should be removed.
> 
> OK, thanks. Let's see what Haihao says, he's closest to the subject at the
> moment.

Is it possible that the frame from input1 and the frame from input0 have the
same type of side data ? If yes, which one will take effect in the output frame
?

Thanks
Haihao
Soft Works Dec. 9, 2021, 8:40 a.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang,
> Haihao
> Sent: Thursday, December 9, 2021 9:35 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> from input to output frame
> 
> On Tue, 2021-12-07 at 12:39 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Tuesday, December 7, 2021 12:51 PM
> > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> data
> > > from input to output frame
> > >
> > > Quoting Soft Works (2021-12-07 09:55:37)
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton
> > > > > Khirnov
> > > > > Sent: Tuesday, December 7, 2021 9:04 AM
> > > > > To: ffmpeg-devel@ffmpeg.org
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> > >
> > > data
> > > > > from input to output frame
> > > > >
> > > > > Quoting Soft Works (2021-12-03 08:58:31)
> > > > > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > > > > ---
> > > > > >  libavfilter/qsvvpp.c         |  5 +++++
> > > > > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > > > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > > > > index d1218355c7..b291216292 100644
> > > > > > --- a/libavfilter/qsvvpp.c
> > > > > > +++ b/libavfilter/qsvvpp.c
> > > > > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > > > >
> > > > > AVFilterLink *inlink, AVFrame *picr
> > > > > >                  return AVERROR(EAGAIN);
> > > > > >              break;
> > > > > >          }
> > > > > > +
> > > > > > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame-
> > > >
> > > > frame,
> > > > > 0);
> > > > > > +        if (ret < 0)
> > > > > > +            return ret;
> > > > > > +
> > > > > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > > > > surface.Data.TimeStamp,
> > > > > >                                               default_tb, outlink-
> > > > > > time_base);
> > > > > >
> > > > > > diff --git a/libavfilter/vf_overlay_qsv.c
> > >
> > > b/libavfilter/vf_overlay_qsv.c
> > > > > > index 7e76b39aa9..02518e020c 100644
> > > > > > --- a/libavfilter/vf_overlay_qsv.c
> > > > > > +++ b/libavfilter/vf_overlay_qsv.c
> > > > > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > > > > >  {
> > > > > >      AVFilterContext  *ctx = fs->parent;
> > > > > >      QSVOverlayContext  *s = fs->opaque;
> > > > > > +    AVFrame       *frame0 = NULL;
> > > > > >      AVFrame        *frame = NULL;
> > > > > > -    int               ret = 0, i;
> > > > > > +    int               ret = 0;
> > > > > >
> > > > > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > > > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > > > > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > > > > -        if (ret == 0)
> > > > > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i],
> > >
> > > frame);
> > > > > > +
> > > > > > +        if (ret == 0) {
> > > > > > +            AVFrame *temp;
> > > > > > +
> > > > > > +            if (i == 0)
> > > > > > +                frame0 = frame;
> > > > > > +            else
> > > > > > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > > > > > +
> > > > > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv,
> ctx-
> > > > > > inputs[i], frame);
> > > > >
> > > > > I don't quite understand the ownership semantics here. This function
> > > > > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > > > > ownership of it. That would mean you're not allowed to keep a pointer
> to
> > > > > it and access it later, because it might have already been freed.
> > > >
> > > > The filter is using framesync, which is taking care of the ownership.
> > > > ff_qsvvpp_filter_frame() clones or copies the frame, depending on case.
> > > > Other than with the normal overlay filter, the frame from input0 is
> > > > not used for output. But the regular overlay filter has established the
> > > > convention that side data from input0 is being kept at the output.
> > >
> > > Okay, if you're sure that framesync guarantees the frame remaining valid
> > > then I have no objections.
> > >
> > > But note that temp is unused and should be removed.
> >
> > OK, thanks. Let's see what Haihao says, he's closest to the subject at the
> > moment.
> 
> Is it possible that the frame from input1 and the frame from input0 have the
> same type of side data ? If yes, which one will take effect in the output
> frame
> ?

You can see in the code above that I'm copying the side data from frame0 
to frame1 before submitting frame1 to qsvvpp. So, it's always the side data from
frame0 that will be used.
This achieves the same behavior as the other overlay filters.

Thanks,
softworkz
Xiang, Haihao Dec. 13, 2021, 5:55 a.m. UTC | #7
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang,
> > Haihao
> > Sent: Thursday, December 9, 2021 9:35 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> > from input to output frame
> > 
> > On Tue, 2021-12-07 at 12:39 +0000, Soft Works wrote:
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> > > > Khirnov
> > > > Sent: Tuesday, December 7, 2021 12:51 PM
> > > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> > 
> > data
> > > > from input to output frame
> > > > 
> > > > Quoting Soft Works (2021-12-07 09:55:37)
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > 
> > Anton
> > > > > > Khirnov
> > > > > > Sent: Tuesday, December 7, 2021 9:04 AM
> > > > > > To: ffmpeg-devel@ffmpeg.org
> > > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy
> > > > > > side
> > > > 
> > > > data
> > > > > > from input to output frame
> > > > > > 
> > > > > > Quoting Soft Works (2021-12-03 08:58:31)
> > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > > > > > ---
> > > > > > >  libavfilter/qsvvpp.c         |  5 +++++
> > > > > > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > > > > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > > > > > index d1218355c7..b291216292 100644
> > > > > > > --- a/libavfilter/qsvvpp.c
> > > > > > > +++ b/libavfilter/qsvvpp.c
> > > > > > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > > > > > 
> > > > > > AVFilterLink *inlink, AVFrame *picr
> > > > > > >                  return AVERROR(EAGAIN);
> > > > > > >              break;
> > > > > > >          }
> > > > > > > +
> > > > > > > +        ret = av_frame_copy_side_data(out_frame->frame, in_frame-
> > > > > 
> > > > > frame,
> > > > > > 0);
> > > > > > > +        if (ret < 0)
> > > > > > > +            return ret;
> > > > > > > +
> > > > > > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > > > > > surface.Data.TimeStamp,
> > > > > > >                                               default_tb, outlink-
> > > > > > > time_base);
> > > > > > > 
> > > > > > > diff --git a/libavfilter/vf_overlay_qsv.c
> > > > 
> > > > b/libavfilter/vf_overlay_qsv.c
> > > > > > > index 7e76b39aa9..02518e020c 100644
> > > > > > > --- a/libavfilter/vf_overlay_qsv.c
> > > > > > > +++ b/libavfilter/vf_overlay_qsv.c
> > > > > > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > > > > > >  {
> > > > > > >      AVFilterContext  *ctx = fs->parent;
> > > > > > >      QSVOverlayContext  *s = fs->opaque;
> > > > > > > +    AVFrame       *frame0 = NULL;
> > > > > > >      AVFrame        *frame = NULL;
> > > > > > > -    int               ret = 0, i;
> > > > > > > +    int               ret = 0;
> > > > > > > 
> > > > > > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > > > > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > > > > > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > > > > > -        if (ret == 0)
> > > > > > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i],
> > > > 
> > > > frame);
> > > > > > > +
> > > > > > > +        if (ret == 0) {
> > > > > > > +            AVFrame *temp;
> > > > > > > +
> > > > > > > +            if (i == 0)
> > > > > > > +                frame0 = frame;
> > > > > > > +            else
> > > > > > > +                ret = av_frame_copy_side_data(frame, frame0, 0);
> > > > > > > +
> > > > > > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv,
> > 
> > ctx-
> > > > > > > inputs[i], frame);
> > > > > > 
> > > > > > I don't quite understand the ownership semantics here. This function
> > > > > > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > > > > > ownership of it. That would mean you're not allowed to keep a
> > > > > > pointer
> > 
> > to
> > > > > > it and access it later, because it might have already been freed.
> > > > > 
> > > > > The filter is using framesync, which is taking care of the ownership.
> > > > > ff_qsvvpp_filter_frame() clones or copies the frame, depending on
> > > > > case.
> > > > > Other than with the normal overlay filter, the frame from input0 is
> > > > > not used for output. But the regular overlay filter has established
> > > > > the
> > > > > convention that side data from input0 is being kept at the output.
> > > > 
> > > > Okay, if you're sure that framesync guarantees the frame remaining valid
> > > > then I have no objections.
> > > > 
> > > > But note that temp is unused and should be removed.
> > > 
> > > OK, thanks. Let's see what Haihao says, he's closest to the subject at the
> > > moment.
> > 
> > Is it possible that the frame from input1 and the frame from input0 have the
> > same type of side data ? If yes, which one will take effect in the output
> > frame
> > ?
> 
> You can see in the code above that I'm copying the side data from frame0 
> to frame1 before submitting frame1 to qsvvpp. So, it's always the side data
> from
> frame0 that will be used.
> This achieves the same behavior as the other overlay filters.

av_frame_copy_side_data() is used to copy the side data from frame0 to frame1.
However in av_frame_copy_side_data(), av_frame_new_side_data_from_buf() is used
to create a new side data of a given type for frame1. If frame1 has already
haven a side data of a given type before copying the side data, frame1 will have
two side data with the same type after copying. Some av functions, e.g.
av_frame_get_side_data() only takes the fist side data of a given type into
account, so it is possible the side data from frame0 won't be used in the final
result. 

I think av_frame_remove_side_data() should be used on frame1 before copying the
side data? 

Thanks
Haihao
Soft Works Dec. 13, 2021, 7:10 a.m. UTC | #8
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang,
> Haihao
> Sent: Monday, December 13, 2021 6:55 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side data
> from input to output frame
> 
> 
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang,
> > > Haihao
> > > Sent: Thursday, December 9, 2021 9:35 AM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> data
> > > from input to output frame
> > >
> > > On Tue, 2021-12-07 at 12:39 +0000, Soft Works wrote:
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton
> > > > > Khirnov
> > > > > Sent: Tuesday, December 7, 2021 12:51 PM
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy side
> > >
> > > data
> > > > > from input to output frame
> > > > >
> > > > > Quoting Soft Works (2021-12-07 09:55:37)
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > >
> > > Anton
> > > > > > > Khirnov
> > > > > > > Sent: Tuesday, December 7, 2021 9:04 AM
> > > > > > > To: ffmpeg-devel@ffmpeg.org
> > > > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avcodec/vpp_qsv: Copy
> > > > > > > side
> > > > >
> > > > > data
> > > > > > > from input to output frame
> > > > > > >
> > > > > > > Quoting Soft Works (2021-12-03 08:58:31)
> > > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com>
> > > > > > > > ---
> > > > > > > >  libavfilter/qsvvpp.c         |  5 +++++
> > > > > > > >  libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++----
> > > > > > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> > > > > > > > index d1218355c7..b291216292 100644
> > > > > > > > --- a/libavfilter/qsvvpp.c
> > > > > > > > +++ b/libavfilter/qsvvpp.c
> > > > > > > > @@ -849,6 +849,11 @@ int ff_qsvvpp_filter_frame(QSVVPPContext
> *s,
> > > > > > >
> > > > > > > AVFilterLink *inlink, AVFrame *picr
> > > > > > > >                  return AVERROR(EAGAIN);
> > > > > > > >              break;
> > > > > > > >          }
> > > > > > > > +
> > > > > > > > +        ret = av_frame_copy_side_data(out_frame->frame,
> in_frame-
> > > > > >
> > > > > > frame,
> > > > > > > 0);
> > > > > > > > +        if (ret < 0)
> > > > > > > > +            return ret;
> > > > > > > > +
> > > > > > > >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > > > > > > surface.Data.TimeStamp,
> > > > > > > >                                               default_tb,
> outlink-
> > > > > > > > time_base);
> > > > > > > >
> > > > > > > > diff --git a/libavfilter/vf_overlay_qsv.c
> > > > >
> > > > > b/libavfilter/vf_overlay_qsv.c
> > > > > > > > index 7e76b39aa9..02518e020c 100644
> > > > > > > > --- a/libavfilter/vf_overlay_qsv.c
> > > > > > > > +++ b/libavfilter/vf_overlay_qsv.c
> > > > > > > > @@ -231,13 +231,24 @@ static int process_frame(FFFrameSync *fs)
> > > > > > > >  {
> > > > > > > >      AVFilterContext  *ctx = fs->parent;
> > > > > > > >      QSVOverlayContext  *s = fs->opaque;
> > > > > > > > +    AVFrame       *frame0 = NULL;
> > > > > > > >      AVFrame        *frame = NULL;
> > > > > > > > -    int               ret = 0, i;
> > > > > > > > +    int               ret = 0;
> > > > > > > >
> > > > > > > > -    for (i = 0; i < ctx->nb_inputs; i++) {
> > > > > > > > +    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
> > > > > > > >          ret = ff_framesync_get_frame(fs, i, &frame, 0);
> > > > > > > > -        if (ret == 0)
> > > > > > > > -            ret = ff_qsvvpp_filter_frame(s->qsv, ctx-
> >inputs[i],
> > > > >
> > > > > frame);
> > > > > > > > +
> > > > > > > > +        if (ret == 0) {
> > > > > > > > +            AVFrame *temp;
> > > > > > > > +
> > > > > > > > +            if (i == 0)
> > > > > > > > +                frame0 = frame;
> > > > > > > > +            else
> > > > > > > > +                ret = av_frame_copy_side_data(frame, frame0,
> 0);
> > > > > > > > +
> > > > > > > > +            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s-
> >qsv,
> > >
> > > ctx-
> > > > > > > > inputs[i], frame);
> > > > > > >
> > > > > > > I don't quite understand the ownership semantics here. This
> function
> > > > > > > does not free frame, so I assume ff_qsvvpp_filter_frame() takes
> > > > > > > ownership of it. That would mean you're not allowed to keep a
> > > > > > > pointer
> > >
> > > to
> > > > > > > it and access it later, because it might have already been freed.
> > > > > >
> > > > > > The filter is using framesync, which is taking care of the
> ownership.
> > > > > > ff_qsvvpp_filter_frame() clones or copies the frame, depending on
> > > > > > case.
> > > > > > Other than with the normal overlay filter, the frame from input0 is
> > > > > > not used for output. But the regular overlay filter has established
> > > > > > the
> > > > > > convention that side data from input0 is being kept at the output.
> > > > >
> > > > > Okay, if you're sure that framesync guarantees the frame remaining
> valid
> > > > > then I have no objections.
> > > > >
> > > > > But note that temp is unused and should be removed.
> > > >
> > > > OK, thanks. Let's see what Haihao says, he's closest to the subject at
> the
> > > > moment.
> > >
> > > Is it possible that the frame from input1 and the frame from input0 have
> the
> > > same type of side data ? If yes, which one will take effect in the output
> > > frame
> > > ?
> >
> > You can see in the code above that I'm copying the side data from frame0
> > to frame1 before submitting frame1 to qsvvpp. So, it's always the side data
> > from
> > frame0 that will be used.
> > This achieves the same behavior as the other overlay filters.
> 
> av_frame_copy_side_data() is used to copy the side data from frame0 to
> frame1.
> However in av_frame_copy_side_data(), av_frame_new_side_data_from_buf() is
> used
> to create a new side data of a given type for frame1. If frame1 has already
> haven a side data of a given type before copying the side data, frame1 will
> have
> two side data with the same type after copying. Some av functions, e.g.
> av_frame_get_side_data() only takes the fist side data of a given type into
> account, so it is possible the side data from frame0 won't be used in the
> final
> result.
> 
> I think av_frame_remove_side_data() should be used on frame1 before copying
> the
> side data?

Yes, you are right, that makes sense. I need to update the patch anyway
as Anton has taken vase of the 1/2 part. 

I'll post a new patch shortly.

Thanks a lot for reviewing,
softworkz
diff mbox series

Patch

diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
index d1218355c7..b291216292 100644
--- a/libavfilter/qsvvpp.c
+++ b/libavfilter/qsvvpp.c
@@ -849,6 +849,11 @@  int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
                 return AVERROR(EAGAIN);
             break;
         }
+
+        ret = av_frame_copy_side_data(out_frame->frame, in_frame->frame, 0);
+        if (ret < 0)
+            return ret;
+
         out_frame->frame->pts = av_rescale_q(out_frame->surface.Data.TimeStamp,
                                              default_tb, outlink->time_base);
 
diff --git a/libavfilter/vf_overlay_qsv.c b/libavfilter/vf_overlay_qsv.c
index 7e76b39aa9..02518e020c 100644
--- a/libavfilter/vf_overlay_qsv.c
+++ b/libavfilter/vf_overlay_qsv.c
@@ -231,13 +231,24 @@  static int process_frame(FFFrameSync *fs)
 {
     AVFilterContext  *ctx = fs->parent;
     QSVOverlayContext  *s = fs->opaque;
+    AVFrame       *frame0 = NULL;
     AVFrame        *frame = NULL;
-    int               ret = 0, i;
+    int               ret = 0;
 
-    for (i = 0; i < ctx->nb_inputs; i++) {
+    for (unsigned i = 0; i < ctx->nb_inputs; i++) {
         ret = ff_framesync_get_frame(fs, i, &frame, 0);
-        if (ret == 0)
-            ret = ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);
+
+        if (ret == 0) {
+            AVFrame *temp;
+
+            if (i == 0)
+                frame0 = frame;
+            else
+                ret = av_frame_copy_side_data(frame, frame0, 0);
+
+            ret = ret < 0 ? ret : ff_qsvvpp_filter_frame(s->qsv, ctx->inputs[i], frame);
+        }
+
         if (ret < 0 && ret != AVERROR(EAGAIN))
             break;
     }