Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On 13.03.2016, at 19:11, Ganesh Ajjanagadde  wrote:

> On Sun, Mar 13, 2016 at 1:46 PM, Reimar Döffinger
>  wrote:
>> 
>>> 
>>> --enable-hardcoded-tables partially does that; it increases memory
>>> usage as tables are burnt into the library at some gains for
>>> initialization time.
>> 
>> No, exactly not. It increases disk usage, but it decreases memory
>> usage.
>> Tables are not loaded just because they are in the binary, not
>> when they are in .rodata.
>> If you are really lucky, even when the cbrt tables are used but only
>> a small part of them, with --enable-hardcoded-tables you'd not
>> even necessarily end up with all of them in RAM (admittedly
>> you can also see that as a disadvantage: the chances that
>> you end up with a page fault in the middle of decoding when
>> you don't really want it is there, too).
> 
> Correct me if I am wrong, but I don't think an OS/runtime system
> actually loads at the granularity of individual tables. It would load
> at the granularity of pages, there can be a greater chance of page
> faults with larger images, such as due to larger .rodata.

The cbrt tables are a lot larger than a page, so I considered that unnecessary 
details.
Also all tables, whether in .bss or .rodata share the page granularity issue.
"greater chance of page faults" is not strictly true as initialising the tables 
in .bss will cause page faults, too. But yes the the .rodata page faults need 
to go to disk and are more costly.
Unless you run a system supporting in-place execution...

>> Gaining initialization time is absolutely not the point of
>> that option, it doesn't make sense and I have no idea
>> how that myth came to be (it may admittedly be true
>> in some cases, but it's not the point or very relevant).
>> Just for AAC CBRT tables (which are small in comparison),
>> hardcoded tables saves 128 kB of RAM/swap once you no longer
>> use the AAC codecs compared to your patch (64 kB for the
>> double table + 2*32kB for the final tables + possibly
>> a little bit from not needing the initialization code).
> 
> Ok, so the tables are pulled into RAM on a as-needed basis. But then
> when are they offloaded? Savings (apart from the 64 kB for the double
> table) will only kick in at the time of offloading when the page is
> freed.

It will be swapped out normally, whenever the OS decides it has a better use 
for the RAM.
.rodata will be easier to swap as it is non-dirty (does not require a write), 
but the exact priority compared to e.g. cache pages is up to the OS. It could 
keep .rodata pages just as part of its generic file system cache if it wants...
Initialised .bss tables can of course be swapped out, too, but they require a 
write and the OS is less likely to do it, plus it requires you to have swap 
(systems designed to leave no traces cannot use swap for example, at least not 
with flash storage where securely erasing data is kind of not possible). If one 
were to dedicate sufficient craziness to it one could probably build a system 
that used munmap/madvise and SIGSEGV or user-space pagefault handling to 
combine the advantages of both. I certainly wouldn't recommend it though.
And as said, in addition sharing the .bss tables across instances tends to not 
be possible but is trivial for .rodata tables.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Ganesh Ajjanagadde 
On Sun, Mar 13, 2016 at 1:46 PM, Reimar Döffinger
 wrote:
> On Sun, Mar 13, 2016 at 01:27:52PM -0400, Ganesh Ajjanagadde wrote:
>> On Sun, Mar 13, 2016 at 1:21 PM, Reimar Döffinger
>>  wrote:
>> >> > I don't understand the waste; the double init anyway needs to be done,
>> >> > all tables are derived from it. This dates to an approach I did in
>> >> > commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.
>> >>
>> >> The waste is in writing to the table that will never be used.
>> >> As long as it is left as 0, it will not use memory, as soon
>> >> as you touch it it uses up memory.
>> >> But see my patch, if people think it's better it can be added.
>> >
>> > And looking at that commit, I actually realize now that the
>> > cbrt_tab_dbl will also permanently waste 64 kB of memory despite
>> > never being used again.
>> > Using malloc/free for it would potentially be better (though
>> > in general I have some doubts about the wisdom of optimizing
>> > for initialization time at the detriment of runtime performance
>> > or memory usage).
>>
>> --enable-hardcoded-tables partially does that; it increases memory
>> usage as tables are burnt into the library at some gains for
>> initialization time.
>
> No, exactly not. It increases disk usage, but it decreases memory
> usage.
> Tables are not loaded just because they are in the binary, not
> when they are in .rodata.
> If you are really lucky, even when the cbrt tables are used but only
> a small part of them, with --enable-hardcoded-tables you'd not
> even necessarily end up with all of them in RAM (admittedly
> you can also see that as a disadvantage: the chances that
> you end up with a page fault in the middle of decoding when
> you don't really want it is there, too).

Correct me if I am wrong, but I don't think an OS/runtime system
actually loads at the granularity of individual tables. It would load
at the granularity of pages, there can be a greater chance of page
faults with larger images, such as due to larger .rodata.

> Gaining initialization time is absolutely not the point of
> that option, it doesn't make sense and I have no idea
> how that myth came to be (it may admittedly be true
> in some cases, but it's not the point or very relevant).
> Just for AAC CBRT tables (which are small in comparison),
> hardcoded tables saves 128 kB of RAM/swap once you no longer
> use the AAC codecs compared to your patch (64 kB for the
> double table + 2*32kB for the final tables + possibly
> a little bit from not needing the initialization code).

Ok, so the tables are pulled into RAM on a as-needed basis. But then
when are they offloaded? Savings (apart from the 64 kB for the double
table) will only kick in at the time of offloading when the page is
freed.

> Of course 128 kB less in swap space vs. 64 kB more in the
> binary/main storage is not really very useful in most cases,
> but I don't think one can argue it is worse.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On Sun, Mar 13, 2016 at 01:27:52PM -0400, Ganesh Ajjanagadde wrote:
> On Sun, Mar 13, 2016 at 1:21 PM, Reimar Döffinger
>  wrote:
> >> > I don't understand the waste; the double init anyway needs to be done,
> >> > all tables are derived from it. This dates to an approach I did in
> >> > commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.
> >>
> >> The waste is in writing to the table that will never be used.
> >> As long as it is left as 0, it will not use memory, as soon
> >> as you touch it it uses up memory.
> >> But see my patch, if people think it's better it can be added.
> >
> > And looking at that commit, I actually realize now that the
> > cbrt_tab_dbl will also permanently waste 64 kB of memory despite
> > never being used again.
> > Using malloc/free for it would potentially be better (though
> > in general I have some doubts about the wisdom of optimizing
> > for initialization time at the detriment of runtime performance
> > or memory usage).
> 
> --enable-hardcoded-tables partially does that; it increases memory
> usage as tables are burnt into the library at some gains for
> initialization time.

No, exactly not. It increases disk usage, but it decreases memory
usage.
Tables are not loaded just because they are in the binary, not
when they are in .rodata.
If you are really lucky, even when the cbrt tables are used but only
a small part of them, with --enable-hardcoded-tables you'd not
even necessarily end up with all of them in RAM (admittedly
you can also see that as a disadvantage: the chances that
you end up with a page fault in the middle of decoding when
you don't really want it is there, too).
Gaining initialization time is absolutely not the point of
that option, it doesn't make sense and I have no idea
how that myth came to be (it may admittedly be true
in some cases, but it's not the point or very relevant).
Just for AAC CBRT tables (which are small in comparison),
hardcoded tables saves 128 kB of RAM/swap once you no longer
use the AAC codecs compared to your patch (64 kB for the
double table + 2*32kB for the final tables + possibly
a little bit from not needing the initialization code).
Of course 128 kB less in swap space vs. 64 kB more in the
binary/main storage is not really very useful in most cases,
but I don't think one can argue it is worse.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Ganesh Ajjanagadde 
On Sun, Mar 13, 2016 at 1:21 PM, Reimar Döffinger
 wrote:
> On Sun, Mar 13, 2016 at 06:14:18PM +0100, Reimar Döffinger wrote:
>> On Sun, Mar 13, 2016 at 01:12:57PM -0400, Ganesh Ajjanagadde wrote:
>> > On Sun, Mar 13, 2016 at 12:49 PM, Reimar Döffinger
>> >  wrote:
>> > > 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
>> > >>  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.
>> >
>> > I don't understand the waste; the double init anyway needs to be done,
>> > all tables are derived from it. This dates to an approach I did in
>> > commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.
>>
>> The waste is in writing to the table that will never be used.
>> As long as it is left as 0, it will not use memory, as soon
>> as you touch it it uses up memory.
>> But see my patch, if people think it's better it can be added.
>
> And looking at that commit, I actually realize now that the
> cbrt_tab_dbl will also permanently waste 64 kB of memory despite
> never being used again.
> Using malloc/free for it would potentially be better (though
> in general I have some doubts about the wisdom of optimizing
> for initialization time at the detriment of runtime performance
> or memory usage).

--enable-hardcoded-tables partially does that; it increases memory
usage as tables are burnt into the library at some gains for
initialization time.

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On Sun, Mar 13, 2016 at 06:14:18PM +0100, Reimar Döffinger wrote:
> On Sun, Mar 13, 2016 at 01:12:57PM -0400, Ganesh Ajjanagadde wrote:
> > On Sun, Mar 13, 2016 at 12:49 PM, Reimar Döffinger
> >  wrote:
> > > 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
> > >>  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.
> > 
> > I don't understand the waste; the double init anyway needs to be done,
> > all tables are derived from it. This dates to an approach I did in
> > commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.
> 
> The waste is in writing to the table that will never be used.
> As long as it is left as 0, it will not use memory, as soon
> as you touch it it uses up memory.
> But see my patch, if people think it's better it can be added.

And looking at that commit, I actually realize now that the
cbrt_tab_dbl will also permanently waste 64 kB of memory despite
never being used again.
Using malloc/free for it would potentially be better (though
in general I have some doubts about the wisdom of optimizing
for initialization time at the detriment of runtime performance
or memory usage).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On Sun, Mar 13, 2016 at 01:12:57PM -0400, Ganesh Ajjanagadde wrote:
> On Sun, Mar 13, 2016 at 12:49 PM, Reimar Döffinger
>  wrote:
> > 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
> >>  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.
> 
> I don't understand the waste; the double init anyway needs to be done,
> all tables are derived from it. This dates to an approach I did in
> commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.

The waste is in writing to the table that will never be used.
As long as it is left as 0, it will not use memory, as soon
as you touch it it uses up memory.
But see my patch, if people think it's better it can be added.

> I had no fancy ifdefs for this, just some Makefile magic.

Because you also always included both tables in the binary,
whether used or not (and due to the shared init function
the linker could not remove the unused one).
I can see that one could argue either way concerning
ifdef mess vs. a few kB wasted though.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On Sun, Mar 13, 2016 at 05:53:10PM +0100, Reimar Döffinger wrote:
> On Sun, Mar 13, 2016 at 05:50:17PM +0100, Hendrik Leppkes wrote:
> > On Sun, Mar 13, 2016 at 5:24 PM, Ganesh Ajjanagadde  
> > wrote:
> > >> @@ -75,9 +66,8 @@ static av_cold void AAC_RENAME(cbrt_tableinit)(void)
> > >>  }
> > >>
> > >>  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.
> > >
> > 
> > Having both float and fixed decoders used at the same time seems like
> > a rather unlikely use-case, so if such an optimization takes rather
> > high complexity, its probably not worth going, IMHO.
> 
> Nah, it should be done separately because it needs some
> code reshuffling that easily gets confusing, but I don't
> think it will be hard.

Wasn't (though I haven't tested properly), see attached,
still don't think it is a good idea though.
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 6bb1af1..ec46d22 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -140,7 +140,7 @@ OBJS-$(CONFIG_AAC_DECODER) += aacdec.o aactab.o aacsbr.o aacps_float
   sbrdsp.o aacpsdsp_float.o cbrt_data.o
 OBJS-$(CONFIG_AAC_FIXED_DECODER)   += aacdec_fixed.o aactab.o aacsbr_fixed.o aacps_fixed.o \
   aacadtsdec.o mpeg4audio.o kbdwin.o \
-  sbrdsp_fixed.o aacpsdsp_fixed.o cbrt_data_fixed.o
+  sbrdsp_fixed.o aacpsdsp_fixed.o cbrt_data.o
 OBJS-$(CONFIG_AAC_ENCODER) += aacenc.o aaccoder.o aacenctab.o\
   aacpsy.o aactab.o  \
   aacenc_is.o \
@@ -1017,8 +1017,7 @@ $(GEN_HEADERS): $(SUBDIR)%_tables.h: $(SUBDIR)%_tablegen$(HOSTEXESUF)
 	$(M)./$< > $@
 
 ifdef CONFIG_HARDCODED_TABLES
-$(SUBDIR)cbrt_data.o: $(SUBDIR)cbrt_tables.h
-$(SUBDIR)cbrt_data_fixed.o: $(SUBDIR)cbrt_fixed_tables.h
+$(SUBDIR)cbrt_data.o: $(SUBDIR)cbrt_tables.h $(SUBDIR)cbrt_fixed_tables.h
 $(SUBDIR)aacps_float.o: $(SUBDIR)aacps_tables.h
 $(SUBDIR)aacps_fixed.o: $(SUBDIR)aacps_fixed_tables.h
 $(SUBDIR)aactab_fixed.o: $(SUBDIR)aac_fixed_tables.h
diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 883ed52..8554149 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -1103,8 +1103,7 @@ static av_cold void aac_static_table_init(void)
 AAC_RENAME(ff_init_ff_sine_windows)(10);
 AAC_RENAME(ff_init_ff_sine_windows)( 9);
 AAC_RENAME(ff_init_ff_sine_windows)( 7);
-
-AAC_RENAME(ff_cbrt_tableinit)();
+ff_cbrt_tableinit();
 }
 
 static AVOnce aac_table_init = AV_ONCE_INIT;
diff --git a/libavcodec/cbrt_data.c b/libavcodec/cbrt_data.c
index f5d9778..962fc65 100644
--- a/libavcodec/cbrt_data.c
+++ b/libavcodec/cbrt_data.c
@@ -22,7 +22,12 @@
 #include "cbrt_data.h"
 
 #if CONFIG_HARDCODED_TABLES
+#if CONFIG_AAC_DECODER || CONFIG_AAC_ENCODER
 #include "libavcodec/cbrt_tables.h"
+#endif
+#if CONFIG_AAC_FIXED_DECODER
+#include "libavcodec/cbrt_fixed_tables.h"
+#endif
 #else
 #include "cbrt_tablegen.h"
 #endif
diff --git a/libavcodec/cbrt_data.h b/libavcodec/cbrt_data.h
index 232f74f..a3da456 100644
--- a/libavcodec/cbrt_data.h
+++ b/libavcodec/cbrt_data.h
@@ -24,13 +24,11 @@
 #include 
 
 #if CONFIG_HARDCODED_TABLES
-#define ff_cbrt_tableinit_fixed()
 #define ff_cbrt_tableinit()
 extern const uint32_t ff_cbrt_tab[1 << 13];
 extern const uint32_t ff_cbrt_tab_fixed[1 << 13];
 #else
 void ff_cbrt_tableinit(void);
-void ff_cbrt_tableinit_fixed(void);
 extern uint32_t ff_cbrt_tab[1 << 13];
 extern uint32_t ff_cbrt_tab_fixed[1 << 13];
 #endif
diff --git a/libavcodec/cbrt_tablegen.h b/libavcodec/cbrt_tablegen.h
index 9af18d8..328c851 100644
--- a/libavcodec/cbrt_tablegen.h
+++ b/libavcodec/cbrt_tablegen.h
@@ -27,20 +27,22 @@
 #include 
 #include "libavutil/attributes.h"
 #include "libavutil/intfloat.h"
-#include "libavcodec/aac_defines.h"
 
-#if USE_FIXED
-#define CBRT(x) lrint((x) * 8192)
-#else
-#define CBRT(x) av_float2int((float)(x))
+#if CONFIG_AAC_DECODER || CONFIG_AAC_ENCODER
+uint32_t ff_cbrt_tab[1 << 13];
+#endif
+#if CONFIG_AAC_FIXED_DECODER
+uint32_t ff_cbrt_tab_fixed[1 << 13];
 #endif
 
-uint32_t AAC_RENAME(ff_cbrt_tab)[1 << 13];
-
-av_cold void AAC_RENAME(ff_cbrt_tableinit)(void)
+av_cold void ff_cbrt_tableinit(void)
 {
 static double cbrt_tab_dbl[1 << 13];
-if

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Ganesh Ajjanagadde 
On Sun, Mar 13, 2016 at 12:49 PM, Reimar Döffinger
 wrote:
> 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
>>  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.

I don't understand the waste; the double init anyway needs to be done,
all tables are derived from it. This dates to an approach I did in
commit:  07a11ebcab9b31e9fc784029e5d24e6fbf486ff3.

> 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.

I had no fancy ifdefs for this, just some Makefile magic.

> That you tried to cram three (mostly unrelated) changes in one
> patch definitely explains a good part of the problems.

That is a very fair assessment. Remarks dropped, patch tested and LGTM. Thanks.

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
On Sun, Mar 13, 2016 at 05:50:17PM +0100, Hendrik Leppkes wrote:
> On Sun, Mar 13, 2016 at 5:24 PM, Ganesh Ajjanagadde  
> wrote:
> >> @@ -75,9 +66,8 @@ static av_cold void AAC_RENAME(cbrt_tableinit)(void)
> >>  }
> >>
> >>  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.
> >
> 
> Having both float and fixed decoders used at the same time seems like
> a rather unlikely use-case, so if such an optimization takes rather
> high complexity, its probably not worth going, IMHO.

Nah, it should be done separately because it needs some
code reshuffling that easily gets confusing, but I don't
think it will be hard.
I don't think it's a good idea though (well, it would
be somewhat nice to share the function to reduce code
size, but that is a kind of questionable benefit as well,
if you care about code size why do you build in both fixed
and float decoder in the first place?).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Reimar Döffinger 
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
>  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

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Hendrik Leppkes 
On Sun, Mar 13, 2016 at 5:24 PM, Ganesh Ajjanagadde  wrote:
> On Sat, Mar 12, 2016 at 1:24 PM, Reimar Döffinger
>  wrote:
>> Allows sharing and reusing the data between different files.
>>
>> Signed-off-by: Reimar Döffinger 
>> ---
>>  libavcodec/Makefile | 10 +-
>>  libavcodec/aacdec.c |  2 +-
>>  libavcodec/aacdec_fixed.c   |  6 +++---
>>  libavcodec/aacdec_template.c|  4 ++--
>>  libavcodec/cbrt_data.c  | 28 +++
>>  libavcodec/cbrt_data.h  | 38 
>> +
>>  libavcodec/cbrt_data_fixed.c| 29 
>>  libavcodec/cbrt_tablegen.h  | 18 --
>>  libavcodec/cbrt_tablegen_template.c |  8 ++--
>>  9 files changed, 116 insertions(+), 27 deletions(-)
>>  create mode 100644 libavcodec/cbrt_data.c
>>  create mode 100644 libavcodec/cbrt_data.h
>>  create mode 100644 libavcodec/cbrt_data_fixed.c
>>
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 1cd9572..6bb1af1 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -137,17 +137,17 @@ OBJS-$(CONFIG_A64MULTI_ENCODER)+= 
>> a64multienc.o elbg.o
>>  OBJS-$(CONFIG_A64MULTI5_ENCODER)   += a64multienc.o elbg.o
>>  OBJS-$(CONFIG_AAC_DECODER) += aacdec.o aactab.o aacsbr.o 
>> aacps_float.o \
>>aacadtsdec.o mpeg4audio.o 
>> kbdwin.o \
>> -  sbrdsp.o aacpsdsp_float.o
>> +  sbrdsp.o aacpsdsp_float.o 
>> cbrt_data.o
>>  OBJS-$(CONFIG_AAC_FIXED_DECODER)   += aacdec_fixed.o aactab.o 
>> aacsbr_fixed.o aacps_fixed.o \
>>aacadtsdec.o mpeg4audio.o 
>> kbdwin.o \
>> -  sbrdsp_fixed.o aacpsdsp_fixed.o
>> +  sbrdsp_fixed.o aacpsdsp_fixed.o 
>> cbrt_data_fixed.o
>>  OBJS-$(CONFIG_AAC_ENCODER) += aacenc.o aaccoder.o aacenctab.o   
>>  \
>>aacpsy.o aactab.o  \
>>aacenc_is.o \
>>aacenc_tns.o \
>>aacenc_ltp.o \
>>aacenc_pred.o \
>> -  psymodel.o mpeg4audio.o kbdwin.o
>> +  psymodel.o mpeg4audio.o kbdwin.o 
>> cbrt_data.o
>>  OBJS-$(CONFIG_AASC_DECODER)+= aasc.o msrledec.o
>>  OBJS-$(CONFIG_AC3_DECODER) += ac3dec_float.o ac3dec_data.o 
>> ac3.o kbdwin.o
>>  OBJS-$(CONFIG_AC3_FIXED_DECODER)   += ac3dec_fixed.o ac3dec_data.o 
>> ac3.o kbdwin.o
>> @@ -1017,8 +1017,8 @@ $(GEN_HEADERS): $(SUBDIR)%_tables.h: 
>> $(SUBDIR)%_tablegen$(HOSTEXESUF)
>> $(M)./$< > $@
>>
>>  ifdef CONFIG_HARDCODED_TABLES
>> -$(SUBDIR)aacdec.o: $(SUBDIR)cbrt_tables.h
>> -$(SUBDIR)aacdec_fixed.o: $(SUBDIR)cbrt_fixed_tables.h
>> +$(SUBDIR)cbrt_data.o: $(SUBDIR)cbrt_tables.h
>> +$(SUBDIR)cbrt_data_fixed.o: $(SUBDIR)cbrt_fixed_tables.h
>>  $(SUBDIR)aacps_float.o: $(SUBDIR)aacps_tables.h
>>  $(SUBDIR)aacps_fixed.o: $(SUBDIR)aacps_fixed_tables.h
>>  $(SUBDIR)aactab_fixed.o: $(SUBDIR)aac_fixed_tables.h
>> diff --git a/libavcodec/aacdec.c b/libavcodec/aacdec.c
>> index 26bdea1..ee9b4eb 100644
>> --- a/libavcodec/aacdec.c
>> +++ b/libavcodec/aacdec.c
>> @@ -50,7 +50,7 @@
>>  #include "aac.h"
>>  #include "aactab.h"
>>  #include "aacdectab.h"
>> -#include "cbrt_tablegen.h"
>> +#include "cbrt_data.h"
>>  #include "sbr.h"
>>  #include "aacsbr.h"
>>  #include "mpeg4audio.h"
>> diff --git a/libavcodec/aacdec_fixed.c b/libavcodec/aacdec_fixed.c
>> index 396a874..acb8178 100644
>> --- a/libavcodec/aacdec_fixed.c
>> +++ b/libavcodec/aacdec_fixed.c
>> @@ -75,7 +75,7 @@
>>  #include "aac.h"
>>  #include "aactab.h"
>>  #include "aacdectab.h"
>> -#include "cbrt_tablegen.h"
>> +#include "cbrt_data.h"
>>  #include "sbr.h"
>>  #include "aacsbr.h"
>>  #include "mpeg4audio.h"
>> @@ -155,9 +155,9 @@ static void vector_pow43(int *coefs, int len)
>>  for (i=0; i>  coef = coefs[i];
>>  if (coef < 0)
>> -coef = -(int)cbrt_tab[-coef];
>> +coef = -(int)ff_cbrt_tab_fixed[-coef];
>>  else
>> -coef = (int)cbrt_tab[coef];
>> +coef = (int)ff_cbrt_tab_fixed[coef];
>>  coefs[i] = coef;
>>  }
>>  }
>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>> index 6bc94c8..883ed52 100644
>> --- a/libavcodec/aacdec_template.c
>> +++ b/libavcodec/aacdec_template.c
>> @@ -1104,7 +1104,7 @@ static av_cold void aac_static_table_init(void)
>>  AAC_RENAME(ff_init_ff_sine_windows)( 9);
>>  AAC_RENAME(ff_init_ff_sine_windows)( 7);
>>

Re: [FFmpeg-devel] [PATCH] Move cbrt tables to a separate cbrt_data(_fixed).c files.

2016-03-13 Thread Ganesh Ajjanagadde 
On Sat, Mar 12, 2016 at 1:24 PM, Reimar Döffinger
 wrote:
> Allows sharing and reusing the data between different files.
>
> Signed-off-by: Reimar Döffinger 
> ---
>  libavcodec/Makefile | 10 +-
>  libavcodec/aacdec.c |  2 +-
>  libavcodec/aacdec_fixed.c   |  6 +++---
>  libavcodec/aacdec_template.c|  4 ++--
>  libavcodec/cbrt_data.c  | 28 +++
>  libavcodec/cbrt_data.h  | 38 
> +
>  libavcodec/cbrt_data_fixed.c| 29 
>  libavcodec/cbrt_tablegen.h  | 18 --
>  libavcodec/cbrt_tablegen_template.c |  8 ++--
>  9 files changed, 116 insertions(+), 27 deletions(-)
>  create mode 100644 libavcodec/cbrt_data.c
>  create mode 100644 libavcodec/cbrt_data.h
>  create mode 100644 libavcodec/cbrt_data_fixed.c
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 1cd9572..6bb1af1 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -137,17 +137,17 @@ OBJS-$(CONFIG_A64MULTI_ENCODER)+= a64multienc.o 
> elbg.o
>  OBJS-$(CONFIG_A64MULTI5_ENCODER)   += a64multienc.o elbg.o
>  OBJS-$(CONFIG_AAC_DECODER) += aacdec.o aactab.o aacsbr.o 
> aacps_float.o \
>aacadtsdec.o mpeg4audio.o kbdwin.o 
> \
> -  sbrdsp.o aacpsdsp_float.o
> +  sbrdsp.o aacpsdsp_float.o 
> cbrt_data.o
>  OBJS-$(CONFIG_AAC_FIXED_DECODER)   += aacdec_fixed.o aactab.o 
> aacsbr_fixed.o aacps_fixed.o \
>aacadtsdec.o mpeg4audio.o kbdwin.o 
> \
> -  sbrdsp_fixed.o aacpsdsp_fixed.o
> +  sbrdsp_fixed.o aacpsdsp_fixed.o 
> cbrt_data_fixed.o
>  OBJS-$(CONFIG_AAC_ENCODER) += aacenc.o aaccoder.o aacenctab.o
> \
>aacpsy.o aactab.o  \
>aacenc_is.o \
>aacenc_tns.o \
>aacenc_ltp.o \
>aacenc_pred.o \
> -  psymodel.o mpeg4audio.o kbdwin.o
> +  psymodel.o mpeg4audio.o kbdwin.o 
> cbrt_data.o
>  OBJS-$(CONFIG_AASC_DECODER)+= aasc.o msrledec.o
>  OBJS-$(CONFIG_AC3_DECODER) += ac3dec_float.o ac3dec_data.o ac3.o 
> kbdwin.o
>  OBJS-$(CONFIG_AC3_FIXED_DECODER)   += ac3dec_fixed.o ac3dec_data.o ac3.o 
> kbdwin.o
> @@ -1017,8 +1017,8 @@ $(GEN_HEADERS): $(SUBDIR)%_tables.h: 
> $(SUBDIR)%_tablegen$(HOSTEXESUF)
> $(M)./$< > $@
>
>  ifdef CONFIG_HARDCODED_TABLES
> -$(SUBDIR)aacdec.o: $(SUBDIR)cbrt_tables.h
> -$(SUBDIR)aacdec_fixed.o: $(SUBDIR)cbrt_fixed_tables.h
> +$(SUBDIR)cbrt_data.o: $(SUBDIR)cbrt_tables.h
> +$(SUBDIR)cbrt_data_fixed.o: $(SUBDIR)cbrt_fixed_tables.h
>  $(SUBDIR)aacps_float.o: $(SUBDIR)aacps_tables.h
>  $(SUBDIR)aacps_fixed.o: $(SUBDIR)aacps_fixed_tables.h
>  $(SUBDIR)aactab_fixed.o: $(SUBDIR)aac_fixed_tables.h
> diff --git a/libavcodec/aacdec.c b/libavcodec/aacdec.c
> index 26bdea1..ee9b4eb 100644
> --- a/libavcodec/aacdec.c
> +++ b/libavcodec/aacdec.c
> @@ -50,7 +50,7 @@
>  #include "aac.h"
>  #include "aactab.h"
>  #include "aacdectab.h"
> -#include "cbrt_tablegen.h"
> +#include "cbrt_data.h"
>  #include "sbr.h"
>  #include "aacsbr.h"
>  #include "mpeg4audio.h"
> diff --git a/libavcodec/aacdec_fixed.c b/libavcodec/aacdec_fixed.c
> index 396a874..acb8178 100644
> --- a/libavcodec/aacdec_fixed.c
> +++ b/libavcodec/aacdec_fixed.c
> @@ -75,7 +75,7 @@
>  #include "aac.h"
>  #include "aactab.h"
>  #include "aacdectab.h"
> -#include "cbrt_tablegen.h"
> +#include "cbrt_data.h"
>  #include "sbr.h"
>  #include "aacsbr.h"
>  #include "mpeg4audio.h"
> @@ -155,9 +155,9 @@ static void vector_pow43(int *coefs, int len)
>  for (i=0; i  coef = coefs[i];
>  if (coef < 0)
> -coef = -(int)cbrt_tab[-coef];
> +coef = -(int)ff_cbrt_tab_fixed[-coef];
>  else
> -coef = (int)cbrt_tab[coef];
> +coef = (int)ff_cbrt_tab_fixed[coef];
>  coefs[i] = coef;
>  }
>  }
> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index 6bc94c8..883ed52 100644
> --- a/libavcodec/aacdec_template.c
> +++ b/libavcodec/aacdec_template.c
> @@ -1104,7 +1104,7 @@ static av_cold void aac_static_table_init(void)
>  AAC_RENAME(ff_init_ff_sine_windows)( 9);
>  AAC_RENAME(ff_init_ff_sine_windows)( 7);
>
> -AAC_RENAME(cbrt_tableinit)();
> +AAC_RENAME(ff_cbrt_tableinit)();
>  }
>
>  static AVOnce aac_table_init = AV_ONCE_INIT;
> @@ -1795,7 +1795,7 @@ static int