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

[Reimar Döffinger]

Hi,

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 ;-)

>>>+
>>>+.
>>>+.
>>
>>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 ;-)

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

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

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

> -	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));

...

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


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

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

Greetings,
Reimar Döffinger