| Message ID | 20200824113652.255648-3-george@nsup.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v3,1/4] lavfi: regroup formats lists in a single structure. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Nicolas George: > Part of the code expect valid lists, in particular no duplicates. expects > 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). > > If we decide to switch for a more efficient merging algorithm, switch to > possibly sorting the lists, these functions will be the prefered preferred > place for pre-processing, and can be renamed accordingly. > > 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(+) > > > Extended commit message. > > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > index 7a85d94971..296920a046 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 695d28ea8e..e575894e77 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -662,3 +662,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)) { As has already been said in my review of the first version: 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 a06e88722e..8378be4b9b 100644 > --- a/libavfilter/formats.h > +++ b/libavfilter/formats.h > @@ -291,4 +291,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-24): > expects > switch to > preferred Locally fixed. > > + if (fmts->all_layouts < fmts->all_counts || > > + (!fmts->all_layouts && !fmts->nb_channel_layouts)) { > > As has already been said in my review of the first version: > This check doesn't fit to the error message and it also makes the next > check below dead code. I missed the first review, sorry. The error message is correct: it is inconsistent to accept all counts without accepting all layouts. And the next test is not dead code: if all_counts and all_layouts are 0, and nb_channel_layouts is also 0, it is triggered. Or am I missing something? > > > + 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); > > + } Regards,
Nicolas George: > Andreas Rheinhardt (12020-08-24): >> expects > >> switch to > >> preferred > > Locally fixed. > >>> + if (fmts->all_layouts < fmts->all_counts || >>> + (!fmts->all_layouts && !fmts->nb_channel_layouts)) { >> >> As has already been said in my review of the first version: >> This check doesn't fit to the error message and it also makes the next >> check below dead code. > > I missed the first review, sorry. > > The error message is correct: it is inconsistent to accept all counts > without accepting all layouts. > > And the next test is not dead code: if all_counts and all_layouts are 0, > and nb_channel_layouts is also 0, it is triggered. > > Or am I missing something? > The above check already contains "|| (!fmts->all_layouts && !fmts->nb_channel_layouts)". So if everything is zero, you get the error for an inconsistent generic list. >> >>> + 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); >>> + } > > Regards, > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Andreas Rheinhardt (12020-08-24): > >>> + if (fmts->all_layouts < fmts->all_counts || > >>> + (!fmts->all_layouts && !fmts->nb_channel_layouts)) { > The above check already contains "|| (!fmts->all_layouts && > !fmts->nb_channel_layouts)". So if everything is zero, you get the error > for an inconsistent generic list. Oh, right, I had considered doing both tests at once, but then separated them for a different message, and forgot. I locally removed the second line above. Sorry. Regards,
On Mon, Aug 24, 2020 at 13:36:50 +0200, Nicolas George wrote: > + 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"); > + } "case" indentation is too large. Moritz
Moritz Barsnick (12020-08-28):
> "case" indentation is too large.
Thanks.
Fixed, rebased and series pushed.
Regards,
Nicolas George: > Moritz Barsnick (12020-08-28): >> "case" indentation is too large. > > Thanks. > > Fixed, rebased and series pushed. > > Regards, > > Seems like this patchset broke compilation with some old versions of GCC. See http://fate.ffmpeg.org/history.cgi?slot=x86_64-openbsd5.6-gcc4.2-conf2 and http://fate.ffmpeg.org/history.cgi?slot=x86_64-openbsd5.6-gcc4.2. I don't get why providing the typedef for AVFilterChannelLayouts in avfilter.h is bad, but doing the same for AVFilterFormats is not. - Andreas
diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index 7a85d94971..296920a046 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 695d28ea8e..e575894e77 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -662,3 +662,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 a06e88722e..8378be4b9b 100644 --- a/libavfilter/formats.h +++ b/libavfilter/formats.h @@ -291,4 +291,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). If we decide to switch for a more efficient merging algorithm, possibly sorting the lists, these functions will be the prefered place for pre-processing, and can be renamed accordingly. 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(+) Extended commit message.