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

[Guillaume POIRIER]

Hi,

Le sam 02/10/2004 à 20:09, Reimar Döffinger a écrit :

> > @@ -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.

I've "discovered" this afternoon vim 'diff' mode, which should allow me
to avoid that kind of embarrassment. :-)


> > -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?

To get half the value? ;-p

Heck I know?
Sorry here, I just send patches, I didn't do the coding :-(

Maybe Edouard will show up and explain that...


> >  	/* 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.

But it's a known problem, and clearly marked as so, so IMHO, it's ok,
and I'm sure I'll get fixed in no time by XviD devs.


> > @@ -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.. ?

Still here, I'm sure that it can be explained, but I'm not the guy in
charge here.


> > @@ -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?

(Still here, I don't know, but it works!) ;-)

> > -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...

Yes, and It's fixed now.


> > -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?!?!?!?

That's what it looks like. See how better the new front-end is? ;-)


> >  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.

I couldn't say which way is better either.
I made the suggested change, and the attached patch features all your
suggestions (at least, it should).

Thanks Reimar for having a look at it. I wasn't expecting feedback that
soon.

Is it ok to commit it like this, and make doxygen comments afterward, or
should it feature doxygen comments now?

I sent this mail in Bcc to Edouard too, so that he can speak up about
the suggested changes or the different "magic numbers".

Regards,
Guillaume

-------------- next part --------------
A non-text attachment was scrubbed...
Name: xvid-1.1-round5.patch
Type: text/x-patch
Size: 38173 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20041002/eb31dcfd/attachment.bin>