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

[Guillaume POIRIER]

Hi,
Le mar 28/09/2004 à 22:44, Reimar Döffinger a écrit :
> Well, you asked for a review. I can't really comment on the code as I 
> don't know it, but I have a few nitpicks about that patch - You asked 
> for it ;-)

First of all, I'd like to thank you for having a look at it.
The thing is: I never had the opportunity until I started contributing
to MPlayer to do some collaborating where peer review as absolutely
necessary.
So I'm absolutely unfamiliar with what is involved by the reviewing
process, and the way a patch should be sent (like: "cosmetics", then
real code, or the other way)

Then I'd like to point out that this patch is not my code, it's Edouard
Gomez's, but as it looks like he's not likely to push it into "mainline"
MEncoder, I'm gonna have to do it for him.
So I don't know the code that well, but I do know it works well, as I've
been using it for 29 days now, and I'm apparently not the only one to
report no problem at all.

> >>>+
> >>>+.
> >>>+.
> >>
> >>Don't add empty lines to the man page.
> > 
> > Can somebody tell me why I keep doing dumb things like this? ;-)
> 
> That's normal, get used to it. And better make sure you can absorb a 
> great amout of cola ;-)

I'm not a cola addict (I prefer tea or coffee), but I looks like given
recent stupid commits I made to the English manpage, I'd better drink
some more when working on MPlayer.

> >  #ifdef HAVE_XVID4
> >  
> > +#include <xvid.h>
> > +
> > +#if XVID_API < XVID_MAKE_API(4,0)
> > +#error This import module compiles only with XviD API 4 libraries (public version >= 1.0.0)
> > +#endif
> > +
> >  #include "vd_internal.h"
> >  #include "m_option.h"
> >  
> > -#include <xvid.h>
> > -
> 
> Why did you move the include up? If possible, leave it where it was...

Yes, I can do that.


> >  static int init(sh_video_t *sh)
> >  {
> > -	xvid_gbl_init_t xvid_ini;
> > -	xvid_dec_create_t dec_p;
> > +	xvid_gbl_info_t xvid_gbl_info;
> > +	xvid_gbl_init_t xvid_gbl_init;
> > +	xvid_dec_create_t xvid_dec_create;
> 
> It seems to me that you mostly renamed xvid_ini to xvid_gbl_init and 
> dec_p to xvid_dec_create. This makes it more difficult to both 
> understand the patch and track possible bugs introduced by it. If you 
> really want it for readability reasons, I'd say put it in a separate patch.

Ok, I'm gonna first commit a patch that features that cosmetics that, I
agree, don't change anything besides readability, but have to be done in
order to have this front-end in sync with Edouard's.

> > -	memset(&xvid_ini, 0, sizeof(xvid_gbl_init_t));
> > -	xvid_ini.version = XVID_VERSION;
> > -	memset(&dec_p, 0, sizeof(xvid_dec_create_t));
> > -	dec_p.version = XVID_VERSION;
> > +	memset(&xvid_gbl_info, 0, sizeof(xvid_gbl_info_t));
> > +	xvid_gbl_info.version = XVID_VERSION;
> > +	
> > +	memset(&xvid_gbl_init, 0, sizeof(xvid_gbl_init_t));
> > +	xvid_gbl_init.version = XVID_VERSION;
> > +	
> > +	memset(&xvid_dec_create, 0, sizeof(xvid_dec_create_t));
> > +	xvid_dec_create.version = XVID_VERSION;
> 
> Most of this patch doesn't change functionality, please remove those 
> changes for now.

The attached patch will provide such name change.

> 
> > -	switch(sh->codec->outfmt[sh->outfmtidx]){
> > +	switch (sh->codec->outfmt[sh->outfmtidx]){
> 
> This doesn't change anything either, does it? So use the original form.
> 
> > -	switch(cs) {
> > +	switch (cs) {
> 
> Only whitespace change. Bad.
> 
> > -	memset(&dec,0,sizeof(xvid_dec_frame_t));
> > +	memset(&dec, 0, sizeof(xvid_dec_frame_t));
> 
> ...

Same, so I'll zap that too.


> > -	dec.general |= XVID_LOWDELAY 
> > -	        | (filmeffect ? XVID_FILMEFFECT : 0 )
> > -	        | (lumadeblock ? XVID_DEBLOCKY : 0 )
> > -	        | (chromadeblock ? XVID_DEBLOCKUV : 0 );
> > -
> > +	dec.general |= XVID_LOWDELAY;
> > +	/* XXX: if lowdelay is unset, and xvidcore internal buffers are
> > +	 *      used => crash. MUST FIX */
> > +	dec.general |= (filmeffect ? XVID_FILMEFFECT : 0 );
> > +	dec.general |= (lumadeblock ? XVID_DEBLOCKY : 0 );
> > +	dec.general |= (chromadeblock ? XVID_DEBLOCKUV : 0 );
> > +#if XVID_API >= XVID_MAKE_API(4,1)
> > +	dec.general |= (lumadering ? XVID_DEBLOCKY|XVID_DERINGY : 0 );
> > +	dec.general |= (chromadering ? XVID_DEBLOCKUV|XVID_DERINGUV : 0 );
> > +#endif
> 
> Try to stay as close as possible to the original code I'd say...

Ok, I can easily fix fix that
> 
> 
> > +/* Returns DAR value according to VOL's informations contained in stats
> > + * param */
> > +static float stats2aspect(xvid_dec_stats_t *stats)
> > +{
> 
> Doxygen style comments are required now, see 
> DOCS/tech/code-documentation.txt

Hum... Given that it's not my code, I wonder if it would be a good idea
for me to write " maybe wrong" comments.
Heck, if it's required, I'll do it, just be warned!

> Sorry, too lazy to have a look at the ve_ part now... But I hope I gave 
> you enough hints to do some work ;-)

Yes, thank you Reimar for your time

The attached patch feature the name change that'll make the move to
1.1.x smoother.
I no one complains, I'll commit that in 3 day...
It may look like I'm putting a knife under the reviewers or that I may
jeopardize my CVS access, but given that this change is minor, that
nobody seem to react, and that there's nobody claimed as maintainer of
that bunch of code on DOCS/tech/MAINTAINERS (even though Ivan is on
AUTHORS, and has done a great job in merging the xvid4 front-end in the
past, it looks like he's busy with real life at the moment ;-),
I guess someone has to do the job, so I'm offering my help.

Please review, comment, flame on what I write/send. ;-)

Regards,

Guillaume

-------------- next part --------------
A non-text attachment was scrubbed...
Name: vd_xvid4.name_change.patch
Type: text/x-patch
Size: 1495 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20040929/528721ef/attachment.bin>