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

[James Almer]

On 7/28/2017 11:59 AM, Tyler Jones wrote:
>>> --- a/libavcodec/vorbisenc.c
>>> +++ b/libavcodec/vorbisenc.c
>>> @@ -33,6 +33,7 @@
>>>  #include "mathops.h"
>>>  #include "vorbis.h"
>>>  #include "vorbis_enc_data.h"
>>> +#include "vorbispsy.h"
>>>  
>>>  #include "audio_frame_queue.h"
>>>  #include "libavfilter/bufferqueue.h"
>>> @@ -136,6 +137,7 @@ typedef struct vorbis_enc_context {
>>>      int64_t next_pts;
>>>  
>>>      AVFloatDSPContext *fdsp;
>>> +    VorbisPsyContext *vpctx;
>>
>> Why a pointer? I don't see the benefit. It means an unnecessary malloc
>> and free call.
> 
> You're probably right. It's changed now.
> 
>>> @@ -1252,6 +1270,7 @@ static av_cold int vorbis_encode_close(AVCodecContext *avctx)
>>>      ff_mdct_end(&venc->mdct[1]);
>>>      ff_af_queue_close(&venc->afq);
>>>      ff_bufqueue_discard_all(&venc->bufqueue);
>>> +    ff_psy_vorbis_close(venc->vpctx);
>>
>> You should pass a pointer to venc->vpctx instead, regardless of what you
>> do with the comment above.
> 
> I'm not sure I understand what you mean. It is passing a pointer to a
> VorbisPsyContext, please see the prototype:
> 
>     av_cold void ff_psy_vorbis_close(VorbisPsyContext *vpctx);    

For this version (but not V4) it should have been a pointer to the
pointer in vorbis_enc_context, that is **vpctx. Otherwise, the
av_freep() call in ff_psy_vorbis_close() would have zeroed a copy of
said pointer.

> 
>>> +/**
>>> + * Calculate the variance of a block of samples
>>> + *
>>> + * @param in     Array of input samples
>>> + * @param length Number of input samples being analyzed
>>> + * @return       The variance for the current block
>>> + */
>>> +static float variance(const float *in, int length)
>>> +{
>>> +    int i;
>>> +    float mean = 0.0f, square_sum = 0.0f;
>>> +
>>> +    for (i = 0; i < length; i++) {
>>> +        mean += in[i];
>>> +        square_sum += in[i] * in[i];
>>
>> Can't you use AVFloatDSPContext's scalarproduct_float for square_sum?
>> The constrains are lax. 16 byte alignment for in and length a multiple
>> of 4. You can pad the buffer if needed to achieve that.
> 
> You are correct, it is switched over now.
> 
>>> +    }
>>> +
>>> +    mean /= length;
>>> +    return (square_sum - length * mean * mean) / (length - 1);
>>> +}
>>> +
>>> +av_cold int ff_psy_vorbis_init(VorbisPsyContext *vpctx, int sample_rate,
>>> +                               int channels, int blocks)
>>> +{
>>> +    int crit_freq;
>>> +    float Q[2] = {.54, 1.31}; // Quality values for maximally flat cascaded filters
>>
>> const float Q[2]
> 
> Fixed.
> 
> Thank you for catching these mistakes and providing suggestions. A new version
> of this patch will be sent soon.
> 
> Thanks again,
> 
> Tyler Jones
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>