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

[Stefano Sabatini]

On date Wednesday 2011-08-03 15:04:49 +0300, Mina Nagy Zaki encoded:
> 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?

-chlayouts may help, but completely unrelated, so no need to fix it
for getting this patch in. 

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

A private libavfilter function may be ok (where to exactly put them it
is secondary, just use common sense).

> 
> > > +                         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.

I need to think more about this, and I don't want this review to take
ages, so for the moment keep it.

> 
> [...]
> 
> > > +    }
> > > +
> > > +    // 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.

Fine, leave it then.

> 
> > 
> > > +    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.

Repetita iuvant, that was mostly to be consistent with the video
source, also having the info on a single line is helpful. Anyway this
is nitlevel 1, so feel free to skip the comment.
 
> [...]
> 
> 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?

As I mentioned on irc, I suppose asrc should work even when the
probing result differs from the initial stream content.
-- 
FFmpeg = Free and Formidable Martial Portentous Elegant Governor