diff mbox series

[FFmpeg-devel] lavfi: flag incorrect uses of ff_outlink APIs.

Message ID 20200909145835.1456596-1-george@nsup.org
State New
Headers show
Series [FFmpeg-devel] lavfi: flag incorrect uses of ff_outlink APIs. | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Nicolas George Sept. 9, 2020, 2:58 p.m. UTC
The link API provides an abstraction, where a filter sees on an
input a stream of frames or samples terminated by a status change.
Calling ff_outlink functions on an input bypasses this abstraction
to peek inside implementation details; it may work now, but it can
break at any point if the implementation is changed or if new
conditions are triggered.

I do not know how to fix these filters, but adding these comments
will avoid new code to imitate the pattern and require fixing too.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/af_afade.c        | 2 ++
 libavfilter/af_afir.c         | 3 +++
 libavfilter/af_headphone.c    | 1 +
 libavfilter/f_interleave.c    | 3 +++
 libavfilter/vf_framepack.c    | 2 ++
 libavfilter/vf_xfade.c        | 2 ++
 libavfilter/vf_xfade_opencl.c | 2 ++
 7 files changed, 15 insertions(+)

Comments

Paul B Mahol Sept. 9, 2020, 4:26 p.m. UTC | #1
On Wed, Sep 09, 2020 at 04:58:35PM +0200, Nicolas George wrote:
> The link API provides an abstraction, where a filter sees on an
> input a stream of frames or samples terminated by a status change.
> Calling ff_outlink functions on an input bypasses this abstraction
> to peek inside implementation details; it may work now, but it can
> break at any point if the implementation is changed or if new
> conditions are triggered.
> 
> I do not know how to fix these filters, but adding these comments
> will avoid new code to imitate the pattern and require fixing too.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/af_afade.c        | 2 ++
>  libavfilter/af_afir.c         | 3 +++
>  libavfilter/af_headphone.c    | 1 +
>  libavfilter/f_interleave.c    | 3 +++
>  libavfilter/vf_framepack.c    | 2 ++
>  libavfilter/vf_xfade.c        | 2 ++
>  libavfilter/vf_xfade_opencl.c | 2 ++
>  7 files changed, 15 insertions(+)
> 

Patch rejected. There is no need to fix what is not broken.
Nicolas George Sept. 9, 2020, 4:44 p.m. UTC | #2
Paul B Mahol (12020-09-09):
> Patch rejected. There is no need to fix what is not broken.

Rejection rejected. I have explained exactly why it is broken, you have
not addressed it at all.
Paul B Mahol Sept. 9, 2020, 5:20 p.m. UTC | #3
On Wed, Sep 09, 2020 at 06:44:44PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-09-09):
> > Patch rejected. There is no need to fix what is not broken.
> 
> Rejection rejected. I have explained exactly why it is broken, you have
> not addressed it at all.

Patch is incomplete, incorrect and defamatory thus rejected. Dealt with it.

Rubberband filter need to know that last samples it feed to librubberband are actually
last ones. What solution you propose to dealt with it?

Until your provide real solutions to real problems patch is rejected.
Nicolas George Sept. 9, 2020, 5:24 p.m. UTC | #4
Paul B Mahol (12020-09-09):
> Patch is incomplete, incorrect and defamatory thus rejected. Dealt with it.

Patch is as complete as I can make it, and it is accurate, and therefore
your accusation of "defamatory" are the defamatory parts in this
discussion.

> Rubberband filter need to know that last samples it feed to librubberband are actually
> last ones. What solution you propose to dealt with it?

I know how to do it, it is rather easy, but I have no obligation to help
you fix your code after the way you treated me. You made your bed, now
lie in it.

I can allow some time to fix this, but if it is not done in a timely
manner, I will push these comments to avoid the bugs spreading farther.

And any new code misusing the API will be rejected on sight. It will be
reverted on sight if pushed without review, like all the commits that
introduce this bugs do.

