[FFmpeg-devel] a64 encoder

[Bitbreaker/METALVOTZE]

[lot of things i gonna fix]
> Those are not local (no "static"), and also using global vars in general 
> is not thread safe (unless it is written just once, in initialization). 
> It is better to move them to A64Context.
>   
I thought of doing so already, but was not sure if it is okay to have 
all the context allocated with all arrays needed for all modes, though 
only a few arrays are needed for a single mode at a time.
>> +    c->multicol_cs_lifetime=4;
>>     
>
> This is constant, so using a #define would simplify the code.
>   
Indeed this is constant yet, but i intend to have it as a variable, as 
soon as i am more into ffmpeg and would spend it a respective option on 
the commandline then, it is lets you choose for how many frames the set 
of blocks is valid, thus giving a chance to influence quality/size.
>> +    c->multicol_dithersteps=MULTICOL_DITHERSTEPS;
>>     
>
> Same thing here, could just use MULTICOL_DITHERSTEPS everywhere.
>   
Same as above, different dithertables would be a nice future option to 
have, but yet it is of course still static, as only one table is used.
> c->multicol_5col = avctx->codec->id==CODEC_ID_A64_MULTI5;
>   
Right :-)
> If c->multicol_cs_lifetime is just a constant, you could put all this in 
> A64Context by
>
> typedef struct A64Context {
> (...)
>      int multicol_charmaps[0x400*MULTICOL_CS_LIFETIME];
>      uint8_t multicol_bitmap[64000*MULTICOL_CS_LIFETIME];
> etc
> (...)
> }
>
> this avoid all the need for alloc'ing/freeing.
>   
sure, but see future plans above, of having lifetime being chosen by the 
user.
>> +        memset(multicol_charmaps,0,0x400*c->multicol_cs_lifetime*sizeof(int));
>> +        memset(multicol_bitmap,0,64000*c->multicol_cs_lifetime*sizeof(uint8_t));
>> +        memset(multicol_charset,0,0x800*sizeof(uint8_t));
>> +        memset(multicol_resolve_map,0,1000*c->multicol_cs_lifetime*sizeof(int));
>> +        memset(multicol_charset_stats,0,1000*c->multicol_cs_lifetime*sizeof(int));
>> +        memset(meta_charset,0,32000*c->multicol_cs_lifetime*sizeof(uint8_t));
>> +        memset(multicol_colram_map,0,1000*c->multicol_cs_lifetime*sizeof(int));
>>     
> Are you sure all those memsets are really needed? Also using something like
> memset(multicol_charmaps,0,0x400*c->multicol_cs_lifetime*sizeof(*multicol_charmaps));
> avoid introducing bugs if you ever change the type of multicol_charmaps...
>   
Probably not all, but for sure for charset_stat, colram_map and 
resolve_map. Gotta have a look on that.
>> +static void find_favourite(int charpos, int lastchar, uint32_t* diff, uint32_t* best) {
>> +    uint32_t a;
>> +    uint32_t delta;
>> +    uint32_t x;
>>     
>
> Here you could use just int (it will use just as much mem, since it will 
>   be mapped to registers anyway). Also, C99 warrants that int has at 
> least 32 bits.
>   
It is part of the very inner loop and rather time consuming, if i am 
right it turned out to be faster when i was just using unsigned int over 
the whole calculations in there.

Toby