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

[Vitor]

Hi

Michael Niedermayer wrote:
> Hi
>   

I've followed your suggestion in the message 
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-June/030406.html
and it did decrease the complexity of the patch.

I think that this encoder is still a bit verbose compared to the average 
ffmpeg encoder (it goes through all the blocks to do motion detection, 
then go through all of it again to find best codebook, etc), but fixing 
this will not give a noticeable speed gain nor make the code simpler 
(just smaller) so I don't think it is worth the work.

(not top-posting, comments follow)...

> On Fri, Jun 08, 2007 at 10:34:09PM +0200, Vitor wrote:
>   
>> Hi,
>>
>> I'm sending the new version of the RoQ patch. Most of the changes were 
>> made by Eric Lasota, I made mostly minor changes. It's now using 
>> properly the YUV444 colorspace and encode properly the MOT blocks.
>>
>> Comments welcome.
>>
>> -Vitor
>>     

[...]

>> +static void enlarge_roq_mb4(uint8_t base[3][16], uint8_t u[3][64])
>> +{
>> +    int x,y,cp;
>> +
>> +    for(y=0;y<8;y++)
>> +        for(x=0;x<8;x++)
>> +            for(cp=0;cp<3;cp++)
>> +                u[cp][y*8+x] = base[cp][(y/2)*4 + (x/2)];
>>     
>
> the for(cp loop should be the outermost IMHO
> also
>
> enlarge_roq_mb4(uint8_t base[3][4][4], uint8_t u[3][8][8])
>
> u[cp][y][x] = base[cp][y>>1][x>>1];
> seems more readable
>
>   

I don't know if I understand your suggestion, but if I do, I really 
don't like passing a [3][16] vector to a function that expects a 
[3][4][4]. I agree it is more readable, but every time I look at it I 
think I spotted an error...

>> +
>> +/**
>> + * Template code to find the codebook with the lowest distortion from an image
>> + */
>> +#define GET_LOWEST_CB_ERR(FUNCT, DIM, ERR_COMMAND) \
>> +static int FUNCT(uint8_t cluster[3][DIM], uint8_t cb[][3][DIM], int numCB, int *outIndex) \
>> +{ \
>> +    int diff; \
>> +    int pick=0, lDiff; \
>> +    int i=0; \
>> +\
>> +    lDiff = INT_MAX; \
>> +\
>> +    /* Diff against the others */ \
>> +    for (i=0; i<numCB; i++) { \
>> +        diff = ERR_COMMAND \
>> +        if (diff < lDiff) { \
>> +            lDiff = diff; \
>> +            pick = i; \
>> +        } \
>> +    } \
>> +\
>> +    *outIndex = pick; \
>> +    return lDiff; \
>> +}
>> +
>> +
>> +GET_LOWEST_CB_ERR(index_mb2,  4, squared_diff_mb2(cluster, cb[i]);)
>> +GET_LOWEST_CB_ERR(index_mb4, 16, squared_diff_mb4(cluster, cb[i]);)
>> +GET_LOWEST_CB_ERR(index_mb8, 64, squared_diff_mb8(cluster, cb[i]);)
>>     
>
> same question here, why is this not a normal funcion?
>
>   

I can't see a good way to avoid passing the parameter cb[][3*DIM] 
without making cb a one-dimensional vector (which I think is less readable).

And the rest is changed in the new patch, or do not apply anymore after 
the changes...

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: roq_video4.diff
Type: text/x-patch
Size: 38271 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070613/d73fd012/attachment.bin>