[FFmpeg-devel] [PATCH 1/6] ac3enc_fixed: convert to 32-bit sample format

[Lynne]

Jan 12, 2021, 08:48 by andreas.rheinhardt at gmail.com:

> Lynne:
>
>> Jan 9, 2021, 22:01 by andreas.rheinhardt at gmail.com:
>>
>>> Lynne:
>>>
>>>> @@ -165,7 +164,11 @@ typedef struct AC3EncodeContext {
>>>>  AVCodecContext *avctx;                  ///< parent AVCodecContext
>>>>  PutBitContext pb;                       ///< bitstream writer context
>>>>  AudioDSPContext adsp;
>>>> +#if AC3ENC_FLOAT
>>>>  AVFloatDSPContext *fdsp;
>>>> +#else
>>>> +    AVFixedDSPContext *fdsp;
>>>> +#endif
>>>>  MECmpContext mecc;
>>>>  AC3DSPContext ac3dsp;                   ///< AC-3 optimized functions
>>>>  FFTContext mdct;                        ///< FFT context for MDCT calculation
>>>>
>>> [...]
>>>
>>>> @@ -118,9 +89,10 @@ static CoefType calc_cpl_coord(CoefSumType energy_ch, CoefSumType energy_cpl)
>>>>  static av_cold void ac3_fixed_mdct_end(AC3EncodeContext *s)
>>>>  {
>>>>  ff_mdct_end(&s->mdct);
>>>> +    av_freep(&s->fdsp);
>>>> +    av_freep(&s->mdct_window);
>>>>  }
>>>>
>>>
>>> ff_ac3_encode_close already unconditionally frees fdsp, so freeing it
>>> above is either unnecessary or ac3_float_mdct_end should also free its
>>> fdsp (and ff_ac3_encode_close shouldn't). Freeing mdct_window can also
>>> be moved to ff_ac3_encode_close (which already frees several buffers
>>> whose pointed-to-type depends upon the encoding mode).
>>> Notice that ac3enc.c uses the fixed-point mode, but the layout of
>>> AC3EncodeContext does not depend upon this (apart from pointed-to-types,
>>> of course). Actually, ff_mdct_end does the same for both fixed- and
>>> floating-point mode, so one could even incorporate
>>> ac3_fixed/float_mdct_end into ff_ac3_encode_close.
>>>
>> Done. Left ac3_fixed/float_mdct_end as-is for now.
>> New patch attached.
>> >
>> @@ -129,8 +99,31 @@ static av_cold void ac3_fixed_mdct_end(AC3EncodeContext *s)
>>  */
>>  static av_cold int ac3_fixed_mdct_init(AC3EncodeContext *s)
>>  {
>> +    int32_t *iwin;
>> +    float fwin[AC3_BLOCK_SIZE];
>> +
>>  int ret = ff_mdct_init(&s->mdct, 9, 0, -1.0);
>> -    s->mdct_window = ff_ac3_window;
>>
>
> You forgot to remove this table.
>
That's done as a part of patch 4/6.



>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    iwin = av_malloc_array(AC3_WINDOW_SIZE, sizeof(*iwin));
>> +    if (!iwin)
>> +        return AVERROR(ENOMEM);
>> +
>> +    ff_kbd_window_init(fwin, 5.0, AC3_WINDOW_SIZE/2);
>> +
>> +    for (int i = 0; i < AC3_WINDOW_SIZE/2; i++)
>> +        iwin[i] = lrintf(fwin[i] * (1 << 22));
>> +
>>
>
> Does this lead to a different result than using ff_kbd_window_init_fixed
> directly?
>

Yes, slightly. We need a different scaling, so if we work with what that
function gives us the rounding is different. Nothing major, and definitely
still better than before, but it's there.
As for what we gain by switching to it... nothing except a single line for
the temporary float array. We do almost the same that function does
anyway.
So I think I'll leave this as-is.