[MPlayer-dev-eng] [PATCH] view stereoscopic videos

Reimar Döffinger Reimar.Doeffinger at gmx.de
Mon Feb 1 21:10:19 CET 2010


On Mon, Feb 01, 2010 at 05:19:21PM +0100, Gordon Schmidt wrote:
> +#define ANA_R(RL,GL,BL,RR,GR,BR)    (av_clip_uint8(((vf->priv->ana_matrix[0]  * RL \
> +                   + vf->priv->ana_matrix[1]  * GL + vf->priv->ana_matrix[2]  * BL \
> +                   + vf->priv->ana_matrix[3]  * RR + vf->priv->ana_matrix[4]  * GR \
> +                   + vf->priv->ana_matrix[5]  * BR) >> 10)))

The outer () is pointless, instead you should put () around all arguments.
However this will look much nicer if you just make it a static inline function.

> +#define ANA_G(RL,GL,BL,RR,GR,BR)    (av_clip_uint8(((vf->priv->ana_matrix[6]  * RL \
> +                   + vf->priv->ana_matrix[7]  * GL + vf->priv->ana_matrix[8]  * BL \
> +                   + vf->priv->ana_matrix[9]  * RR + vf->priv->ana_matrix[10] * GR \
> +                   + vf->priv->ana_matrix[11] * BR) >> 10)))
> +
> +#define ANA_B(RL,GL,BL,RR,GR,BR)    (av_clip_uint8(((vf->priv->ana_matrix[12] * RL \
> +                   + vf->priv->ana_matrix[13] * GL + vf->priv->ana_matrix[14] * BL \
> +                   + vf->priv->ana_matrix[15] * RR + vf->priv->ana_matrix[16] * GR \
> +                   + vf->priv->ana_matrix[17] * BR) >> 10)))

Also looking at these, I think you should change
> +    int ana_matrix[18];
to
> + int ana_matrix[3][6]

Overall resulting in
static inline uint8_t ana_conv(int coeff[6], int rl, int gl, int bl, int rr, int gr, int gl)
{...}
ana_conv(vf->priv->ana_matrix[i], ...);

Also I suggest changing the shift from >> 10 to >> 16 (and adjusting the constants appropriately).
This will be faster on some architectures and also easier to accelerate with SIMD.

