| 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 | expand |
| 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 |
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.
> -----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
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.
> -----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
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
> -----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
> > -----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
> -----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 --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; }
Signed-off-by: softworkz <softworkz@hotmail.com> --- libavfilter/qsvvpp.c | 5 +++++ libavfilter/vf_overlay_qsv.c | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)