diff mbox series

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

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
Related show

Checks

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

Commit Message

Soft Works Nov. 30, 2021, 2:22 p.m. UTC
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(-)

Comments

Anton Khirnov Dec. 1, 2021, 10:54 a.m. UTC | #1
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.
Soft Works Dec. 3, 2021, 7:55 a.m. UTC | #2
> -----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
Anton Khirnov Dec. 3, 2021, 8:23 a.m. UTC | #3
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.
Soft Works Dec. 3, 2021, 8:51 a.m. UTC | #4
> -----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
Anton Khirnov Dec. 7, 2021, 8:07 a.m. UTC | #5
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.
Soft Works Dec. 7, 2021, 9:03 a.m. UTC | #6
> -----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
Anton Khirnov Dec. 7, 2021, 11:27 a.m. UTC | #7
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 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;
     }
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.
  *