[FFmpeg-devel] [PATCH] RoQ video encoder, take 2

[Vitor]

Hi,

Reimar D?ffinger wrote:
> Hello,
> On Sun, May 27, 2007 at 12:21:03PM +0200, Vitor wrote:
>   
>> +#define SQUARE(x) ((x)*(x))
>>     
>
> I think you are using this always on the same kinds of data, so I'd
> consider making this a static inline function, at low optimization
> levels this would avoid evaluating x twice.
>
>   

agreed.

>> +typedef struct
>> +{
>> +    roq_cell block[4];
>> +} roq_cell4;
>>     
>
> Hmm.. Did you compare this both in code complexity and speed to just
> using roq_cell *a or roq_cell a[4]?
>
>   

It's a leftover of the library in which it is based... I guess your 
suggestion will reduce code complexity (this structure is not very speed 
critical).

>> +typedef struct
>> +{
>> +    uint32_t eval_se[4];
>> +
>> +    uint8_t subCels[4];
>> +
>> +    int8_t motionX, motionY;
>> +    uint8_t cbEntry;
>> +} roq_subcel_evaluation_t;
>> +
>> +typedef struct
>> +{
>> +    uint32_t eval_se[3];
>> +
>> +    roq_subcel_evaluation_t subCels[4];
>> +
>> +    int8_t motionX, motionY;
>> +    uint8_t cbEntry;
>> +
>> +    uint32_t sourceX, sourceY;
>> +} cel_evaluation_t;
>>     
>
> Unless there is a special reason not to, try using just "int" instead of
> "int8_t".
>   
[...]
>> +    }
>>     
>
> Lots of useless (), I also think you should probably use
> avcodec_check_dimensions somewhere.
>
> [...]
>   

Agreed with all those suggestions

>> +#define get_word(in_buffer) ((unsigned short)(in_buffer += 2, \
>> +  (in_buffer[-1] << 8 | in_buffer[-2])))
>> +#define get_long(in_buffer) ((unsigned long)(in_buffer += 4, \
>> +  (in_buffer[-1] << 24 | in_buffer[-2] << 16 | in_buffer[-3] << 8 | in_buffer[-4])))
>>     
>
> I don't think these are valid C.
> Are these so speed critical that you can't use bytestream functions?
>
>   

It was there before my patch (actually, not really, as I forgot to fetch 
the latest SVN version. Ramiro changed that before me.

>> +    for(i = -512; i < 512; i++)
>> +        s->uiclp[i] = (i < 0 ? 0 : (i > 255 ? 255 : i));
>>     
>
> av_clip_uint8 ?
>
> Hmm... these seem to be there in the current code already though...
>
>   

It was there before, but using av_clip_uint8 is a good idea, as it will 
avoid the hackish addition of uiclp in RoqContext...

> [...]
>   
>> +    AVFrame *last_frame;
>> +    AVFrame *current_frame;
>> +
>> +    uint8_t **last_frame_data;
>> +    uint8_t **current_frame_data;
>>     
>
> Are these two really needed, can't you just use the AVFrames above as in
> the decoder?
> At least they should be uint8_t *last_frame_data[3]; IMO, thus avoiding
> an extra malloc and indirection step on access. I don't think it matters
> much if you copy 4/8 or 12/24 bytes on swapping them.
>
>   

Well, I have to put them because I use the apply_motion_xx and 
apply_vector_xx in the encoder, where the last frame is reconstructed. 
But I agree with using *last_frame_data[3].

-Vitor