[FFmpeg-devel] [ffmpeg-devel] [PATCH 1/4] lavfi: add asrc_abuffer - audio source buffer filter

[Mina Nagy Zaki]

On Tue, Aug 02, 2011 at 11:48:47AM +0200, Stefano Sabatini wrote:
> On date Monday 2011-08-01 12:36:26 +0300, Mina Nagy Zaki encoded:
> 
> > From d464fc54179c9d96b74ea6f4e90d9fecc5a78b88 Mon Sep 17 00:00:00 2001
> > From: Mina Nagy Zaki <mnzaki at gmail.com>
> > Date: Mon, 1 Aug 2011 11:33:26 +0300
> > Subject: [PATCH 02/11] lavfi: add asrc_abuffer - audio source buffer filter
> > 
> > Originally based on code by Stefano Sabatini and S. N. Hemanth
> > ---
> >  doc/filters.texi           |   48 ++++++
> >  libavfilter/Makefile       |    2 +
> >  libavfilter/allfilters.c   |    1 +
> >  libavfilter/asrc_abuffer.c |  377 ++++++++++++++++++++++++++++++++++++++++++++
> >  libavfilter/asrc_abuffer.h |   81 ++++++++++
> >  5 files changed, 509 insertions(+), 0 deletions(-)
> >  create mode 100644 libavfilter/asrc_abuffer.c
> >  create mode 100644 libavfilter/asrc_abuffer.h
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 87f6c6a..dd1a1ee 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -128,6 +128,54 @@ Pass the audio source unchanged to the output.
> >  
> >  Below is a description of the currently available audio sources.
> >  
> > + at section abuffer
> > +
> > +Buffer audio frames, and make them available to the filter chain.
> > +
> > +This source is mainly intended for a programmatic use, in particular
> > +through the interface defined in @file{libavfilter/asrc_abuffer.h}.
> > +
> > +It accepts the following mandatory parameters:
> > + at var{sample_rate}:@var{sample_fmt}:@var{channel_layout}:@var{packing}
> > +
> > + at table @option
> > +
> > + at item sample_rate
> > +
> > +The sample rate of the incoming audio buffers.
> > +
> > + at item sample_fmt
> > +
> > +The sample format of the incoming audio buffers.
> > +Either a sample format name or its corresponging integer representation from
> > +the enum AVSampleFormat in @file{libavutil/samplefmt.h}
> > +
> > + at item channel_layout
> > +
> > +The channel layout of the incoming audio buffers.
> > +Either a channel layout name from channel_layout_map in
> > + at file{libavutil/audioconvert.c} or its corresponding integer representation
> > +from the AV_CH_LAYOUT_* macros in @file{libavutil/audioconvert.h}
> 
> Note: somehow unpretty (we should list them in the manual and/or list
> through some switch).
> 

This is a problem with abuffer, aconvert, and aformat. I think I should add a
section listing all formats and just refer to it?

Also the argument parsing code is exactly the same for all of them, can this go
into libavutil/parseutils.{c,h} ?

> > +                         const char *filt_name, const char *args)
> > +{
> > +    int ret;
> > +
> > +    if ((ret = avfilter_open(filter, avfilter_get_by_name(filt_name),
> > +                            "audio filter")) < 0)
> > +        return ret;
> > +
> > +    link->src->outputs[0] = NULL;
> > +    if ((ret = avfilter_link(link->src, 0, *filter, 0)) < 0) {
> > +        link->src->outputs[0] = link;
> > +        return ret;
> > +    }
> > +
> > +    link->src             = *filter;
> > +    link->srcpad          = &((*filter)->output_pads[0]);
> > +    (*filter)->outputs[0] = link;
> 
> avfilter_insert_filter()?

avfilter_insert_filter() inserts the filter *after* the specified link, but I
need to insert it *before* the specified (old) link which will retain it's
original formats, so that the newly established link (which is the input to the
new filter) can be configured with the new formats for the conversion to kick
in.

[...]

> > +    }
> > +
> > +    // Normalize input
> > +
> 
> > +    link = ctx->outputs[0];
> 
> this can be done in the var init, save a line

I reuse link all over the place so I kept the assignment above each block where
it is reused to make things easier to follow.

> 
> > +    if (!link->sample_rate)
> > +        abuffer->sample_rate = link->sample_rate = samplesref->audio->sample_rate;
> 
> why this check? sample_rate should be always set in the link, or some
> configuration error happened.
> 

Remnant of something else, removed.

> > +    if (samplesref->audio->sample_rate != link->sample_rate) {
> > +        av_log(ctx, AV_LOG_INFO, "Audio sample rate changed, normalizing\n");
> 
> For debugging purposes, I ask you to add indications of src and dst
> sample rates, that's extremely helpful when debugging such things (or
> spotting a problem from reading an user report).

Both aconvert and aresample will immediately log the conversions they have been
set up to make, so it's already taken care of.

[...]

No patch posted. Waiting to resolve the doc issue and also the sample rate
probing glitch. The glitch can be avoided if I discard the sample_rate argument
and take the sample rate from the very first buffer added. What do you think?