[MPlayer-dev-eng] [PATCH] XviD 1.1.x support
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Sat Oct 2 20:09:54 CEST 2004
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
More information about the MPlayer-dev-eng
mailing list