[FFmpeg-devel] [PATCH 3/3] Allow either float or int16_t input and output in ff_iir_filter().

[Justin Ruggles]

On 01/19/2011 11:40 PM, Ronald S. Bultje wrote:

> Hi,
> 
> 2011/1/19 M?ns Rullg?rd <mans at mansr.com>:
>> Justin Ruggles <justin.ruggles at gmail.com> writes:
>>
>>> ---
>>>  libavcodec/iirfilter.c |  103 +++++++++++++++++++++++++++++++----------------
>>>  libavcodec/iirfilter.h |    3 +-
>>>  libavcodec/psymodel.c  |    3 +-
>>>  3 files changed, 72 insertions(+), 37 deletions(-)
>>>
>>>
>>>  void ff_iir_filter(const struct FFIIRFilterCoeffs *c, struct FFIIRFilterState *s,
>>> -                   int size, const int16_t *src, int sstep, int16_t *dst, int dstep)
>>> +                   int size, const void *src, int sstep, void *dst, int dstep,
>>> +                   enum AVSampleFormat sample_fmt)
>>>  {
>>> -    int i;
>>> -
>>> -    if(c->order == 4){
>>> -        for(i = 0; i < size; i += 4){
>>> -            float in, res;
>>> -
>>> -            FILTER(0, 1, 2, 3);
>>> -            FILTER(1, 2, 3, 0);
>>> -            FILTER(2, 3, 0, 1);
>>> -            FILTER(3, 0, 1, 2);
>>> -        }
>>> -    }else{
>>> -        for(i = 0; i < size; i++){
>>> -            int j;
>>> -            float in, res;
>>> -            in = *src * c->gain;
>>> -            for(j = 0; j < c->order; j++)
>>> -                in += c->cy[j] * s->x[j];
>>> -            res = s->x[0] + in + s->x[c->order >> 1] * c->cx[c->order >> 1];
>>> -            for(j = 1; j < c->order >> 1; j++)
>>> -                res += (s->x[j] + s->x[c->order - j]) * c->cx[j];
>>> -            for(j = 0; j < c->order - 1; j++)
>>> -                s->x[j] = s->x[j + 1];
>>> -            *dst = av_clip_int16(lrintf(res));
>>> -            s->x[c->order - 1] = in;
>>> -            src += sstep;
>>> -            dst += dstep;
>>> -        }
>>> +    if (!(sample_fmt == AV_SAMPLE_FMT_S16 || sample_fmt == AV_SAMPLE_FMT_FLT))
>>> +        return;
>>> +
>>> +    if (sample_fmt == AV_SAMPLE_FMT_S16) {
>>> +        void (*conv_filter_output)(float, void *) = conv_filter_output_s16;
>>> +        if (c->order == 4)
>>> +            FILTER4(int16_t)
>>> +        else
>>> +            FILTER_DIRECT_FORM_II(int16_t)
>>> +    } else {
>>> +        void (*conv_filter_output)(float, void *) = conv_filter_output_flt;
>>> +        if (c->order == 4)
>>> +            FILTER4(float)
>>> +        else
>>> +            FILTER_DIRECT_FORM_II(float)
>>
>> I think two different functions would be preferable.  Fewer branches
>> (and parameters) is always better, and I dread to think what some
>> compilers might do with code like the above.  Also consider that we
>> might want to write asm versions of those some day.  That is much
>> easier if they are separated.

Ok. I do want to avoid code duplication, but I think I can do it purely
with macros and still provide 2 separate functions.

> I tend to agree. For voice codecs, we wrote a bunch of these filters
> already anyway, and there's always 2 of each, one int (function()) and
> one float (functionf()), works fine there, is probably OK here also.
> 
> Also, how similar are voice-codec iir filters
> (ff_celp_lp_zero_synthesis_filterf()) from this one?


ff_celp_lp_zero_synthesis_filterf(): Seems to be an fir filter.

ff_celp_lp_synthesis_filterf(): Purely backwards iir filter.

ff_iir_filter(): Some of the coefficients are applied to input samples,
some are applied to output samples.

The celp filters could probably be handled in ff_iir_filter() with some
zero coefficients if desired.  They would likely be slower though unless
special-cased.  Also, the coeffs seem to be generated from the input
stream, so ff_iir_filter_init_coeffs() would need to be altered to
accept input coefficients rather than calculating them based on filter
type, cutoff ratio, etc... or a new function created for initializing
coefficients directly (at least partially).

Currently the code in iirfilter.c is not completely generic.  It assumes
that the output coefficients can be normalized to integers.  This is
fine for Butterworth and Biquad (which are what I need for ac3enc), so I
don't need to modify it, but it probably should be changed if we want to
use different filter types in the future.

-Justin