[FFmpeg-devel] [PATCH] RoQ video encoder (take 4)

[Vitor]

Hi

Michael Niedermayer wrote:
[...]

>>>> +/**
>>>> + * Get macroblocks from parts of the image
>>>> + */
>>>> +static void get_frame_mb(AVFrame *frame, int x, int y, uint8_t mb[], int 
>>>> dim)
>>>> +{
>>>> +    int i, j, cp;
>>>> +    int stride = frame->linesize[0];
>>>> +
>>>> +    for (i=0; i<dim; i++)
>>>> +        for (j=0; j<dim; j++)
>>>> +            for (cp=0; cp<3; cp++)
>>>> +                mb[i*3*dim+j*3+cp] = frame->data[cp][(y+i)*stride+x+j];
>>>> +}
>>>>    
>>> why is the planar data converted to packed?
>>>  
>> It simplified some code...
> 
> which code?
> also a 3 byte compare where 2 are weighted by CHROMA_BIAS is very SIMD
> unfriendly (=slow)

Agreed. Converted to planar.

[...]

>> +/**
>> + * Returns distortion between two macroblocks
>> + */
>> +static inline int squared_diff_macroblock(uint8_t a[], uint8_t b[], int size)
>> +{
>> +    int sdiff=0;
>> +    int n, cp, bias;
>> +
>> +    for(cp=0;cp<3;cp++) {
>> +        bias = (cp ? CHROMA_BIAS : 1);
>> +        for(n=0;n<size*size;n++)
>> +            sdiff += (bias*square(b[cp+3*n] - a[cp+3*n]) + 2)/4;
>> +    }
>> +
>> +    return sdiff;
>> +}
> 
> duplicate of block_sse()

I didn't find a nice way to avoid duplication (one gets uint_8 ** as a 
parameter while the other a uint8_t []), so at least I tried to reuse 
code...

> 
> 
>> +
>> +typedef struct
>> +{
>> +    int eval_dist[4];
>> +    int best_bit_use;
>> +    int best_coding;
>> +
>> +    int subCels[4];
>> +
>> +    int motionX, motionY;
>> +    int cbEntry;
>> +} subcel_evaluation_t;
>> +
>> +typedef struct
>> +{
>> +    int eval_dist[4];
>> +    int best_coding;
>> +
>> +    subcel_evaluation_t subCels[4];
>> +
>> +    int motionX, motionY;
>> +    int cbEntry;
>> +
>> +    int sourceX, sourceY;
>> +} cel_evaluation_t;
> 
> *X/*Y could be int[2] or motion_vect
> 
> also iam tempted to suggest to merge these 2 structs, they are pretty much the
> same, but maybe it cant be done cleanly?

I didn't found a great way of doing it...

[...]

>> +/**
>> + * Gets distortion for all options available to a subcel
>> + */
>> +static void gather_data_for_subcel(subcel_evaluation_t *subcel, int x,
>> +                                   int y, RoqContext *enc, roq_tempdata_t *tempData)
>> +{
>> +    uint8_t mb4[4*4*3];
>> +    uint8_t mb2[2*2*3];
>> +    int cluster_index;
>> +    int i, best_dist;
>> +
>> +    static const int bitsUsed[4] = {2, 10, 10, 34};
>> +
>> +    if (enc->framesSinceKeyframe >= 1) {
>> +        subcel->motionX = enc->this_motion4[y*enc->width/16 + x/4].d[0];
>> +        subcel->motionY = enc->this_motion4[y*enc->width/16 + x/4].d[1];
>> +
>> +        subcel->eval_dist[RoQ_ID_FCC] =
>> +            eval_motion_dist(enc, x, y,
>> +                             enc->this_motion4[y*enc->width/16 + x/4].d[0],
>> +                             enc->this_motion4[y*enc->width/16 + x/4].d[1], 4);
>> +    } else
>> +        subcel->eval_dist[RoQ_ID_FCC] = INT_MAX;
> 
> maybe setting all eval_dist to INT_MAX at some central place would look
> simpler then these else cases

When I read you comment it seemed a good idea, but looking at the code I 
realized it won't simplify it.

I've changed as suggested the rest. I've added also a commented debug 
code. If it is a problem, I can remove it.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: roq_video6.diff
Type: text/x-patch
Size: 36310 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070624/3ddb71b2/attachment.bin>