[FFmpeg-devel] [PATCH] cabac: initialize all of ff_h264_cabac_tables programmatically.

[Reimar Döffinger]

On 30.08.2014, at 19:01, wm4 <nfxjfg at googlemail.com> wrote:
> On Sat, 30 Aug 2014 18:41:52 +0200
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
>> On 30.08.2014, at 18:29, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>>> On Sat, Aug 30, 2014 at 06:20:56PM +0200, wm4 wrote:
>>>> On Sat, 30 Aug 2014 18:18:41 +0200
>>>> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>>>> 
>>>>> Moves it from .data to .bss, slightly reducing binary size.
>>>>> 
>>>>> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>>>>> ---
>>>>> libavcodec/cabac.c | 26 ++++----------------------
>>>>> 1 file changed, 4 insertions(+), 22 deletions(-)
>>>>> 
>>>>> diff --git a/libavcodec/cabac.c b/libavcodec/cabac.c
>>>>> index 65579d8..803c81c 100644
>>>>> --- a/libavcodec/cabac.c
>>>>> +++ b/libavcodec/cabac.c
>>>>> @@ -32,28 +32,7 @@
>>>>> #include "cabac.h"
>>>>> #include "cabac_functions.h"
>>>>> 
>>>>> -uint8_t ff_h264_cabac_tables[512 + 4*2*64 + 4*64 + 63] = {
>>>>> - 9,8,7,7,6,6,6,6,5,5,5,5,5,5,5,5,
>>>>> - 4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,
>>>>> - 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,
>>>>> - 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,
>>>>> - 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,
>>>>> - 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,
>>>>> - 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,
>>>>> - 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,
>>>>> - 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
>>>>> - 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
>>>>> - 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
>>>>> - 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
>>>>> -};
>>>>> +uint8_t ff_h264_cabac_tables[512 + 4*2*64 + 4*64 + 63];
>>>>> 
>>>>> static const uint8_t lps_range[64][4]= {
>>>>> {128,176,208,240}, {128,167,197,227}, {128,158,187,216}, {123,150,178,205},
>>>>> @@ -143,6 +122,9 @@ void ff_init_cabac_states(void)
>>>>>    if (initialized)
>>>>>        return;
>>>>> 
>>>>> +    for (i = 0; i < 512; i++)
>>>>> +        ff_h264_norm_shift[i] = i ? 8 - av_log2(i) : 9;
>>>>> +
>>>>>    for(i=0; i<64; i++){
>>>>>        for(j=0; j<4; j++){ //FIXME check if this is worth the 1 shift we save
>>>>>            ff_h264_lps_range[j*2*64+2*i+0]=
>>>> 
>>>> Doesn't this make library safety issues worse, instead of improving it?
>>> 
>>> Why would it make anything even a slightest bit worse than it is now?
>>> Also, it makes it easier to make this hardcoded, which I think solves
>>> your concern?
>> 
>> Just to be clear since it's not obvious: the lines just below what I added initialise the second half of the array.
>> I just added that the first half is initialised in the same place.
> 
> I have no strong feelings about it, but adding even more global mutable
> state feels wrong. Yes, I know there are many places like this etc.,
> so your patch is pretty innocent,

I don't consider it more mutable state, as said the array already is initialised at runtime.
Though I guess it would be more if you count by-wise (though mutable is still kind of the wrong word, not changing it doesn't make it const)

> but still I wonder why you'd optimize
> for binary size, in exchange for higher RAM usage. What is your
> motivation for doing this?

The only higher RAM usage would be when you actually use H.264, and it would be from the two additional lines of code.
When you are not using H.264 the RAM usage (with some caveats on what the linker does) will be several 100 bytes lower.
My motivation for it is that I see _no advantage whatsoever_ speaking for the current code, and a few ones, including code clarify speaking for my code.
Though the biggest one maybe being that I want to move it to tablegen and wanted to clean up the code first.
Sorry, but to me the current code is just "obviously stupid" (no offense meant), I so far don't really understand what your concerns are with a change like that (moving things from .data - not .rodata - to .bss)...