On Sun, Mar 13, 2016 at 12:24:25PM -0400, Ganesh Ajjanagadde wrote: > On Sat, Mar 12, 2016 at 1:24 PM, Reimar Döffinger > <reimar.doeffin...@gmx.de> wrote: > > for (i = 0; i < 1<<13; i++) > > - cbrt_tab[i] = CBRT(cbrt_tab_dbl[i]); > > + AAC_RENAME(ff_cbrt_tab)[i] = CBRT(cbrt_tab_dbl[i]); > > } > > } > > Note that cbrt_tab_dbl is really intended to be shared by both the > fixed/floating table inits. This was another thing my patch achieved: > only doing the more expensive double table init once across > float/fixed, and then doing the cheap conversion to uint32_t via > av_float2int or lrint(x*8192). Please change; it could go into a > separate patch if you prefer.
IMHO you really need to argue why that would be desirable. The only case I can see this as useful is if someone runs at the same time fixed-point AAC decode and AAC encode, where it saves a bit of startup time. In all other cases you waste time and memory initializing a table that will never be used. Also doing that you end up with 3 different things: one that should only be compiled in when there is fixed-point stuff, one you should only have for float, and one that is needed if either is enabled. As a result, you'd probably need a 3rd file (or some #ifdef mess that duplicates stuff in the Makefile). Though I admit once you have a single init function it becomes easier to put it all in one file. It still will be quite ugly ifdefs though. That you tried to cram three (mostly unrelated) changes in one patch definitely explains a good part of the problems. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
