| Message ID | 20200908211856.16290-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | dfd46e2d160afcb7e453d0e2394a6978cb447712 |
| Headers | show |
| Series | [FFmpeg-devel,01/25] avfilter/af_headphone: Don't use uninitialized buffer in log message | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Andreas Rheinhardt (12020-09-08): > Not providing any samples makes no sense at all. And if no samples > were provided for one of the HRIR streams, one would either run into > an av_assert1 in ff_inlink_consume_samples() or into a segfault in > take_samples() in avfilter.c. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavfilter/af_headphone.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavfilter/af_headphone.c b/libavfilter/af_headphone.c > index 1024ff57b1..f488e0e28d 100644 > --- a/libavfilter/af_headphone.c > +++ b/libavfilter/af_headphone.c > @@ -631,8 +631,14 @@ static int activate(AVFilterContext *ctx) > if ((ret = check_ir(ctx->inputs[i], i)) < 0) > return ret; > > - if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) > + if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) { > + if (!ff_inlink_queued_samples(ctx->inputs[i])) { > + av_log(ctx, AV_LOG_ERROR, "No samples provided for " > + "HRIR stream %d.\n", i - 1); > + return AVERROR_INVALIDDATA; > + } > s->in[i].eof = 1; > + } No, this is bogus. A filter should not call ff_outlink_get_status() on its input, that is breaking the abstraction provided by the API. In fact, everything in `git grep 'ff_outlink_get_status.*input'` is a bug. > } > > for (i = 1; i < s->nb_inputs; i++) { Regards,
Nicolas George: > Andreas Rheinhardt (12020-09-08): >> Not providing any samples makes no sense at all. And if no samples >> were provided for one of the HRIR streams, one would either run into >> an av_assert1 in ff_inlink_consume_samples() or into a segfault in >> take_samples() in avfilter.c. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavfilter/af_headphone.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/libavfilter/af_headphone.c b/libavfilter/af_headphone.c >> index 1024ff57b1..f488e0e28d 100644 >> --- a/libavfilter/af_headphone.c >> +++ b/libavfilter/af_headphone.c >> @@ -631,8 +631,14 @@ static int activate(AVFilterContext *ctx) >> if ((ret = check_ir(ctx->inputs[i], i)) < 0) >> return ret; >> >> - if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) >> + if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) { >> + if (!ff_inlink_queued_samples(ctx->inputs[i])) { >> + av_log(ctx, AV_LOG_ERROR, "No samples provided for " >> + "HRIR stream %d.\n", i - 1); >> + return AVERROR_INVALIDDATA; >> + } >> s->in[i].eof = 1; >> + } > > No, this is bogus. A filter should not call ff_outlink_get_status() on > its input, that is breaking the abstraction provided by the API. In > fact, everything in `git grep 'ff_outlink_get_status.*input'` is a bug. > How should one then check whether an input is finished? Check for ff_inlink_check_available_samples(ctx->inputs[i], ff_inlink_queued_samples(ctx->inputs[i]) + 1) being 1? - Andreas
Andreas Rheinhardt (12020-09-09): > How should one then check whether an input is finished? Check for > ff_inlink_check_available_samples(ctx->inputs[i], > ff_inlink_queued_samples(ctx->inputs[i]) + 1) being 1? No, these tell you what can be obtained immediately. The test for a finished input is ff_inlink_acknowledge_status(). The need for this test is probably the sign of a flawed logic: the filter is trying to anticipate things when it should just be organized to do as necessary. Regards,
Nicolas George: > Andreas Rheinhardt (12020-09-09): >> How should one then check whether an input is finished? Check for >> ff_inlink_check_available_samples(ctx->inputs[i], >> ff_inlink_queued_samples(ctx->inputs[i]) + 1) being 1? > > No, these tell you what can be obtained immediately. > The documentation of ff_inlink_check_available_samples() contains the note "on EOF and error, min becomes 1". Which means that ff_inlink_check_available_samples() can be used to peek into the error code of this inlink by setting min so high that the not enough samples can be available. You seem to want to forbid it absolutely that a filter knows whether there will be more data forthcoming after the data that is already queued, so I am surprised that you have not also closed this loophole in your patch just now. > The test for a finished input is ff_inlink_acknowledge_status(). > > The need for this test is probably the sign of a flawed logic: the > filter is trying to anticipate things when it should just be organized > to do as necessary. > For this filter to work it is necessary to know whether all inputs except input 0 are finished; it can only start when they are finished. - Andreas
Andreas Rheinhardt (12020-09-09): > The documentation of ff_inlink_check_available_samples() contains the > note "on EOF and error, min becomes 1". Which means that > ff_inlink_check_available_samples() can be used to peek into the error > code of this inlink by setting min so high that the not enough samples > can be available. You seem to want to forbid it absolutely that a filter > knows whether there will be more data forthcoming after the data that is > already queued, so I am surprised that you have not also closed this > loophole in your patch just now. It can be used to do that, but a filter that uses it is doing something very wrong. But in the meantime, ff_inlink_check_available_samples() is doing exactly what it is supposed to do, i.e. predict that ff_inlink_consume_samples() will succeed. I do not want to "close loopholes" because we are not in an adversarial situation. > For this filter to work it is necessary to know whether all inputs > except input 0 are finished; it can only start when they are finished. Then it is easy: the filter should consume the samples on its input, if there are none try to acknowledge the status change: if the status has changed, the input is finished. Checking beforehand is quite unnecessary. Regards,
diff --git a/libavfilter/af_headphone.c b/libavfilter/af_headphone.c index 1024ff57b1..f488e0e28d 100644 --- a/libavfilter/af_headphone.c +++ b/libavfilter/af_headphone.c @@ -631,8 +631,14 @@ static int activate(AVFilterContext *ctx) if ((ret = check_ir(ctx->inputs[i], i)) < 0) return ret; - if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) + if (ff_outlink_get_status(ctx->inputs[i]) == AVERROR_EOF) { + if (!ff_inlink_queued_samples(ctx->inputs[i])) { + av_log(ctx, AV_LOG_ERROR, "No samples provided for " + "HRIR stream %d.\n", i - 1); + return AVERROR_INVALIDDATA; + } s->in[i].eof = 1; + } } for (i = 1; i < s->nb_inputs; i++) {
Not providing any samples makes no sense at all. And if no samples were provided for one of the HRIR streams, one would either run into an av_assert1 in ff_inlink_consume_samples() or into a segfault in take_samples() in avfilter.c. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/af_headphone.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)