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

[Rostislav Pehlivanov]

On 17 July 2017 at 16:17, Tyler Jones <tdjones879 at gmail.com> wrote:

> The encoder will attempt to determine the existence of transient
> signals by applying a 4th order highpass filter to remove dominant
> low frequency waveforms. Frames are then split up into blocks
> where the variance is calculated and compared with blocks from
> the previous frame. A preecho is only likely to be noticeable when
> relatively quiet audio is followed by a loud transient signal.
>
> Signed-off-by: Tyler Jones <tdjones879 at gmail.com>
> ---
> V2 - Properly prefix non-static functions with "ff_"
>
>  libavcodec/Makefile    |   2 +-
>  libavcodec/vorbisenc.c |  28 +++++++--
>  libavcodec/vorbispsy.c | 153 ++++++++++++++++++++++++++++++
> +++++++++++++++++++
>  libavcodec/vorbispsy.h |  79 +++++++++++++++++++++++++
>  4 files changed, 256 insertions(+), 6 deletions(-)
>  create mode 100644 libavcodec/vorbispsy.c
>  create mode 100644 libavcodec/vorbispsy.h
>




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



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


> +        av_freep(&vpctx);
> +    }
> +}
> diff --git a/libavcodec/vorbispsy.h b/libavcodec/vorbispsy.h
> new file mode 100644
> index 0000000000..021a5e8a28
> --- /dev/null
> +++ b/libavcodec/vorbispsy.h
> @@ -0,0 +1,79 @@
> +/*
> + * Vorbis encoder psychoacoustic model
> + * Copyright (C) 2017 Tyler Jones
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * Vorbis psychoacoustic model
> + */
> +
> +#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?


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.