[FFmpeg-devel] [PATCH 3/3] lavfi: support multiple rounds of format negotiation.
Stefano Sabatini
stefasab at gmail.com
Sat Mar 23 01:29:59 CET 2013
On date Thursday 2013-02-21 20:44:15 +0100, Nicolas George encoded:
> Remove the temporary hack for amerge and replace it with a
> generic solution.
>
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
> doc/filter_design.txt | 5 +++
> libavfilter/avfiltergraph.c | 103 ++++++++++++++++++++++++++++++++-----------
> 2 files changed, 83 insertions(+), 25 deletions(-)
>
>
> The logic is this: the query_formats() function will fail with
> AVERROR(EINVAL) if some filters could not be configured yet but progress was
> made, and the calling site will loop on that condition. It is as simple as
> that, all the noise in the diff is mostly connected to making sure that
> filters are not configured twice (most of them would cause crashes).
>
> I believe that with this feature, overlay can peek at its inputs supported
> formats (and possibly output too) and decide automatically between the
> yuv420/yuv444/rgb mode.
>
> Note that that feature must be used parsimoniously, because the mode filters
> use it, the more likely it is that the process will fail because of a
> circular dependency.
>
>
> diff --git a/doc/filter_design.txt b/doc/filter_design.txt
> index 772ca9d..2f9e57d 100644
> --- a/doc/filter_design.txt
> +++ b/doc/filter_design.txt
> @@ -29,6 +29,11 @@ Format negotiation
> same format amongst a supported list, all it has to do is use a reference
> to the same list of formats.
>
> + query_formats can leave some formats unset and return AVERROR(EAGAIN) to
> + cause the negotiation mechanism to try again later. That can be used by
> + filters with complex requirements to use the format negotiated on one link
> + to set the formats supported on another.
> +
>
> Buffer references ownership and permissions
> ===========================================
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index 0bc8464..600255c 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -24,6 +24,7 @@
> #include <string.h>
>
> #include "libavutil/avassert.h"
> +#include "libavutil/bprint.h"
> #include "libavutil/channel_layout.h"
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> @@ -211,8 +212,9 @@ static int filter_query_formats(AVFilterContext *ctx)
> AVMEDIA_TYPE_VIDEO;
>
> if ((ret = ctx->filter->query_formats(ctx)) < 0) {
> - av_log(ctx, AV_LOG_ERROR, "Query format failed for '%s': %s\n",
> - ctx->name, av_err2str(ret));
> + if (ret != AVERROR(EAGAIN))
> + av_log(ctx, AV_LOG_ERROR, "Query format failed for '%s': %s\n",
> + ctx->name, av_err2str(ret));
> return ret;
> }
>
> @@ -238,27 +240,46 @@ static int filter_query_formats(AVFilterContext *ctx)
> return 0;
> }
>
> +static int formats_queried(AVFilterContext *f)
nit++: more meaningful variable (e.g. "ctx" or even "filter_ctx"
should be ok)
Also sorry to nit but the name is a bit confusing, since on a filter
context you can call query_formats() (and thus having "formats
queried"), but the formats may not be set. So a possibly more
descriptive name could be:
formats_selected()
or
formats_chosen()
> +{
> + int i;
> +
> + for (i = 0; i < f->nb_inputs; i++) {
> + if (!f->inputs[i]->out_formats)
> + return 0;
> + if (f->inputs[i]->type == AVMEDIA_TYPE_AUDIO &&
> + !(f->inputs[i]->out_samplerates &&
> + f->inputs[i]->out_channel_layouts))
> + return 0;
> + }
> + for (i = 0; i < f->nb_outputs; i++) {
> + if (!f->outputs[i]->in_formats)
> + return 0;
> + if (f->outputs[i]->type == AVMEDIA_TYPE_AUDIO &&
> + !(f->outputs[i]->in_samplerates &&
> + f->outputs[i]->in_channel_layouts))
> + return 0;
> + }
> + return 1;
> +}
> +
> static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
> {
> int i, j, ret;
> int scaler_count = 0, resampler_count = 0;
> + int merge_done = 0, merge_already = 0, merge_delayed = 0;
merge already is a bit confusing
>
> - for (j = 0; j < 2; j++) {
> - /* ask all the sub-filters for their supported media formats */
> for (i = 0; i < graph->filter_count; i++) {
> - /* Call query_formats on sources first.
> - This is a temporary workaround for amerge,
> - until format renegociation is implemented. */
> - if (!graph->filters[i]->nb_inputs == j)
> + AVFilterContext *f = graph->filters[i];
> + if (formats_queried(f))
> continue;
> - if (graph->filters[i]->filter->query_formats)
> - ret = filter_query_formats(graph->filters[i]);
> + if (f->filter->query_formats)
> + ret = filter_query_formats(f);
> else
> - ret = ff_default_query_formats(graph->filters[i]);
> - if (ret < 0)
> + ret = ff_default_query_formats(f);
> + if (ret < 0 && ret != AVERROR(EAGAIN))
> return ret;
> }
> - }
>
> /* go through and merge as many format lists as possible */
> for (i = 0; i < graph->filter_count; i++) {
> @@ -271,20 +292,32 @@ static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
> if (!link)
> continue;
>
> - if (link->in_formats != link->out_formats &&
> - !ff_merge_formats(link->in_formats,
> - link->out_formats))
> - convert_needed = 1;
> +#define MERGE_DISPATCH(field, statement) \
> + if (!(link->in_ ## field && link->out_ ## field)) { \
> + merge_delayed++; \
> + } else if (link->in_ ## field == link->out_ ## field) { \
> + merge_already++; \
> + } else { \
> + merge_done++; \
> + statement \
> + }
> + MERGE_DISPATCH(formats,
> + if (!ff_merge_formats(link->in_formats, link->out_formats))
> + convert_needed = 1;
> + )
> if (link->type == AVMEDIA_TYPE_AUDIO) {
> - if (link->in_channel_layouts != link->out_channel_layouts &&
> - !ff_merge_channel_layouts(link->in_channel_layouts,
> + MERGE_DISPATCH(channel_layouts,
> + if (!ff_merge_channel_layouts(link->in_channel_layouts,
> link->out_channel_layouts))
weird indent
> - convert_needed = 1;
> - if (link->in_samplerates != link->out_samplerates &&
> - !ff_merge_samplerates(link->in_samplerates,
> - link->out_samplerates))
> - convert_needed = 1;
> + convert_needed = 1;
> + )
> + MERGE_DISPATCH(samplerates,
> + if (!ff_merge_samplerates(link->in_samplerates,
> + link->out_samplerates))
> + convert_needed = 1;
> + )
> }
> +#undef MERGE_DISPATCH
>
> if (convert_needed) {
> AVFilterContext *convert;
> @@ -363,6 +396,24 @@ static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
> }
> }
>
> + av_log(graph, AV_LOG_DEBUG,
> + "query_formats: %d done, %d already, %d delayed\n",
> + merge_done, merge_already, merge_delayed);
> + if (merge_delayed) {
> + AVBPrint bp;
> +
> + if (merge_done)
> + return AVERROR(EAGAIN);
> + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
Unrelated note: since I see this is a recurring pattern, I wonder if
we could have a macro to init a bprint buffer, something like:
AVBPrint bp = AV_BPRINT_INIT(0, AV_BPRINT_SIZE_AUTOMATIC);
and save a few lines.
> + for (i = 0; i < graph->filter_count; i++)
> + if (!formats_queried(graph->filters[i]))
> + av_bprintf(&bp, "%s%s", bp.len ? ", " : "",
> + graph->filters[i]->name);
> + av_log(graph, AV_LOG_ERROR,
> + "The following filters could not choose their formats: %s\n"
> + "Consider inserting the (a)format filter.\n", bp.str);
where? Adding a few more indications will save some questions later.
> + return AVERROR(EIO);
EINVAL may be more adequate
> + }
> return 0;
> }
>
> @@ -826,7 +877,9 @@ static int graph_config_formats(AVFilterGraph *graph, AVClass *log_ctx)
> int ret;
>
> /* find supported formats from sub-filters, and merge along links */
> - if ((ret = query_formats(graph, log_ctx)) < 0)
> + while ((ret = query_formats(graph, log_ctx)) == AVERROR(EAGAIN))
> + av_log(graph, AV_LOG_DEBUG, "query_formats not finished\n");
> + if (ret < 0)
> return ret;
I'll try to have another look tomorrow, sorry for the long delay.
--
FFmpeg = Fanciful and Fabulous Mind-dumbing Proud Ecstatic Gargoyle
More information about the ffmpeg-devel
mailing list