[MPlayer-dev-eng] [PATCH] XviD 1.1.x support

[Reimar Döffinger]

Hi,

>>When I have more time, I'll send the last blast of patches. Hopefully,
>>given that those will not be cosmetics, they should be easier to get
>>accepted by the dev crowd. ;-)
>>
>>But please, I you, or Ivan think that the job should stay like this
>>half-finished (and so close to the goal!), I can get out of the way, you
>>know! ;-)
> 
> So here is the patch that features a clean support for XviD 1.1.x
> (decoder and encoder).
> 
> Any comments welcome!

A few tiny things...

> -
> -	if(!mpcodecs_config_vo(sh, sh->disp_w, sh->disp_h, IMGFMT_YV12))
> -		return(0);
> +	

remove that lonely tab ;-)
There are some other "empty" lines that aren't empty. If they aren't 
already in the old code, remove all lonely tabs and spaces...

> -	if(xvid_global(NULL, XVID_GBL_INIT, &xvid_gbl_init, NULL))

....

> +	if (xvid_global(NULL, XVID_GBL_INIT, &xvid_gbl_init, NULL))
>  		return(0);

Only a space added here (after if) , please remove.

> -	dec.general |= XVID_LOWDELAY 
> +	dec.general |= XVID_LOWDELAY

Hmmm. Dangling whitespace isn't good but leave it there anyway.

> @@ -229,21 +282,27 @@
>  	int max_sse_u;
>  	int max_sse_v;
>  	int max_framenum;
> -	
>  	int pixels;
> +	FILE *fvstats;
> +	

another useless tab ;-)
P.S. if you simply mark all text, those lines that contain only tabs 
should be easily visible.

> -typedef struct XVIDRational{
> +typedef struct _xvid_rational_t {
>      int num; 
>      int den;
> -} XVIDRational;
> +} xvid_rational_t;

This is only a renaming, too.

>  	mod->mux->bih->biSizeImage = 
> -		mod->mux->bih->biWidth * mod->mux->bih->biHeight * 3;
> +		mod->mux->bih->biWidth * mod->mux->bih->biHeight * 3 / 2;

Why /2?

>  	/*--------------------------------------------------------------------
> -	 * Dispatch all module settings to XviD structures
> +	 * Init some module settings
>  	 *------------------------------------------------------------------*/
> -
> +	

Tab ;-)

> +	/* Diplayed width and height, used for autoaspect */

Typo: DiSplayed.

> @@ -315,45 +382,31 @@
>  static void
>  uninit(struct vf_instance_s* vf)
>  {
> -
>  	xvid_mplayer_module_t *mod = (xvid_mplayer_module_t *)vf->priv;

cosmetics.

> +	/* Close PSNR file if ever opened */
> +	if (mod->fvstats) {
> +		fclose(mod->fvstats);
> +		mod->fvstats = NULL;
>  	}
> -
> +	

You know ;-)

>  	/* ToDo: free matrices, and some string settings (quant method, matrix
> -	 * filenames...) */
> +	 * filenames...) - leaks a few bytes only, is it worth ?*/

?? Memleaks sure aren't good.

> @@ -535,7 +550,7 @@
>  	mod->mux->bih->biWidth = 0;
>  	mod->mux->bih->biHeight = 0;
>  	mod->mux->bih->biPlanes = 1;
> -	mod->mux->bih->biBitCount = 24;
> +	mod->mux->bih->biBitCount = 12;

Hmm.. ?

> @@ -827,9 +855,6 @@
>  	create->width  = mod->mux->bih->biWidth;
>  	create->height = mod->mux->bih->biHeight;
>  
> -	/* Pixels are needed for PSNR calculations */
> -	mod->pixels = create->width * create->height;
> -

Where is that initialized now? Did I miss something?

> -static int64_t xvid_gcd(int64_t a, int64_t b){
> -    if(b) return xvid_gcd(b, a%b);
> -    else  return a;
> +static int64_t
> +xvid_gcd(int64_t a, int64_t b)
> +{
> +     if(b)
> +	     return xvid_gcd(b, a%b);
> +     else
> +	     return a;
>  }

Only cosmetics I think...

> @@ -1103,18 +1257,18 @@
>          den= -den;
>          nom= -nom;
>      }
> -    
> +
>      if(nom < 0){
>          nom= -nom;
>          sign= 1;
>      }
> -    
> +
>      gcd = xvid_gcd(nom, den);
>      nom /= gcd;
>      den /= gcd;
> -    
> +

This time the tabs were removed ;-)

> @@ -1125,38 +1279,66 @@
>              if(a2n > max || a2d > max) break;
>  
>              nom %= den;
> -        
> +
>              a0= a1;
> -            a1= (XVIDRational){a2n, a2d};
> +            a1= (xvid_rational_t){a2n, a2d};
>              if(nom==0) break;
>              x= nom; nom=den; den=x;
>          }
>          nom= a1.num;
>          den= a1.den;
>      }
> -    
> +
>      assert(xvid_gcd(nom, den) == 1);
> -    
> +
>      if(sign) nom= -nom;
> -    
> +
>      *dst_nom = nom;
>      *dst_den = den;
> -    
> +

You sure know by now what I want to say...

> -static XVIDRational xvid_d2q(double d, int max){
> -    XVIDRational a;
> +static void xvid_d2q(xvid_rational_t *r, double d, int max)
> +{
>      int exponent= MAX( (int)(log(ABS(d) + 1e-20)/log(2)), 0);
>      int64_t den= 1LL << (61 - exponent);
> -    xvid_reduce(&a.num, &a.den, (int64_t)(d * den + 0.5), den, max);
> +    xvid_reduce(&r->num, &r->den, (int64_t)(d * den + 0.5), den, max);
> +}
>  
> -    return a;

Who coded the original version? Returning a local variable?!?!?!?

>  static const char *errorstring(int err)
>  {
> -	char *error;
> +	const char *error;
>  	switch(err) {
>  	case XVID_ERR_FAIL:
>  		error = "General fault";
> @@ -1177,7 +1359,7 @@
>  		error = "Unknown";
>  	}
>  
> -	return((const char *)error);
> +	return(error);
>  }

Is that an improvement or not? Both variants look bad to me.
Better remove the char *error variable and replace "error =" by return. 
I think that should work. You can also remove the break; statements then.

Greetings,
Reimar Döffinger