> +//==types==//
> +typedef enum stereo_code {
> +    NOT_SET,            //no value set - TODO: needs autodetection
> +    SIDE_BY_SIDE_LR,    //side by side parallel (left eye left, right eye right)
> +    SIDE_BY_SIDE_RL,    //side by side crosseye (right eye left, left eye right)
> +    ABOVE_BELOW_LR,     //above-below (left eye above, right eye below)
> +    ABOVE_BELOW_RL,     //above-below (right eye above, left eye below)
> +    ABOVE_BELOW_2_LR,   //above-below with half height resolution (left eye above, right eye below)
> +    ABOVE_BELOW_2_RL,   //above-below with half height resolution (right eye above, left eye below)
> +    ANAGLYPH_DUBOIS_RC, //color anaglyph red/cyan optimized with the least squares projection of dubois (red filter on left eye, cyan filter on right eye)
> +    ANAGLYPH_COLOR_RC,  //color anaglyph red/cyan (red filter on left eye, cyan filter on right eye)
> +    ANAGLYPH_HALF_RC,   //half color anaglyph red/cyan (red filter on left eye, cyan filter on right eye)
> +    ANAGLYPH_GRAY_RC,   //gray anaglyph red/cyan (red filter on left eye, cyan filter on right eye)
> +    ANAGLYPH_COLOR_GM,  //color anaglyph green/magenta (green filter on left eye, magenta filter on right eye)
> +    ANAGLYPH_HALF_GM,   //half color anaglyph green/magenta (green filter on left eye, magenta filter on right eye)
> +    ANAGLYPH_GRAY_GM,   //gray anaglyph green/magenta (green filter on left eye, magenta filter on right eye)
> +    ANAGLYPH_COLOR_YB,  //color anaglyph yellow/blue (yellow filter on left eye, blue filter on right eye)
> +    ANAGLYPH_HALF_YB,   //half color anaglyph yellow/blue (yellow filter on left eye, blue filter on right eye)
> +    ANAGLYPH_GRAY_YB,   //gray anaglyph yellow/blue (yellow filter on left eye, blue filter on right eye)
> +    MONO_L,             //mono output for debugging (left eye only)
> +    MONO_R              //mono output for debugging (right eye only)

Maybe move NOT_SET last or add a STEREO_CODE_COUNT last so you can verify
the command-line values?

> +    if ((0 != width % 4) && (0 != height % 4)) {

&& ?
Personally I consider
(width & 3) etc. more readable.

> +        //same as MONO_L only needs switching of input offsets 

trailing whitespace

> +    case ANAGLYPH_COLOR_RC:
> +        vf->priv->ana_matrix[0]     = 1024;
> +        vf->priv->ana_matrix[10]    = 1024;
> +        vf->priv->ana_matrix[17]    = 1024;
> +        break;
> +    case ANAGLYPH_HALF_RC:
> +        vf->priv->ana_matrix[0]     = 306;
> +        vf->priv->ana_matrix[1]     = 601;
> +        vf->priv->ana_matrix[2]     = 117;
> +        vf->priv->ana_matrix[10]    = 1024;
> +        vf->priv->ana_matrix[17]    = 1024;
> +        break;
> +    case ANAGLYPH_GRAY_RC:
> +        vf->priv->ana_matrix[0]     = 306;
> +        vf->priv->ana_matrix[1]     = 601;
> +        vf->priv->ana_matrix[2]     = 117;
> +        vf->priv->ana_matrix[9]     = 306;
> +        vf->priv->ana_matrix[10]    = 601;
> +        vf->priv->ana_matrix[11]    = 117;
> +        vf->priv->ana_matrix[15]    = 306;
> +        vf->priv->ana_matrix[16]    = 601;
> +        vf->priv->ana_matrix[17]    = 117;
> +        break;
> +    case ANAGLYPH_DUBOIS_RC:
> +        vf->priv->ana_matrix[0]     =  467;
> +        vf->priv->ana_matrix[1]     =  513;
> +        vf->priv->ana_matrix[2]     =  181;
> +        vf->priv->ana_matrix[3]     =  -45;
> +        vf->priv->ana_matrix[4]     =  -90;
> +        vf->priv->ana_matrix[5]     =   -2;
> +        vf->priv->ana_matrix[6]     =  -41;
> +        vf->priv->ana_matrix[7]     =  -39;
> +        vf->priv->ana_matrix[8]     =  -16;
> +        vf->priv->ana_matrix[9]     =  388;
> +        vf->priv->ana_matrix[10]    =  751;
> +        vf->priv->ana_matrix[11]    =  -19;
> +        vf->priv->ana_matrix[12]    =  -15;
> +        vf->priv->ana_matrix[13]    =  -21;
> +        vf->priv->ana_matrix[14]    =   -6;
> +        vf->priv->ana_matrix[15]    =  -74;
> +        vf->priv->ana_matrix[16]    = -116;
> +        vf->priv->ana_matrix[17]    = 1256;
> +        break;
> +    case ANAGLYPH_COLOR_GM:
> +        vf->priv->ana_matrix[3]     = 1024;
> +        vf->priv->ana_matrix[7]     = 1024;
> +        vf->priv->ana_matrix[17]    = 1024;
> +        break;
> +    case ANAGLYPH_HALF_GM:
> +        vf->priv->ana_matrix[3]     = 1024;
> +        vf->priv->ana_matrix[6]     = 306;
> +        vf->priv->ana_matrix[7]     = 601;
> +        vf->priv->ana_matrix[8]     = 117;
> +        vf->priv->ana_matrix[17]    = 1024;
> +        break;
> +    case ANAGLYPH_GRAY_GM:
> +        vf->priv->ana_matrix[3]     = 306;
> +        vf->priv->ana_matrix[4]     = 601;
> +        vf->priv->ana_matrix[5]     = 117;
> +        vf->priv->ana_matrix[6]     = 306;
> +        vf->priv->ana_matrix[7]     = 601;
> +        vf->priv->ana_matrix[8]     = 117;
> +        vf->priv->ana_matrix[15]    = 306;
> +        vf->priv->ana_matrix[16]    = 601;
> +        vf->priv->ana_matrix[17]    = 117;
> +        break;
> +    case ANAGLYPH_COLOR_YB:
> +        vf->priv->ana_matrix[3]     = 1024;
> +        vf->priv->ana_matrix[10]    = 1024;
> +        vf->priv->ana_matrix[14]    = 1024;
> +        break;
> +    case ANAGLYPH_HALF_YB:
> +        vf->priv->ana_matrix[3]     = 1024;
> +        vf->priv->ana_matrix[10]    = 1024;
> +        vf->priv->ana_matrix[12]    = 306;
> +        vf->priv->ana_matrix[13]    = 601;
> +        vf->priv->ana_matrix[14]    = 117;
> +        break;
> +    case ANAGLYPH_GRAY_YB:
> +        vf->priv->ana_matrix[3]     = 306;
> +        vf->priv->ana_matrix[4]     = 601;
> +        vf->priv->ana_matrix[5]     = 117;
> +        vf->priv->ana_matrix[9]     = 306;
> +        vf->priv->ana_matrix[10]    = 601;
> +        vf->priv->ana_matrix[11]    = 117;
> +        vf->priv->ana_matrix[12]    = 306;
> +        vf->priv->ana_matrix[13]    = 601;
> +        vf->priv->ana_matrix[14]    = 117;
> +        break;

I think it would be nicer if you declared those values as static const
tables and copied them over to ana_maatrix with memcpy.
Maybe a bit too hackish, but if you put the ANAGLYPH enums first you could
even do something like
memcpy(vf->priv->ana_matrix, ana_tables[format], sizeof(vf->priv->ana_matrix));
for all of those.

> +    if (vf->priv->in.fmt == vf->priv->out.fmt) { //nothing to do
> +        dmpi = mpi; 

Trailing whitespace.

> +                memcpy_pic(dmpi->planes[0] + vf->priv->out.off_left + (i * vf->priv->out.stride),  mpi->planes[0] + vf->priv->in.off_left,  3 * vf->priv->width, vf->priv->height / (vf->priv->in.hres * vf->priv->out.hres), vf->priv->out.stride * vf->priv->in.hres, vf->priv->in.stride * vf->priv->out.hres);
> +                memcpy_pic(dmpi->planes[0] + vf->priv->out.off_right + (i * vf->priv->out.stride), mpi->planes[0] + vf->priv->in.off_right, 3 * vf->priv->width, vf->priv->height / (vf->priv->in.hres * vf->priv->out.hres), vf->priv->out.stride * vf->priv->in.hres, vf->priv->in.stride * vf->priv->out.hres);

Those lines are faaar to long, where reasonably possible keep them below 80 characters
per line, and about 100 at the very most in general.

> +                    dmpi->planes[0][o + 1]  = ANA_G(rl, gl, bl, rr, gr, br);
> +                    dmpi->planes[0][o + 2]  = ANA_B(rl, gl, bl, rr, gr, br);
> +                    il  += 3;
> +                    ir  += 3;
> +                    o   += 3;

One space more than necessary for aligning the +=?

> +        if ((0 == strcmp( arg, "sbsl")) || (0 == strcmp(arg, "side_by_side_left_first"))) {
> +            return_value = SIDE_BY_SIDE_LR;
> +        } else if((0 == strcmp(arg, "sbsr")) || (0 == strcmp(arg, "side_by_side_right_first"))) {
> +            return_value = SIDE_BY_SIDE_RL;
> +        } else if((0 == strcmp(arg, "abl")) || (0 == strcmp(arg, "above_below_left_first"))) {
> +            return_value = ABOVE_BELOW_LR;
> +        } else if((0 == strcmp(arg, "abr")) || (0 == strcmp(arg, "above_below_right_first"))) {
> +            return_value = ABOVE_BELOW_RL;
> +        } else if((0 == strcmp(arg, "ab2l")) || (0 == strcmp(arg, "above_below_half_height_left_first"))) {
> +            return_value = ABOVE_BELOW_2_LR;
> +        } else if((0 == strcmp(arg, "ab2r")) || (0 == strcmp(arg, "above_below_half_height_right_first"))) {
> +            return_value = ABOVE_BELOW_2_RL;
> +        } else if((0 == strcmp(arg, "acrc")) || (0 == strcmp(arg, "anaglyph_color_red_cyan"))) {
> +            return_value = ANAGLYPH_COLOR_RC;
> +        } else if((0 == strcmp(arg, "ahrc")) || (0 == strcmp(arg, "anaglyph_half_color_red_cyan"))) {
> +            return_value = ANAGLYPH_HALF_RC;
> +        } else if((0 == strcmp(arg, "agrc")) || (0 == strcmp(arg, "anaglyph_gray_red_cyan"))) {
> +            return_value = ANAGLYPH_GRAY_RC;
> +        } else if((0 == strcmp(arg, "adrc")) || (0 == strcmp(arg, "anaglyph_dubois_red_cyan"))) {
> +            return_value = ANAGLYPH_DUBOIS_RC;
> +        } else if((0 == strcmp(arg, "acgm")) || (0 == strcmp(arg, "anaglyph_color_green_magenta"))) {
> +            return_value = ANAGLYPH_COLOR_GM;
> +        } else if((0 == strcmp(arg, "ahgm")) || (0 == strcmp(arg, "anaglyph_half_color_green_magenta"))) {
> +            return_value = ANAGLYPH_HALF_GM;
> +        } else if((0 == strcmp(arg, "aggm")) || (0 == strcmp(arg, "anaglyph_gray_green_magenta"))) {
> +            return_value = ANAGLYPH_GRAY_GM;
> +        } else if((0 == strcmp(arg, "acyb")) || (0 == strcmp(arg, "anaglyph_color_yellow_blue"))) {
> +            return_value = ANAGLYPH_COLOR_YB;
> +        } else if((0 == strcmp(arg, "ahyb")) || (0 == strcmp(arg, "anaglyph_half_color_yellow_blue"))) {
> +            return_value = ANAGLYPH_HALF_YB;
> +        } else if((0 == strcmp(arg, "agyb")) || (0 == strcmp(arg, "anaglyph_gray_yellow_blue"))) {
> +            return_value = ANAGLYPH_GRAY_YB;
> +        } else if((0 == strcmp(arg, "ml")) || (0 == strcmp(arg, "mono_left"))) {
> +            return_value = MONO_L;
> +        } else if((0 == strcmp(arg, "mr")) || (0 == strcmp(arg, "mono_right"))) {
> +            return_value = MONO_R;
> +        }

That's far too much code.
Use a table + loop to do the string -> number conversion.
Has also the advantage that it's easy to reuse it if you ever want to do
the conversion the other way round.

> +    //uninit priv    
> +    if (vf->priv)
> +    {
> +        free(vf->priv);
> +    }

The if is completely unnecessary.

> +    vf->priv = malloc(sizeof(struct vf_priv_s));
> +    if (!vf->priv) {
> +        return 0;
> +    }
> +    memset(&vf->priv->ana_matrix[0], 0, sizeof(vf->priv->ana_matrix));

Use calloc and everything will be initialized to 0 without extra code.

> +  	vf->query_format    = query_format;

There's a tab.

> +    if (args) {
> +        char *arg;
> +        //input code
> +        arg = strtok(args, ": \0\n");
> +        vf->priv->in.fmt = get_arg(arg);
> +        //output code
> +        arg = strtok(NULL, ": \0\n");
> +        vf->priv->out.fmt = get_arg(arg);
> +    }

Look at e.g. vf_scale.c and there the vf_opts_fields
for how to reuse the parsing code MPlayer already has.
Actually vf_format is probably the simples example,
it contains almost no other code.
By adding m_struct_t to vf_info_t you also don't need
to alloc and initialize vf->priv manually.

> +anaglyph color red/cyan optimized with the least squares projection of dubois (red filter on left eye, cyan filter on right eye)    

Lots of trailing whitespace on this and other lines.
Also, for the man page I think you're supposed to stay below
70 characters per line or something like that.
While I could probably go on for a while reviewing before it's
perfect, I think after this pass it's probably good enough to commit.



More information about the MPlayer-dev-eng mailing list