This is all I have to say to you on the matter.
Paul B Mahol Sept. 9, 2020, 5:47 p.m. UTC | #5
On Wed, Sep 09, 2020 at 07:24:43PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-09-09):
> > Patch is incomplete, incorrect and defamatory thus rejected. Dealt with it.
> 
> Patch is as complete as I can make it, and it is accurate, and therefore
> your accusation of "defamatory" are the defamatory parts in this
> discussion.
> 
> > Rubberband filter need to know that last samples it feed to librubberband are actually
> > last ones. What solution you propose to dealt with it?
> 
> I know how to do it, it is rather easy, but I have no obligation to help
> you fix your code after the way you treated me. You made your bed, now
> lie in it.
> 
> I can allow some time to fix this, but if it is not done in a timely
> manner, I will push these comments to avoid the bugs spreading farther.
> 
> And any new code misusing the API will be rejected on sight. It will be
> reverted on sight if pushed without review, like all the commits that
> introduce this bugs do.
> 
> This is all I have to say to you on the matter.

You are extremely evil person.

Also your patch is incomplete if it can be considered correct at all.

PATCH REJECTED!
diff mbox series

Patch

diff --git a/libavfilter/af_afade.c b/libavfilter/af_afade.c
index 4edfd27a3c..fc549522a3 100644
--- a/libavfilter/af_afade.c
+++ b/libavfilter/af_afade.c
@@ -555,9 +555,11 @@  static int activate(AVFilterContext *ctx)
             return ff_filter_frame(outlink, out);
         }
     } else if (ff_outlink_frame_wanted(ctx->outputs[0])) {
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (!s->cf0_eof && ff_outlink_get_status(ctx->inputs[0])) {
             s->cf0_eof = 1;
         }
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (ff_outlink_get_status(ctx->inputs[1])) {
             ff_outlink_set_status(ctx->outputs[0], AVERROR_EOF, AV_NOPTS_VALUE);
             return 0;
diff --git a/libavfilter/af_afir.c b/libavfilter/af_afir.c
index ca90a158a1..56725064b5 100644
--- a/libavfilter/af_afir.c
+++ b/libavfilter/af_afir.c
@@ -642,6 +642,7 @@  static int activate(AVFilterContext *ctx)
         if (ret < 0)
             return ret;
 
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (ff_outlink_get_status(ctx->inputs[1 + s->selir]) == AVERROR_EOF)
             s->eof_coeffs[s->selir] = 1;
 
@@ -698,6 +699,7 @@  static int activate(AVFilterContext *ctx)
     }
 
     if (ff_outlink_frame_wanted(ctx->outputs[0]) &&
+        /* FIXME calling ff_outlink_something on an input is invalid */
         !ff_outlink_get_status(ctx->inputs[0])) {
         ff_inlink_request_frame(ctx->inputs[0]);
         return 0;
@@ -705,6 +707,7 @@  static int activate(AVFilterContext *ctx)
 
     if (s->response &&
         ff_outlink_frame_wanted(ctx->outputs[1]) &&
+        /* FIXME calling ff_outlink_something on an input is invalid */
         !ff_outlink_get_status(ctx->inputs[0])) {
         ff_inlink_request_frame(ctx->inputs[0]);
         return 0;
diff --git a/libavfilter/af_headphone.c b/libavfilter/af_headphone.c
index edf8e773d7..317e631de5 100644
--- a/libavfilter/af_headphone.c
+++ b/libavfilter/af_headphone.c
@@ -520,6 +520,7 @@  static int activate(AVFilterContext *ctx)
             if ((ret = check_ir(input, i)) < 0)
                 return ret;
 
+            /* FIXME calling ff_outlink_something on an input is invalid */
             if (ff_outlink_get_status(input) == AVERROR_EOF) {
                 if (!ff_inlink_queued_samples(input)) {
                     av_log(ctx, AV_LOG_ERROR, "No samples provided for "
diff --git a/libavfilter/f_interleave.c b/libavfilter/f_interleave.c
index a18bbe79b3..638252f28e 100644
--- a/libavfilter/f_interleave.c
+++ b/libavfilter/f_interleave.c
@@ -116,10 +116,12 @@  static int activate(AVFilterContext *ctx)
     }
 
     for (i = 0; i < ctx->nb_inputs; i++)
+        /* FIXME calling ff_outlink_something on an input is invalid */
         nb_eofs += !!ff_outlink_get_status(ctx->inputs[i]);
 
     if ((nb_eofs > 0 && s->duration_mode == DURATION_SHORTEST) ||
         (nb_eofs == ctx->nb_inputs && s->duration_mode == DURATION_LONGEST) ||
+        /* FIXME calling ff_outlink_something on an input is invalid */
         (ff_outlink_get_status(ctx->inputs[0]) && s->duration_mode == DURATION_FIRST)) {
         ff_outlink_set_status(outlink, AVERROR_EOF, s->pts);
         return 0;
@@ -129,6 +131,7 @@  static int activate(AVFilterContext *ctx)
         if (ff_inlink_queued_frames(ctx->inputs[i]))
             continue;
         if (ff_outlink_frame_wanted(outlink) &&
+            /* FIXME calling ff_outlink_something on an input is invalid */
             !ff_outlink_get_status(ctx->inputs[i])) {
             ff_inlink_request_frame(ctx->inputs[i]);
             return 0;
diff --git a/libavfilter/vf_framepack.c b/libavfilter/vf_framepack.c
index b5d877ca99..e3f0b6b820 100644
--- a/libavfilter/vf_framepack.c
+++ b/libavfilter/vf_framepack.c
@@ -354,6 +354,7 @@  static int activate(AVFilterContext *ctx)
     FF_FILTER_FORWARD_STATUS(ctx->inputs[1], outlink);
 
     if (ff_outlink_frame_wanted(ctx->outputs[0]) &&
+        /* FIXME calling ff_outlink_something on an input is invalid */
         !ff_outlink_get_status(ctx->inputs[0]) &&
         !s->input_views[0]) {
         ff_inlink_request_frame(ctx->inputs[0]);
@@ -361,6 +362,7 @@  static int activate(AVFilterContext *ctx)
     }
 
     if (ff_outlink_frame_wanted(ctx->outputs[0]) &&
+        /* FIXME calling ff_outlink_something on an input is invalid */
         !ff_outlink_get_status(ctx->inputs[1]) &&
         !s->input_views[1]) {
         ff_inlink_request_frame(ctx->inputs[1]);
diff --git a/libavfilter/vf_xfade.c b/libavfilter/vf_xfade.c
index 7412709587..0eebfe457d 100644
--- a/libavfilter/vf_xfade.c
+++ b/libavfilter/vf_xfade.c
@@ -1824,10 +1824,12 @@  static int xfade_activate(AVFilterContext *ctx)
     }
 
     if (ff_outlink_frame_wanted(outlink)) {
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (!s->eof[0] && ff_outlink_get_status(ctx->inputs[0])) {
             s->eof[0] = 1;
             s->xfade_is_over = 1;
         }
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (!s->eof[1] && ff_outlink_get_status(ctx->inputs[1])) {
             s->eof[1] = 1;
         }
diff --git a/libavfilter/vf_xfade_opencl.c b/libavfilter/vf_xfade_opencl.c
index 4736043147..18f0ce29d8 100644
--- a/libavfilter/vf_xfade_opencl.c
+++ b/libavfilter/vf_xfade_opencl.c
@@ -323,10 +323,12 @@  static int xfade_opencl_activate(AVFilterContext *avctx)
     }
 
     if (ff_outlink_frame_wanted(outlink)) {
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (!ctx->eof[0] && ff_outlink_get_status(avctx->inputs[0])) {
             ctx->eof[0] = 1;
             ctx->xfade_is_over = 1;
         }
+        /* FIXME calling ff_outlink_something on an input is invalid */
         if (!ctx->eof[1] && ff_outlink_get_status(avctx->inputs[1])) {
             ctx->eof[1] = 1;
         }