| Message ID | 20200813112027.371100-1-george@nsup.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] lavfi: check the validity of formats lists. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/configure | warning | Failed to apply patch |
Nicolas George: > Part of the code expect valid lists, in particular no duplicates. Which part of the code fails if there are duplicates? ff_merge_channel_layouts() can be easily fixed by adding breaks to all loops, so if it is only this, we could forgo checking as its cost is quadratic in the number of elements. > These tests allow to catch bugs in filters (unlikely but possible) > and to give a clear message when the error comes from the user > ((a)formats) or the application (buffersink). > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/avfiltergraph.c | 50 ++++++++++++++++++++++++++ > libavfilter/formats.c | 71 +++++++++++++++++++++++++++++++++++++ > libavfilter/formats.h | 28 +++++++++++++++ > 3 files changed, 149 insertions(+) > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > index c8a52b1f47..b9c7669417 100644 > --- a/libavfilter/avfiltergraph.c > +++ b/libavfilter/avfiltergraph.c > @@ -313,6 +313,53 @@ static void sanitize_channel_layouts(void *log, AVFilterChannelLayouts *l) > } > } > > +static int filter_link_check_formats(void *log, AVFilterLink *link, AVFilterFormatsConfig *cfg) > +{ > + int ret; > + > + switch (link->type) { > + > + case AVMEDIA_TYPE_VIDEO: > + if ((ret = ff_formats_check_pixel_formats(log, cfg->formats)) < 0) > + return ret; > + break; > + > + case AVMEDIA_TYPE_AUDIO: > + if ((ret = ff_formats_check_sample_formats(log, cfg->formats)) < 0 || > + (ret = ff_formats_check_sample_rates(log, cfg->samplerates)) < 0 || > + (ret = ff_formats_check_channel_layouts(log, cfg->channel_layouts)) < 0) > + return ret; > + break; > + > + default: > + av_assert0(!"reached"); > + } > + return 0; > +} > + > +/** > + * Check the validity of the formats / etc. lists set by query_formats(). > + * > + * In particular, check they do not contain any redundant element. > + */ > +static int filter_check_formats(AVFilterContext *ctx) > +{ > + unsigned i; > + int ret; > + > + for (i = 0; i < ctx->nb_inputs; i++) { > + ret = filter_link_check_formats(ctx, ctx->inputs[i], &ctx->inputs[i]->outcfg); > + if (ret < 0) > + return ret; > + } > + for (i = 0; i < ctx->nb_outputs; i++) { > + ret = filter_link_check_formats(ctx, ctx->outputs[i], &ctx->outputs[i]->incfg); > + if (ret < 0) > + return ret; > + } > + return 0; > +} > + > static int filter_query_formats(AVFilterContext *ctx) > { > int ret, i; > @@ -329,6 +376,9 @@ static int filter_query_formats(AVFilterContext *ctx) > ctx->name, av_err2str(ret)); > return ret; > } > + ret = filter_check_formats(ctx); > + if (ret < 0) > + return ret; > > for (i = 0; i < ctx->nb_inputs; i++) > sanitize_channel_layouts(ctx, ctx->inputs[i]->outcfg.channel_layouts); > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index 8843200f47..fb32874fb6 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -723,3 +723,74 @@ int ff_parse_channel_layout(int64_t *ret, int *nret, const char *arg, > > return 0; > } > + > +static int check_list(void *log, const char *name, const AVFilterFormats *fmts) > +{ > + unsigned i, j; > + > + if (!fmts) > + return 0; > + if (!fmts->nb_formats) { > + av_log(log, AV_LOG_ERROR, "Empty %s list\n", name); > + return AVERROR(EINVAL); > + } > + for (i = 0; i < fmts->nb_formats; i++) { > + for (j = i + 1; j < fmts->nb_formats; j++) { > + if (fmts->formats[i] == fmts->formats[j]) { > + av_log(log, AV_LOG_ERROR, "Duplicated %s\n", name); > + return AVERROR(EINVAL); > + } > + } > + } > + return 0; > +} > + > +int ff_formats_check_pixel_formats(void *log, const AVFilterFormats *fmts) > +{ > + return check_list(log, "pixel format", fmts); > +} > + > +int ff_formats_check_sample_formats(void *log, const AVFilterFormats *fmts) > +{ > + return check_list(log, "sample format", fmts); > +} > + > +int ff_formats_check_sample_rates(void *log, const AVFilterFormats *fmts) > +{ > + if (!fmts || !fmts->nb_formats) > + return 0; > + return check_list(log, "sample rate", fmts); > +} > + > +static int layouts_compatible(uint64_t a, uint64_t b) > +{ > + return a == b || > + (KNOWN(a) && !KNOWN(b) && av_get_channel_layout_nb_channels(a) == FF_LAYOUT2COUNT(b)) || > + (KNOWN(b) && !KNOWN(a) && av_get_channel_layout_nb_channels(b) == FF_LAYOUT2COUNT(a)); > +} > + > +int ff_formats_check_channel_layouts(void *log, const AVFilterChannelLayouts *fmts) > +{ > + unsigned i, j; > + > + if (!fmts) > + return 0; > + if (fmts->all_layouts < fmts->all_counts || > + (!fmts->all_layouts && !fmts->nb_channel_layouts)) { This check doesn't fit to the error message and it also makes the next check below dead code. > + av_log(log, AV_LOG_ERROR, "Inconsistent generic list\n"); > + return AVERROR(EINVAL); > + } > + if (!fmts->all_layouts && !fmts->nb_channel_layouts) { > + av_log(log, AV_LOG_ERROR, "Empty channel layout list\n"); > + return AVERROR(EINVAL); > + } > + for (i = 0; i < fmts->nb_channel_layouts; i++) { > + for (j = i + 1; j < fmts->nb_channel_layouts; j++) { > + if (layouts_compatible(fmts->channel_layouts[i], fmts->channel_layouts[j])) { > + av_log(log, AV_LOG_ERROR, "Duplicated or redundant channel layout\n"); > + return AVERROR(EINVAL); > + } > + } > + } > + return 0; > +} > diff --git a/libavfilter/formats.h b/libavfilter/formats.h > index ffe7a12d53..40fb2caa85 100644 > --- a/libavfilter/formats.h > +++ b/libavfilter/formats.h > @@ -295,4 +295,32 @@ void ff_formats_unref(AVFilterFormats **ref); > */ > void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref); > > +/** > + * Check that fmts is a valid pixel formats list. > + * > + * In particular, check for duplicates. > + */ > +int ff_formats_check_pixel_formats(void *log, const AVFilterFormats *fmts); > + > +/** > + * Check that fmts is a valid sample formats list. > + * > + * In particular, check for duplicates. > + */ > +int ff_formats_check_sample_formats(void *log, const AVFilterFormats *fmts); > + > +/** > + * Check that fmts is a valid sample rates list. > + * > + * In particular, check for duplicates. > + */ > +int ff_formats_check_sample_rates(void *log, const AVFilterFormats *fmts); > + > +/** > + * Check that fmts is a valid channel layouts list. > + * > + * In particular, check for duplicates. > + */ > +int ff_formats_check_channel_layouts(void *log, const AVFilterChannelLayouts *fmts); > + > #endif /* AVFILTER_FORMATS_H */ >
Andreas Rheinhardt (12020-08-13): > Which part of the code fails if there are duplicates? At least the merge loops. They can be protected, but the result is still invalid and should be avoided, preferably with useful diagnostic. > ff_merge_channel_layouts() can be easily fixed by adding breaks to all > loops, so if it is only this, we could forgo checking as its cost is > quadratic in the number of elements. The merges are quadratic too, this is just changing the constant. We could avoid all quadratic operations by sorting the list. If we decide to do so, these check functions are good candidates to be renamed "prepare" or "optimize" and do it. Regards,
On Thu, Aug 13, 2020 at 01:20:28PM +0200, Nicolas George wrote: > Part of the code expect valid lists, in particular no duplicates. > These tests allow to catch bugs in filters (unlikely but possible) > and to give a clear message when the error comes from the user > ((a)formats) or the application (buffersink). > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/avfiltergraph.c | 50 ++++++++++++++++++++++++++ > libavfilter/formats.c | 71 +++++++++++++++++++++++++++++++++++++ > libavfilter/formats.h | 28 +++++++++++++++ > 3 files changed, 149 insertions(+) this ffplay -nostats mm-short.mpg -t 1 -af volume=replaygain=track,ebur128 -nodisp -autoexit gets stuck with the patch, seems not depend on the input sample thx [...]
Michael Niedermayer (12020-08-14): > this > ffplay -nostats mm-short.mpg -t 1 -af volume=replaygain=track,ebur128 -nodisp -autoexit > > gets stuck with the patch, seems not depend on the input sample Thanks for the testing. It reveals two bugs in ffplay: - On buffersink, it sets both channel_layouts to { stereo } and channel_counts to { 2 }, which is redundant and should not be done, although it is not properly documented. - audio_thread() exits, but ffplay does not. I can both fix and mitigate the first problem, but I am not familiar enough with the workings of ffplay to fix the threading issue, and it should be fixed too, because this check is not the only possible cause for a failure at this point. Regards,
diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index c8a52b1f47..b9c7669417 100644 --- a/libavfilter/avfiltergraph.c +++ b/libavfilter/avfiltergraph.c @@ -313,6 +313,53 @@ static void sanitize_channel_layouts(void *log, AVFilterChannelLayouts *l) } } +static int filter_link_check_formats(void *log, AVFilterLink *link, AVFilterFormatsConfig *cfg) +{ + int ret; + + switch (link->type) { + + case AVMEDIA_TYPE_VIDEO: + if ((ret = ff_formats_check_pixel_formats(log, cfg->formats)) < 0) + return ret; + break; + + case AVMEDIA_TYPE_AUDIO: + if ((ret = ff_formats_check_sample_formats(log, cfg->formats)) < 0 || + (ret = ff_formats_check_sample_rates(log, cfg->samplerates)) < 0 || + (ret = ff_formats_check_channel_layouts(log, cfg->channel_layouts)) < 0) + return ret; + break; + + default: + av_assert0(!"reached"); + } + return 0; +} + +/** + * Check the validity of the formats / etc. lists set by query_formats(). + * + * In particular, check they do not contain any redundant element. + */ +static int filter_check_formats(AVFilterContext *ctx) +{ + unsigned i; + int ret; + + for (i = 0; i < ctx->nb_inputs; i++) { + ret = filter_link_check_formats(ctx, ctx->inputs[i], &ctx->inputs[i]->outcfg); + if (ret < 0) + return ret; + } + for (i = 0; i < ctx->nb_outputs; i++) { + ret = filter_link_check_formats(ctx, ctx->outputs[i], &ctx->outputs[i]->incfg); + if (ret < 0) + return ret; + } + return 0; +} + static int filter_query_formats(AVFilterContext *ctx) { int ret, i; @@ -329,6 +376,9 @@ static int filter_query_formats(AVFilterContext *ctx) ctx->name, av_err2str(ret)); return ret; } + ret = filter_check_formats(ctx); + if (ret < 0) + return ret; for (i = 0; i < ctx->nb_inputs; i++) sanitize_channel_layouts(ctx, ctx->inputs[i]->outcfg.channel_layouts); diff --git a/libavfilter/formats.c b/libavfilter/formats.c index 8843200f47..fb32874fb6 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -723,3 +723,74 @@ int ff_parse_channel_layout(int64_t *ret, int *nret, const char *arg, return 0; } + +static int check_list(void *log, const char *name, const AVFilterFormats *fmts) +{ + unsigned i, j; + + if (!fmts) + return 0; + if (!fmts->nb_formats) { + av_log(log, AV_LOG_ERROR, "Empty %s list\n", name); + return AVERROR(EINVAL); + } + for (i = 0; i < fmts->nb_formats; i++) { + for (j = i + 1; j < fmts->nb_formats; j++) { + if (fmts->formats[i] == fmts->formats[j]) { + av_log(log, AV_LOG_ERROR, "Duplicated %s\n", name); + return AVERROR(EINVAL); + } + } + } + return 0; +} + +int ff_formats_check_pixel_formats(void *log, const AVFilterFormats *fmts) +{ + return check_list(log, "pixel format", fmts); +} + +int ff_formats_check_sample_formats(void *log, const AVFilterFormats *fmts) +{ + return check_list(log, "sample format", fmts); +} + +int ff_formats_check_sample_rates(void *log, const AVFilterFormats *fmts) +{ + if (!fmts || !fmts->nb_formats) + return 0; + return check_list(log, "sample rate", fmts); +} + +static int layouts_compatible(uint64_t a, uint64_t b) +{ + return a == b || + (KNOWN(a) && !KNOWN(b) && av_get_channel_layout_nb_channels(a) == FF_LAYOUT2COUNT(b)) || + (KNOWN(b) && !KNOWN(a) && av_get_channel_layout_nb_channels(b) == FF_LAYOUT2COUNT(a)); +} + +int ff_formats_check_channel_layouts(void *log, const AVFilterChannelLayouts *fmts) +{ + unsigned i, j; + + if (!fmts) + return 0; + if (fmts->all_layouts < fmts->all_counts || + (!fmts->all_layouts && !fmts->nb_channel_layouts)) { + av_log(log, AV_LOG_ERROR, "Inconsistent generic list\n"); + return AVERROR(EINVAL); + } + if (!fmts->all_layouts && !fmts->nb_channel_layouts) { + av_log(log, AV_LOG_ERROR, "Empty channel layout list\n"); + return AVERROR(EINVAL); + } + for (i = 0; i < fmts->nb_channel_layouts; i++) { + for (j = i + 1; j < fmts->nb_channel_layouts; j++) { + if (layouts_compatible(fmts->channel_layouts[i], fmts->channel_layouts[j])) { + av_log(log, AV_LOG_ERROR, "Duplicated or redundant channel layout\n"); + return AVERROR(EINVAL); + } + } + } + return 0; +} diff --git a/libavfilter/formats.h b/libavfilter/formats.h index ffe7a12d53..40fb2caa85 100644 --- a/libavfilter/formats.h +++ b/libavfilter/formats.h @@ -295,4 +295,32 @@ void ff_formats_unref(AVFilterFormats **ref); */ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref); +/** + * Check that fmts is a valid pixel formats list. + * + * In particular, check for duplicates. + */ +int ff_formats_check_pixel_formats(void *log, const AVFilterFormats *fmts); + +/** + * Check that fmts is a valid sample formats list. + * + * In particular, check for duplicates. + */ +int ff_formats_check_sample_formats(void *log, const AVFilterFormats *fmts); + +/** + * Check that fmts is a valid sample rates list. + * + * In particular, check for duplicates. + */ +int ff_formats_check_sample_rates(void *log, const AVFilterFormats *fmts); + +/** + * Check that fmts is a valid channel layouts list. + * + * In particular, check for duplicates. + */ +int ff_formats_check_channel_layouts(void *log, const AVFilterChannelLayouts *fmts); + #endif /* AVFILTER_FORMATS_H */
Part of the code expect valid lists, in particular no duplicates. These tests allow to catch bugs in filters (unlikely but possible) and to give a clear message when the error comes from the user ((a)formats) or the application (buffersink). Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/avfiltergraph.c | 50 ++++++++++++++++++++++++++ libavfilter/formats.c | 71 +++++++++++++++++++++++++++++++++++++ libavfilter/formats.h | 28 +++++++++++++++ 3 files changed, 149 insertions(+)