[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Michael Niedermayer]

On Sun, Oct 26, 2008 at 03:41:09PM +0200, Kostya wrote:
[...]
> +
> +#define RV40_LUMA_LOOP_FIRST 13
> +static const RV40LoopFilterCond rv40_loop_cond_luma_first_row[RV40_LUMA_LOOP_FIRST] = {
> +    {  0,  4, 0, 0x0010, -1, -1, 0x0001,  0 }, // subblock 0
> +    {  0,  0, 1, 0x0001, -1,  2, 0x0008,  0 },
> +    {  0,  0, 0, 0x0001,  1, -1, 0x1000,  0 },
> +    {  0,  0, 1, 0x0001,  2, -1, 0x0008,  0 },
> +    {  4,  4, 0, 0x0020, -1, -1, 0x0002,  4 }, // subblocks 1-3
> +    {  4,  0, 1, 0x0002, -1, -1, 0x0001,  4 },
> +    {  4,  0, 0, 0x0002,  1, -1, 0x2000,  4 },
> +    {  8,  4, 0, 0x0040, -1, -1, 0x0004,  8 },
> +    {  8,  0, 1, 0x0004, -1, -1, 0x0002,  8 },
> +    {  8,  0, 0, 0x0004,  1, -1, 0x4000,  8 },
> +    { 12,  4, 0, 0x0080, -1, -1, 0x0008, 12 },
> +    { 12,  0, 1, 0x0008, -1, -1, 0x0004, 12 },
> +    { 12,  0, 0, 0x0008,  1, -1, 0x8000, 12 }
> +};
> +
> +#define RV40_LUMA_LOOP_NEXT 9
> +static const RV40LoopFilterCond rv40_loop_cond_luma_next_rows[RV40_LUMA_LOOP_NEXT] = {
> +    {  0,  4, 0, 0x0010, -1, -1, 0x0001, 0 }, // first subblock of the row
> +    {  0,  0, 1, 0x0001,  2, -1, 0x0008, 0 },
> +    {  0,  0, 1, 0x0001, -1,  2, 0x0008, 0 },
> +    {  4,  4, 0, 0x0020, -1, -1, 0x0002, 1 }, // the rest of subblocks
> +    {  4,  0, 1, 0x0002, -1, -1, 0x0001, 1 },
> +    {  8,  4, 0, 0x0040, -1, -1, 0x0004, 2 },
> +    {  8,  0, 1, 0x0004, -1, -1, 0x0002, 2 },
> +    { 12,  4, 0, 0x0080, -1, -1, 0x0008, 3 },
> +    { 12,  0, 1, 0x0008, -1, -1, 0x0004, 3 }
> +};
> +
> +#define RV40_CHROMA_LOOP 12
> +static const RV40LoopFilterCond rv40_loop_cond_chroma[RV40_CHROMA_LOOP] = {
> +    { 0, 4, 0, 0x04, -1, -1, 0x01, 0 }, // subblock 0
> +    { 0, 0, 1, 0x01, -1,  2, 0x02, 0 },
> +    { 0, 0, 0, 0x01,  1, -1, 0x04, 0 },
> +    { 0, 0, 1, 0x01,  2, -1, 0x02, 0 },
> +    { 4, 4, 0, 0x08, -1, -1, 0x02, 8 }, // subblock 1
> +    { 4, 4, 1, 0x02, -1, -1, 0x01, 0 },
> +    { 4, 4, 0, 0x02,  1, -1, 0x08, 8 },
> +    { 0, 8, 0, 0x10, -1, -1, 0x04, 0 }, // subblock 2
> +    { 0, 4, 1, 0x04, -1,  2, 0x08, 8 },
> +    { 0, 4, 1, 0x04,  2, -1, 0x08, 8 },
> +    { 4, 8, 0, 0x20, -1, -1, 0x08, 8 }, // subblock 3
> +    { 4, 4, 1, 0x08, -1, -1, 0x04, 8 },
> +};

have you confirmed that this loop filter is bit exact?
Id like to make really sure that this is correct before i spend time
on finding a clean implementation.

[...]
> +            if(s->width * s->height <= 0x6300){
> +                betaY += beta;
> +            }

0x6300= 176*144

[...]
> +                c_v_deblock[i] = ((uvcbp[0][i] << 1) & ~0x5) | (uvcbp[2][i] >> 1)
> +                               | (uvcbp[3][i] << 4) | uvcbp[0][i];
> +                c_h_deblock[i] = (uvcbp[3][i] << 4) | uvcbp[0][i] | (uvcbp[1][i] >> 2)
> +                               | (uvcbp[3][i] << 6) | (uvcbp[0][i] << 2);
> +                uvcbp[0][i] = (uvcbp[3][i] << 4) | uvcbp[0][i];

this can be simplified a lot, the term "(uvcbp[3][i] << 4) | uvcbp[0][i]"
occurs several times in there
besides id like to repeat that this should be using a 2d array NOT bits.
if you insist on using bits, then they MUST be documented, that is each bit
must be documented so its clear to which block or edge it belongs.


> +                if(!s->mb_x){
> +                    c_v_deblock[i] &= ~0x5;
> +                }
> +                if(!s->mb_y){
> +                    c_h_deblock[i] &= ~0x3;
> +                }

> +                if(s->mb_y == s->mb_height - 1 || mbtype[0] == 2 || mbtype[3] == 2){

mbtype should never be compared against litteral numbers, always
named values like from an enum.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081027/a752574f/attachment.pgp>