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

[Reimar Döffinger]

On Tue, Feb 02, 2010 at 03:23:36PM +0100, Gordon Schmidt wrote:
> >&& ?
> >Personally I consider
> >(width & 3) etc. more readable.
> what's with the logical AND?

You only fail if _both_ are not divisible by 2, is that really
intentional?
I assumed you meant to check if either is not divisible by 2.

> On Mon, Feb 02, 2010 at 01:15:21PM +0100, Diego Biurrun wrote:
> >Get rid of all tabs and trailing whitespace, break those long
> >lines please.
> I hope i've found every mistake.

You actually added some more trailing whitespace while breaking the lines :-)

> +static const char* stereo_code_strings[STEREO_CODE_COUNT][2] = {

static const char * const stereo_code_strings

> +    {"arcg", "anaglyph_red_cyan_gray"},             //ANAGLYPH_RC_GRAY

The comments are somewhat ugly and unreliable.
You could write it as
[ANAGLYPH_RC_GRAY] = {"arcg", "anaglyph_red_cyan_gray"},
instead...

> +static inline uint8_t ana_convert(int coeff[6], int rl, int gl, int bl,
> +    int rr, int gr, int br)

You could align those below the rl etc.

> +{
> +    return av_clip_uint8(((coeff[0] * rl + coeff[1] * gl + coeff[2] * bl
> +         + coeff[3] * rr + coeff[4] * gr + coeff[5] * br) >> 16));

And use a temporary variable for better readability, something like
int tmp;
tmp  = coeff[0] * rl;
tmp += coeff[1] * gl;

> +                for (x = 0; x < vf->priv->out.width; x++) {
> +                    rl  = mpi->planes[0][il    ];
> +                    gl  = mpi->planes[0][il + 1];
> +                    bl  = mpi->planes[0][il + 2];
> +                    rr  = mpi->planes[0][ir    ];
> +                    gr  = mpi->planes[0][ir + 1];
> +                    br  = mpi->planes[0][ir + 2];
> +                    dmpi->planes[0][o    ]  = ana_convert(
> +                        vf->priv->ana_matrix[0], rl, gl, bl, rr, gr, br);

Or you could even go one step further and do something along the lines of (probably
not 100% correct):
> static inline uint8_t ana_convert(int coeff[6], uint8_t left[3], uint8_t right[3])
> {
>    int tmp;
>    tmp = coeff[0] * left[0] + coeff[3] * right[0];
...
And call that as
> ana_convert(vf->priv->ana_matrix[0], mpi->planes[0] + il, mpi->planes[0] + ir);

(I admit that will make it more annoying to re-add YUV support if ever wanted,
but for speed reasons I doubt that should use the same ana_convert function anyway).