| Message ID | 20200909145835.1456596-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavfi: flag incorrect uses of ff_outlink APIs. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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.
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.
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.
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.
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 --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; }
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(+)