[FFmpeg-devel] [PATCH] lavc/aacenc_quantization: use cbrt table

[Ganesh Ajjanagadde]

On Fri, Mar 11, 2016 at 3:05 AM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On 11.03.2016, at 03:48, Ganesh Ajjanagadde <gajjanag at gmail.com> wrote:
>
>> On Thu, Mar 10, 2016 at 3:12 AM, Reimar Döffinger
>> <Reimar.Doeffinger at gmx.de> wrote:
>>> On 10.03.2016, at 00:49, Ganesh Ajjanagadde <gajjanag at gmail.com> wrote:
>>>
>>>> There is no reason for computing cbrtf at runtime; we have a table for
>>>> this.
>>>>
>>>> Cruft needed due to the build system, the people who still like using
>>>> hardcoded tables and need for single cbrt_tab across the code.
>>>
>>> Could you please explain what exactly the problem is?
>>> Why would you need cbrt_table.c for example? You can just conditionally include it from cbrt_data.c for example.
>>> Hardcoded tables are meant to only change when the initialization code is run, not where the resulting data is located, i.e. it should only ever replace a
>>> int table[size];
>>> by
>>> const int table[size] = { data };
>>> (though with the indirection of placing the second one in a header)
>>
>> The data can't go into a header; else one gets multiple copies, see
>> above. Please not that I may still be missing something; this
>> hardcoded ifdefry and build system is something I am not that familiar
>> with.
>
> You just include the header in the C file (and only that one) where you want the data to be.
> Obviously if you do not want multiple copies that will be some shared C file, not the encoder or decoder code directly.
> But the same applies to the non-hardcoded tables, they also need a shared file or else you get two copies in the bss section.
> I really don't see why the issue you describe should need anything other than replacing a
> int ff_something[size];
> by
> #if hardcodec
> #include "something_table.h"
> #else
> ff_something[size];
> #endif
>
> hardcoding should _never_ ever require changing the code structure or the .c files compiled in.
> It should only consist of
> 1) generating a table header (can get a bit tricky admittedly, this should be the most difficult step)
> 2) including that header instead of the code otherwise creating the uninitialised table in .bss
> 3) disabling the code that initializes the table
>
> Nothing magic beyond that.

Ok. Let me put it this way: I have a super simple patch that simply
moves stuff to cbrt_data.c and works perfectly well under default
configure, with no changes to the Makefile apart from adding
cbrt_data.o to the list of objects for AAC.

As soon as I test under --enable-hardcoded-tables, I get linker errors
during the host table generation phase due to undefined ff_cbrt_tab,
etc. I also know why they occur: I declare ff_cbrt_tab as extern in
cbrt_tablegen.h, so basically during host generation phase cbrt_data.c
needs to also get compiled in, where the implementation is done. But
the host gen simply #includes cbrt_tablegen.h, sees the extern
declaration (since during host gen it #defs hardcoded to 0, that is
how it gets the "magic" done), does not find the implementation, and
naturally complains.

So one could of course duplicate the code in tablegen_template etc and
not #include cbrt_tablegen, it can actually be very short since we
don't care about efficiency there and the fancy current algorithm does
not need to be used.

But whatever it is, be it Makefile chicanery or some ad-hoc methods
like above, something needs to be done; there is no "natural"
solution.

TL;DR: I am not going to waste time on this. Anyone wants to do it, it
is conceptually trivial.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel