[FFmpeg-devel] [PATCH V2 1/2] avcodec/vorbisenc: Add pre-echo detection

[Tyler Jones]

On Wed, Jul 26, 2017 at 12:51:31AM +0100, Rostislav Pehlivanov wrote:
> On 17 July 2017 at 16:17, Tyler Jones <tdjones879 at gmail.com> wrote:
> 
> > +    float last_var;
> > +    const float eps = 1e-4;
> >
> 
> Use normal notation for floats and add an f at the end to inform the
> compiler the constant is a float.

Fixed.

> > +{
> > +    if (vpctx) {
> > +        if (vpctx->filter_delay)
> > +            av_freep(&vpctx->filter_delay);
> > +
> > +        if (vpctx->variance)
> > +            av_freep(&vpctx->variance);
> > +
> >
> 
> You can free NULL pointers, n o need to check.

Fixed.

> > +#ifndef AVCODEC_VORBISPSY_H
> > +#define AVCODEC_VORBISPSY_H
> > +
> > +#include "libavutil/attributes.h"
> > +
> > +/**
> > + * Second order IIR Filter
> > + */
> > +typedef struct IIRFilter {
> > +    float b[3]; ///< Normalized cofficients for numerator of transfer
> > function
> > +    float a[3]; ///< Normalized coefficiets for denominator of transfer
> > function
> > +} IIRFilter;
> >
> 
> We already have an IIR filter (libavcodec/iirfilter.h), could you check it
> out if it can be reused perhaps?

This is where I had initially looked, and my issue was that it was not possible
to generate a high-pass butterworth or biquads that behaved like butterworths
when cascaded (controlling the quality factor). Manually initializing the structs
and only using the filter function resulted in more complex code and more boilerplate
than implementing it separately here.

If necessary, I can switch to using iirfilter.h now, but I'd rather use this implementation
temporarily and add the functionality needed in iirfilter.h in a separate patch if that
is acceptable. I'd argue that this would result in cleaner code. From what I can see,
psymodel.c is the only component that uses iirfilter.h, and the biquad filter is unused,
so it should be a less difficult change to make.

> Apart from those patch looks good and should be ready to merge once those
> nits get fixed.
> I can hear a noticeable positive difference at low rates, good job.

Thanks for catching these mistakes and taking a look.

Thanks again,

Tyler Jones
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170726/32187483/attachment.sig>