[FFmpeg-devel] a64 encoder

Vitor Sessak vitor1001
Thu Jan 15 21:26:43 CET 2009


Bitbreaker/METALVOTZE wrote:
> [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.

Indeed it is not really clean, but non thread-safe code is not acceptable.

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

I think you are looking for AVFrame.quality and "-qscale <num>".

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

I was giving a second look at your patch and I think that the whole 
process of review might be more smooth if you first send a patch to add 
just one of the six encoders (the simplest one) and after getting it 
approved send the others, one per patch. In general, smaller patches are 
reviewed way faster...

-Vitor




More information about the ffmpeg-devel mailing list