[FFmpeg-devel] [ffmpeg-devel] [PATCH 3/4] lavfi: add audio convert filter

[Stefano Sabatini]

On date Monday 2011-08-01 11:47:04 +0300, Mina Nagy Zaki encoded:
> On Wed, Jul 27, 2011 at 03:49:05PM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2011-07-27 01:05:19 +0300, Mina Nagy Zaki encoded:
> > Missing docs (can be added later when we approach to the final
> > revision).
> 
> Regarding docs, they will look almost exactly like 'aformat'. For audio aformat
> is pretty much useless except for testing..
> 
> [...]
>  
> > > +    char sample_fmt_str[8] = "", chlayout_str[32] = "";
> > 
> > 32 is not enough for the layout
> 
> Why is 32 not enough?? The longest layout name is about 15 chars.

Forget my comment, indeed I believed av_get_channel_layout() was
accepting a more general chlayout specification.

> > > +
> > > +    // rematrixing
> > > +    if (aconvert->convert_chlayout) {
> > > +        aconvert->mix_samplesref =
> > > +                avfilter_get_audio_buffer(outlink,
> > > +                                          AV_PERM_WRITE | AV_PERM_REUSE2,
> > > +                                          inlink->format,
> > > +                                          nb_samples,
> > > +                                          outlink->channel_layout,
> > > +                                          inlink->planar);
> > > +        in_channels = out_channels;
> > > +    }
> > > +
> > > +    /* If there's any conversion left to do, we need a buffer */
> > > +    if (format_conv || packing_conv || stereo_downmix) {
> > > +        aconvert->out_samplesref = avfilter_get_audio_buffer(outlink,
> > > +                                          AV_PERM_WRITE | AV_PERM_REUSE2,
> > > +                                          outlink->format,
> > > +                                          nb_samples,
> > > +                                          outlink->channel_layout,
> > > +                                          outlink->planar);
> > > +    }
> > > +
> > > +    /* if there's a format/mode conversion or packed stereo downmixing,
> > > +     * we need an audio_convert context
> > > +     */
> > > +    if (format_conv || packing_conv || (stereo_downmix && !outlink->planar)) {
> > > +        aconvert->in_strides[0]  = av_get_bytes_per_sample(inlink->format);
> > > +        aconvert->out_strides[0] = av_get_bytes_per_sample(outlink->format);
> > > +
> > > +        aconvert->out_data = aconvert->out_samplesref->data;
> > > +        if (aconvert->mix_samplesref)
> > > +            aconvert->in_data  = aconvert->mix_samplesref->data;
> > > +
> > > +        if (packing_conv) {
> > > +            if (outlink->planar) {
> > > +                if (aconvert->mix_samplesref)
> > > +                    aconvert->packed_data[0] =
> > > +                        aconvert->mix_samplesref->data[0];
> > > +                aconvert->in_data         = aconvert->packed_data;
> > > +                packed_stride             = aconvert->in_strides[0];
> > > +                aconvert->in_strides[0]  *= in_channels;
> > > +            } else {
> > > +                aconvert->packed_data[0]  = aconvert->out_samplesref->data[0];
> > > +                aconvert->out_data        = aconvert->packed_data;
> > > +                packed_stride             = aconvert->out_strides[0];
> > > +                aconvert->out_strides[0] *= out_channels;
> > > +            }
> > > +        } else if (!outlink->planar) {
> > > +            out_channels = 1;
> > > +        }
> > > +
> > > +        for (i = 1; i < out_channels; i++) {
> > > +            aconvert->packed_data[i] = aconvert->packed_data[i-1] +
> > > +                                           packed_stride;
> > > +            aconvert->in_strides[i]  = aconvert->in_strides[0];
> > > +            aconvert->out_strides[i] = aconvert->out_strides[0];
> > > +        }
> > > +
> > > +        aconvert->audioconvert_ctx =
> > > +                av_audio_convert_alloc(outlink->format, out_channels,
> > > +                                       inlink->format,  out_channels, NULL, 0);
> > 
> > Add a check here, just to be sure (or an av_assert if you're lazy).
> 
> Add a check for what?

alloc may fail

> 
> [...]
> > 
> > > +REMATRIX_FUNC_PACKED(stereo_to_mono_packed)
> > > +{
> > > +    while (nb_samples >= 4) {
> > > +        out[0] = (in[0] + in[1]) DIV2;
> > > +        out[1] = (in[2] + in[3]) DIV2;
> > > +        out[2] = (in[4] + in[5]) DIV2;
> > > +        out[3] = (in[6] + in[7]) DIV2;
> > > +        out += 4;
> > > +        in  += 8;
> > > +        nb_samples -= 4;
> > > +    }
> > > +    while (nb_samples--) {
> > > +        out[0] = (in[0] + in[1]) DIV2;
> > > +        out++;
> > > +        in += 2;
> > > +    }
> > > +}
> > > +
> > > +REMATRIX_FUNC_PACKED(mono_to_stereo_packed)
> > > +{
> > > +    while (nb_samples >= 4) {
> > > +        out[0] = out[1] = in[0];
> > > +        out[2] = out[3] = in[1];
> > > +        out[4] = out[5] = in[2];
> > > +        out[6] = out[7] = in[3];
> > > +        out += 8;
> > > +        in  += 4;
> > > +        nb_samples -= 4;
> > > +    }
> > > +    while (nb_samples--) {
> > > +        out[0] = out[1] = in[0];
> > > +        out += 2;
> > > +        in  += 1;
> > > +    }
> > > +}
> > 
> > why the nb_samples >= 4 special casing?
> 
> It's just faster when the loop is unrolled, nothing special.

ok, feel free to add a note mentioning speed issues

> 
> [...]
> 
> Rest of problems fixed.

Going to review your next patch. 
-- 
FFmpeg = Fanciful and Funny Mysterious Powered Elegant Gangster