[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