| Message ID | DM8P223MB036556C935366DDDCCA9F425BA679@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/1] avcodec/vpp_qsv: Copy side data from input to output frame | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/makex86 | warning | New warnings during build |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/makeppc | warning | New warnings during build |
Quoting Soft Works (2021-11-30 15:22:38) > Signed-off-by: softworkz <softworkz@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. > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 9dfd5a886a..6dec040ce8 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -885,6 +885,18 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src); > */ > int av_frame_copy_props(AVFrame *dst, const AVFrame *src); > > +/** > + * Copy only side-data from src to dst. > + * > + * @param dst a frame to which the side data should be copied. > + * @param src a frame from which to copy the side data. > + * @param force_copy determines whether to copy the actual data or only just > + * create references to the buffers. > + * > + * @return >= 0 on success, a negative AVERROR on error. > + */ > +int av_frame_copy_side_data(AVFrame* dst, const AVFrame* src, int force_copy); Better change force_copy into unsigned int flags and define e.g. AV_FRAME_COPY_PROPS_REF to mean you want refs rather than copies. That way you're not wasting an entire integer argument on a single flag and allowing future extensions.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Wednesday, December 1, 2021 11:55 AM > To: ffmpeg-devel@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@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 made all other changes, though. Thanks for reviewing, softworkz
Quoting Soft Works (2021-12-03 08:55:19) > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > Khirnov > > Sent: Wednesday, December 1, 2021 11:55 AM > > To: ffmpeg-devel@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@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.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Friday, December 3, 2021 9:23 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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@ffmpeg.org> On Behalf Of Anton > > > Khirnov > > > Sent: Wednesday, December 1, 2021 11:55 AM > > > To: ffmpeg-devel@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@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
Quoting Soft Works (2021-12-03 09:51:13) > > 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 suspect the overwhelming majority of these calls are done between the frames with the same resolution, so the relevant code is not executed. The places where it does matter should be quite few. And more generally, there's a lot of stuff in our codebase that shouldn't be there. We shouldn't give up on improving things just because they have been around for a long time.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Tuesday, December 7, 2021 9:08 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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 09:51:13) > > > 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 suspect the overwhelming majority of these calls are done between the > frames with the same resolution, so the relevant code is not executed. > The places where it does matter should be quite few. > > And more generally, there's a lot of stuff in our codebase that > shouldn't be there. We shouldn't give up on improving things just > because they have been around for a long time. I'm totally with you. I mean, I've gone over 4 patch versions just for improving the documentation of the linesize[] parameter ;-) You didn't quote the other part where I said that it might more sense to investigate the PANSCAN story directly. Anyway, I think the solution with the flags is good now. softworkz
Quoting Soft Works (2021-12-07 10:03:28) > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > Khirnov > > Sent: Tuesday, December 7, 2021 9:08 AM > > To: FFmpeg development discussions and patches <ffmpeg-devel@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 09:51:13) > > > > 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 suspect the overwhelming majority of these calls are done between the > > frames with the same resolution, so the relevant code is not executed. > > The places where it does matter should be quite few. > > > > And more generally, there's a lot of stuff in our codebase that > > shouldn't be there. We shouldn't give up on improving things just > > because they have been around for a long time. > > I'm totally with you. I mean, I've gone over 4 patch versions just for improving > the documentation of the linesize[] parameter ;-) > > You didn't quote the other part where I said that it might more sense to > investigate the PANSCAN story directly. > Anyway, I think the solution with the flags is good now. I had nothing to add there. You are correct, the reason that code was added should be investigated and we should find a better solution to the problem. But that's for whenever somebody feels motivated enough to do it.
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; } 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; + if (force_copy) { + sd_dst = av_frame_new_side_data(dst, sd_src->type, + sd_src->size); + if (!sd_dst) { + wipe_side_data(dst); + return AVERROR(ENOMEM); + } + memcpy(sd_dst->data, sd_src->data, sd_src->size); + } else { + AVBufferRef *ref = av_buffer_ref(sd_src->buf); + sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref); + if (!sd_dst) { + av_buffer_unref(&ref); + wipe_side_data(dst); + return AVERROR(ENOMEM); + } + } + av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0); + } + return 0; +} + static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy) { int ret, i; @@ -340,31 +370,8 @@ static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy) av_dict_copy(&dst->metadata, src->metadata, 0); - for (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; - if (force_copy) { - sd_dst = av_frame_new_side_data(dst, sd_src->type, - sd_src->size); - if (!sd_dst) { - wipe_side_data(dst); - return AVERROR(ENOMEM); - } - memcpy(sd_dst->data, sd_src->data, sd_src->size); - } else { - AVBufferRef *ref = av_buffer_ref(sd_src->buf); - sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref); - if (!sd_dst) { - av_buffer_unref(&ref); - wipe_side_data(dst); - return AVERROR(ENOMEM); - } - } - av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0); - } + if (ret = av_frame_copy_side_data(dst, src, force_copy) < 0) + return ret; ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); ret |= av_buffer_replace(&dst->private_ref, src->private_ref); diff --git a/libavutil/frame.h b/libavutil/frame.h index 9dfd5a886a..6dec040ce8 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -885,6 +885,18 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src); */ int av_frame_copy_props(AVFrame *dst, const AVFrame *src); +/** + * Copy only side-data from src to dst. + * + * @param dst a frame to which the side data should be copied. + * @param src a frame from which to copy the side data. + * @param force_copy determines whether to copy the actual data or only just + * create references to the buffers. + * + * @return >= 0 on success, a negative AVERROR on error. + */ +int av_frame_copy_side_data(AVFrame* dst, const AVFrame* src, int force_copy); + /** * Get the buffer reference a given data plane is stored in. *
Signed-off-by: softworkz <softworkz@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(-)