[MPlayer-dev-eng] [PATCH] aspect and round params for dsize

[Attila Kinali]

On Sat, 9 Jul 2005 14:55:01 +0300
Oded Shimon <ods15 at ods15.dyndns.org> wrote:

> [dsize.patch  text/plain (5096 bytes)]
> Index: libmpcodecs/vf_dsize.c
> ===================================================================
> RCS file: /cvsroot/mplayer/main/libmpcodecs/vf_dsize.c,v
> retrieving revision 1.1
> diff -u -r1.1 vf_dsize.c
> --- libmpcodecs/vf_dsize.c	27 Apr 2003 18:55:04 -0000	1.1
> +++ libmpcodecs/vf_dsize.c	4 Jul 2005 09:01:25 -0000
> @@ -12,6 +12,7 @@
>  
>  struct vf_priv_s {
>  	int w, h;
> +	int a, r;

while w and h are imediatly clear in the vo context, what
are a and r ? Please add at least a comment here or even better
use longer variable names and comment them

>  	float aspect;
>  };
>  
> @@ -19,7 +20,27 @@
>  	int width, int height, int d_width, int d_height,
>  	unsigned int flags, unsigned int outfmt)
>  {
> -	if (vf->priv->w && vf->priv->h) {
> +	if (vf->priv->aspect < 0.001) {

Where does the value 0.001 come from ?
What do we exactly check here ?
Why isn't there any comment ?

> +		if (vf->priv->w == 0) vf->priv->w = d_width;
> +		if (vf->priv->h == 0) vf->priv->h = d_height;
> +		if (vf->priv->w == -1) vf->priv->w = width;
> +		if (vf->priv->h == -1) vf->priv->h = height;
> +		if (vf->priv->w == -2) vf->priv->w = vf->priv->h * (double)d_width/d_height;
> +		if (vf->priv->w == -3) vf->priv->w = vf->priv->h * (double)width / height;
> +		if (vf->priv->h == -2) vf->priv->h = vf->priv->w * (double)d_height / d_width;
> +		if (vf->priv->h == -3) vf->priv->h = vf->priv->w * (double)height / width;

What happens here if both h and w are negative ?

> +		if (vf->priv->a > -1) { // 0 -> downscale, 1-> upscale.  +2 -> original aspect.
> +			double aspect = (vf->priv->a & 2) ? ((double)d_height / d_width) : ((double)height / width);

write the condition as vf->priv->a !=0, the a&2 hardly readable

> +			if ((vf->priv->h > vf->priv->w * aspect) ^ (vf->priv->a & 1)) {

Use constructs like this in the ioccc but not in MPlayer.

> +				vf->priv->h = vf->priv->w * aspect;
> +			} else {
> +				vf->priv->w = vf->priv->h / aspect;
> +			}
> +		}
> +		if (vf->priv->r > 1) {
> +			vf->priv->w += (vf->priv->r - 1 - (vf->priv->w - 1) % vf->priv->r);
> +			vf->priv->h += (vf->priv->r - 1 - (vf->priv->h - 1) % vf->priv->r);

That code is fragile!
Module operation of negative numbers does not work 
on all architectures as expected.
And why do you always round down ?
Why isn't this mentioned in the manpage?

> +		}
>  		d_width = vf->priv->w;
>  		d_height = vf->priv->h;
>  	} else {
[...]

Over all: a bit too obfuscated for my taste. Learn that complicated
code doesn't let you look smarter. But it makes maintenance
of the project much harder.
You should also add many more comments on what's going on,
especialy if you are doing something not so obvious.
You should also learn to use C-code idioms a lot more, it
makes reading easier.

And while you are at it, could you add some doxygen docs for
the whole file? Thanks.


				Attila Kinali

-- 
郷に入れば郷に従え