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

[Vitor]

Hi

Michael Niedermayer wrote:
> Hi
>
>   
[...]

>> +#define CHROMA_BIAS 2
>>     
>
> why 2?
> does it look better? or is it just because it gives chroma in 4:4:4 the same
> weight as luma?
>
>   

It just tried to find what looked better. But there was a bug in the
code, now it is kind of irrelevant (I can't see any difference in
quality varying this parameter).

> [...]
>   
>> +static int eval_motion_dist(RoqContext *enc, int x, int y, int mx, int my,
>> +                             int size)
>> +{
>> +    if (mx < -7 || mx > 7)
>> +        return INT_MAX;
>> +
>> +    if (my < -7 || my > 7)
>> +        return INT_MAX;
>> +
>> +    if ((x + mx - size < 0) || (x + mx > enc->width - size))
>> +        return INT_MAX;
>> +
>> +    if ((y + my - size < 0) || (y + my > enc->height - size))
>> +        return INT_MAX;
>>     
>
> is (y + my - size < 0) correct? shouldnt it be y + my < 0
>
>
>   
>> +
>> +
>> +    return block_sse(enc->frame_to_enc->data, enc->last_frame->data, x, y,
>> +                     x+mx, y+my, enc->y_stride, size);
>> +}
>>     
>
>   
[...]

> also block_sse could be split like:
>
>
>               block_sse(dst[0], src[0], x, y, mx, my, enc->y_stride, size)
> + CHROMA_BIAS*block_sse(dst[1], src[1], x, y, mx, my, enc->c_stride, size)
> + CHROMA_BIAS*block_sse(dst[2], src[2], x, y, mx, my, enc->c_stride, size);
>
>
>   

I took the liberty to ignore this suggestion as the function as is
useful to calculate the error for skipping a block

> [...]
>   
>> +/**
>> + * 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...

>> +#define EVAL_MOTION(MOTION) \
>> +    do { \
>> +        diff = eval_motion_dist(enc, j, i, (MOTION).d[0], \
>> +                                 (MOTION).d[1], blocksize); \
>> +            \
>> +        if (diff < lowestdiff) { \
>> +            lowestdiff = diff; \
>> +            bestpick = MOTION; \
>> +        } \
>> +    } while(0)
>>     
>
> this can be an inline function
>
>   

Do you think it is worth to have a function that gets 7 parameters for 4
lines of code?

[...]
>> +
>> +
>> +
>> +#define SPOOL_TYPECODE(type)        \
>> +do {\
>> +    int a;\
>> +    typeSpool |= ((type) & 3) << (14 - typeSpoolLength);\
>> +    typeSpoolLength += 2;\
>> +    if (typeSpoolLength == 16) {\
>> +        bytestream_put_le16(&enc->out_buf, typeSpool); \
>> +        for (a=0; a<argumentSpoolLength; a++)\
>> +            bytestream_put_byte(&enc->out_buf, argumentSpool[a]); \
>> +        typeSpoolLength = 0;\
>> +        typeSpool = 0;\
>> +        argumentSpoolLength = 0;\
>> +    }\
>> +} while(0)
>>     
>
> instead of this complicated fifo spool thingy, why not
>
> use
> bytestream_put_byte(&out, arg); instead of
> SPOOL_ARGUMENT(arg)
>
> and
>
> write_typecode(){
>     spool |= (type & 3) << (14 - spool_length);
>     spool_length += 2;
>     if (spool_length == 16){
>         AV_WL16(typep, spool);
>         spool= 0;
>         typep= out;
>         out+=2;
>         spool_length=0;
>     }
> }
>   

I didn't do exactly as you suggested. If you still think it is still not
ok, I'll try to polish it some more.

>> +typedef struct {
>> +    int d[2];
>> +} motion_vect;
>>     
>
> why not drop this struct and use int [2] directly?
>   

For being able do do motion1=motion2;

>> +    double lambda;
>>     
>
> floating point code is very very bad as it means that the output is not binary
> identical between architectures or compilers so regression tests wouldnt be
> possible
>   

I understand. I changed that, I don't know if in the usual way.

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: roqvideo5.diff
Type: text/x-patch
Size: 37362 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070620/80286485/attachment.bin>