| Message ID | 20231214143911.39198-1-ffmpeg@haasn.xyz |
|---|---|
| State | Accepted |
| Commit | e687a8485425e3d03ad8fea35b17ac8827ea1b82 |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/formats: set audio fmt lists for vaf filters | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, 14 Dec 2023 15:39:11 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote: > From: Niklas Haas <git@haasn.dev> > > Currently, the logic inside the FF_FILTER_FORMATS_QUERY_FUNC branch > prevents this code from running in the event that we have a filter with > a single video input and a single audio output, as the resulting audio > output link will not have its channel counts / samplerates correctly > initialized to their default values, possibly triggering a segfault > downstream. > > An example of such a filter is vaf_spectrumsynth. Although this > particular filter already sets up the channel counts and samplerates as > part of the query function and therefore avoids triggering this bug, the > bug still exists in principle. (And importantly, sets a wrong precedent) > --- > libavfilter/formats.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index d1c97daf64..114886aeb2 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -808,16 +808,17 @@ int ff_default_query_formats(AVFilterContext *ctx) > /* Intended fallthrough */ > case FF_FILTER_FORMATS_PASSTHROUGH: > case FF_FILTER_FORMATS_QUERY_FUNC: > - type = ctx->nb_inputs ? ctx->inputs [0]->type : > - ctx->nb_outputs ? ctx->outputs[0]->type : AVMEDIA_TYPE_VIDEO; > - formats = ff_all_formats(type); > + type = AVMEDIA_TYPE_UNKNOWN; > + formats = ff_all_formats(ctx->nb_inputs ? ctx->inputs [0]->type : > + ctx->nb_outputs ? ctx->outputs[0]->type : > + AVMEDIA_TYPE_VIDEO); > break; > } > > ret = ff_set_common_formats(ctx, formats); > if (ret < 0) > return ret; > - if (type == AVMEDIA_TYPE_AUDIO) { > + if (type != AVMEDIA_TYPE_VIDEO) { > ret = ff_set_common_all_channel_counts(ctx); > if (ret < 0) > return ret; > -- > 2.43.0 > This patch fixes the underlying issue (alongside the corresponding adjustment to the conditional from `type == AVMEDIA_TYPE_VIDEO` to `type != AVMEDIA_TYPE_AUDIO` in patch 03/15).
On Thu, Dec 14, 2023 at 03:40:36PM +0100, Niklas Haas wrote: > On Thu, 14 Dec 2023 15:39:11 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote: > > From: Niklas Haas <git@haasn.dev> > > > > Currently, the logic inside the FF_FILTER_FORMATS_QUERY_FUNC branch > > prevents this code from running in the event that we have a filter with > > a single video input and a single audio output, as the resulting audio > > output link will not have its channel counts / samplerates correctly > > initialized to their default values, possibly triggering a segfault > > downstream. > > > > An example of such a filter is vaf_spectrumsynth. Although this > > particular filter already sets up the channel counts and samplerates as > > part of the query function and therefore avoids triggering this bug, the > > bug still exists in principle. (And importantly, sets a wrong precedent) > > --- > > libavfilter/formats.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > > index d1c97daf64..114886aeb2 100644 > > --- a/libavfilter/formats.c > > +++ b/libavfilter/formats.c > > @@ -808,16 +808,17 @@ int ff_default_query_formats(AVFilterContext *ctx) > > /* Intended fallthrough */ > > case FF_FILTER_FORMATS_PASSTHROUGH: > > case FF_FILTER_FORMATS_QUERY_FUNC: > > - type = ctx->nb_inputs ? ctx->inputs [0]->type : > > - ctx->nb_outputs ? ctx->outputs[0]->type : AVMEDIA_TYPE_VIDEO; > > - formats = ff_all_formats(type); > > + type = AVMEDIA_TYPE_UNKNOWN; > > + formats = ff_all_formats(ctx->nb_inputs ? ctx->inputs [0]->type : > > + ctx->nb_outputs ? ctx->outputs[0]->type : > > + AVMEDIA_TYPE_VIDEO); > > break; > > } > > > > ret = ff_set_common_formats(ctx, formats); > > if (ret < 0) > > return ret; > > - if (type == AVMEDIA_TYPE_AUDIO) { > > + if (type != AVMEDIA_TYPE_VIDEO) { > > ret = ff_set_common_all_channel_counts(ctx); > > if (ret < 0) > > return ret; > > -- > > 2.43.0 > > > > This patch fixes the underlying issue (alongside the corresponding > adjustment to the conditional from `type == AVMEDIA_TYPE_VIDEO` to `type > != AVMEDIA_TYPE_AUDIO` in patch 03/15). do you have some git branch i can test so i dont have to apply a patchset with adjustments and hope i have the exact same code ? thx [...]
Niklas Haas (12023-12-14): > From: Niklas Haas <git@haasn.dev> > > Currently, the logic inside the FF_FILTER_FORMATS_QUERY_FUNC branch > prevents this code from running in the event that we have a filter with > a single video input and a single audio output, as the resulting audio > output link will not have its channel counts / samplerates correctly > initialized to their default values, possibly triggering a segfault > downstream. > > An example of such a filter is vaf_spectrumsynth. Although this > particular filter already sets up the channel counts and samplerates as > part of the query function and therefore avoids triggering this bug, the > bug still exists in principle. (And importantly, sets a wrong precedent) > --- > libavfilter/formats.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) You are asking the framework to guess the audio format when it has NOTHING to base its guess on. This is a terrible idea. When a filter has no audio input but must produce audio, only the filter itself knows what kind of audio it will produce, and it should say so in query_formats(). What happens in spectrumsynth is what MUST happens. It is not a wrong precedent, it is the proper way of things. The worry that a filter will forget to do so and result in a segfault seems wrong to me: just testing the filter before committing would reveal it, not even regression testing is necessary. But if you really want, I would not object to a consistency check after the call to query_formats(). Regards,
diff --git a/libavfilter/formats.c b/libavfilter/formats.c index d1c97daf64..114886aeb2 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -808,16 +808,17 @@ int ff_default_query_formats(AVFilterContext *ctx) /* Intended fallthrough */ case FF_FILTER_FORMATS_PASSTHROUGH: case FF_FILTER_FORMATS_QUERY_FUNC: - type = ctx->nb_inputs ? ctx->inputs [0]->type : - ctx->nb_outputs ? ctx->outputs[0]->type : AVMEDIA_TYPE_VIDEO; - formats = ff_all_formats(type); + type = AVMEDIA_TYPE_UNKNOWN; + formats = ff_all_formats(ctx->nb_inputs ? ctx->inputs [0]->type : + ctx->nb_outputs ? ctx->outputs[0]->type : + AVMEDIA_TYPE_VIDEO); break; } ret = ff_set_common_formats(ctx, formats); if (ret < 0) return ret; - if (type == AVMEDIA_TYPE_AUDIO) { + if (type != AVMEDIA_TYPE_VIDEO) { ret = ff_set_common_all_channel_counts(ctx); if (ret < 0) return ret;
From: Niklas Haas <git@haasn.dev> Currently, the logic inside the FF_FILTER_FORMATS_QUERY_FUNC branch prevents this code from running in the event that we have a filter with a single video input and a single audio output, as the resulting audio output link will not have its channel counts / samplerates correctly initialized to their default values, possibly triggering a segfault downstream. An example of such a filter is vaf_spectrumsynth. Although this particular filter already sets up the channel counts and samplerates as part of the query function and therefore avoids triggering this bug, the bug still exists in principle. (And importantly, sets a wrong precedent) --- libavfilter/formats.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)