[FFmpeg-devel] [PATCH 2/5] avcodec/fft_template: Remove unused fixed-point cosine tables

[Andreas Rheinhardt]

Michael Niedermayer:
> On Sun, Jan 10, 2021 at 01:56:21AM +0100, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Thu, Jan 07, 2021 at 12:13:05AM +0100, Andreas Rheinhardt wrote:
>>>> There are three types of FFTs: floating-point, 32-bit fixed-point and
>>>> 16-bit fixed-point. The latter has exactly one user: The fixed-point
>>>> AC-3-encoder; the cosine tables used by it use up to seven bits. The
>>>> tables corresponding to eight to seventeen bits are unused, as are the
>>>> FFT functions for these bits.
>>>>
>>>> Therefore this commit removes these tables and functions. This is
>>>> especially beneficial when using hardcoded tables as they take up moreFirst,
>>>> than 255 KiB. But even without it one saves said unused functions as
>>>> well as entries in corresponding tables (this also saves relocations).
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>> ---
>>>> Thee changes to ARM assembly are honstely untested. I hope someone can
>>>> test them. Btw: It seems that the ARM assembly code wouldn't be able to
>>>> deal with an FFT with more than 16 bits (no function for this has been
>>>> defined), which only worked because no one ever used that many bits with
>>>> the fixed-point FFT.
>>>>
>>>>  libavcodec/arm/fft_fixed_neon.S | 18 ------------------
>>>>  libavcodec/cos_tablegen.c       |  4 ++--
>>>>  libavcodec/fft.h                |  4 +++-
>>>>  libavcodec/fft_fixed.c          |  1 +
>>>>  libavcodec/fft_template.c       | 31 +++++++++++++++++++++++--------
>>>>  tests/fate/fft.mak              |  8 ++++++--
>>>>  6 files changed, 35 insertions(+), 31 deletions(-)
>>>
>>> make -j32 libavcodec/tests/fft-fixed && libavcodec/tests/fft-fixed
>>> Segmentation fault (core dumped)
>>>
>>> (if you cant repro say so and ill rebuild with debug symbols ...)
>>>
>>> thx
>>> [...]
>>>
>> 1. Lynne has an alternative patchset that makes the only user of
>> fft_fixed use fft_fixed_32 instead, so this is not important any more.
> 
>> 2. Are you testing the ARM assembly code (for which I ask for a test) or
> 
> x86-64
> 
>> not? If not, then this surprises me. Did you apply the changes to
>> fft.mak (some of the tests have been removed as they tested
>> functionality that was unused (apart from the tests) and has therefore
>> been removed).
> 
> i applied the changes from this patchset up to and including the patch
> and also did a make distclean
> FFT 512 test

Now I see what's the problem. You are not running the fft-tests from the
FATE-suite (where I disabled unused and newly unsupported tests);
instead you are directly using the underlying tests in libavcodec/tests
and these tests default to nbits = 9, which is unsupported for fft-fixed
with this patch applied. ff_fft_init_fixed is correctly erroring out,
yet said error code is ignored in the test's fft_init (therefore testing
an unsupported nbits already leads to segfaults on master (try
libavcodec/tests/fft -n 20)). So the error checking in the tests needs
to be improved (there are other unchecked allocations, too).
Furthermore, if this patch were to be applied, one should set the
default number of bits for fft-fixed to something supported; but that
point is moot given that this patch has been superseded by Lynne's (who
wants to nuke fft-fixed).

- Andreas