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

[Ganesh Ajjanagadde]

On Sat, Mar 12, 2016 at 11:35 AM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Sat, Mar 12, 2016 at 10:21:10AM -0500, Ganesh Ajjanagadde wrote:
>> 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.
>
> I really don't see what the problem is??
> Attached is a (approx.) 4 line change that avoids the Makefile
> mess and does not duplicate code.
> It needs one hack due to config.h usage in cbrt_data.c.
> It and the ugly cbrt_data.c include in the table generator
> could be avoided by simply leaving ff_cbrt_tab and the table
> generation code in cbrt_tablegen.h and using a cbrt_data.h
> in aacenc.c etc. which is more logical anyway than having
> a cbrt_tablegen.h header that contains declarations for
> stuff that is actually in cbrt_data.c.
> It seems to me the only problem here was caused by
> trying to physically move stuff into cbrt_data.c instead
> of just including whatever needs to be exported for
> use by the other files, and in addition (mis?) using
> the cbrt_tablegen.h header for the stuff in cbrt_data.c.
> I can of course try to clean up these last things,
> but I'm not really familiar with the code and
> some of the changes seem not completely trivial
> (e.g. the aacenc_quantization.h one)

There is nothing going on there, all I am trying trying to achieve is:
libavcodec/aacenc_quantization.h:108:                        quantized
= c*cbrtf(c)*IQ;
by quantized = cbrt_tab[c]*IQ.

There is just the details of:
1. Making sure cbrt_tableinit is called before this.
2. Making it build and run correctly.

> so I'd rather not.
> (I admit that there must be something about that
> tablegen code that is genuinely aligned with my
> thinking but not most else's, because to me it
> looks clean and obvious while a lot of people think
> it is a confusing horror... I have been unable to
> figure out a way to fix this)

Maybe it is because you were the one who wrote most of this stuff originally.
BTW, here is an example of why I consider it a horror: look at lines
62, 63, 998-1002 of libavcodec/Makefile. These are all things that are
not at all needed if we did not have this stuff.

And yes, for me the proof of the pudding is in the eating. In other
words, if you or someone else sends a small, simple patch achieving
the above one-line change, I will be very happy and will learn
something.

But until then, I can't really do much; it is just from my perspective useless:
1. Gains are anyway small.
2. Essentially it is a one line change as noted above, why do I need
to spend so much time trying to get it to work?
3. Why do we even need all this hardcoded table chicanery? No one has
presented a use case where they make a serious difference, with
benches to support when I called for it wrt cbrt_tab and aac. Of
course it will be faster in many cases (e.g mpegaudio_tablegen still
takes 1M cycles down from 10M before), but by how much is really the
question. Ronald gave one related to numerous small files, but even
that was a stretch as one can batch process them together. In fact,
taking some inspiration from a remark by wm4, I put in some effort for
making dynamic init faster, in one or two cases we got rid of this
chicanery altogether.
Most clients anyway do not --enable-hardcoded-tables as it is not the
default, and (rightfully so) is placed under the "expert options".

Simply put, why do I need to waste my time on a feature that:
a) essentially no client cares about
b) that is clearly not localized/contained properly and complicates
the build system.
c) adds complexity to the codebase that only a few people truly
understand so that
d) it becomes non-trivial in order to do a trivial change that has
arguably more impact than the net utility of all of this complexity?

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