[MPlayer-dev-eng] [PATCH] support for external VIDIX

[Diego Biurrun]

On Sun, Mar 26, 2006 at 11:32:45PM +0200, Nico Sabbi wrote:
> Diego Biurrun wrote:
> 
> >>+else
> >>+  _external_vidix=no
> >
> >The else clause is superfluous, you've already set _external_vidix=no
> >above.
> >
> no, it handles the case --disable-internal-vidix --disable-external-vidix,
> otherwise disabling vidix entirely would not be possible

Huh?  Both variables are 'no' to begin with then, why do you need to set
one to 'no' again?

> >I don't particularly like the _has_vidix variable.  Instead you could
> >rename _vidix to _vidix_internal and use _vidix for the common case.
> 
> updated

Ideally you would do a commit with the rename first, that will make
reviewing your changes easier.  Feel free to do it right away.

> --- configure	21 Mar 2006 05:36:09 -0000	1.1149
> +++ configure	26 Mar 2006 21:22:04 -0000
> @@ -7133,9 +7137,35 @@
>  linux && _def_linux='#define TARGET_LINUX 1'
>  
>  # TODO cleanup the VIDIX stuff here
> -echocheck "VIDIX"
> +echocheck "VIDIX (internal)"
>  _def_vidix='#define CONFIG_VIDIX 1'
> -test "$_vidix" = no && _def_vidix='#undef CONFIG_VIDIX'
> +_vidix=yes
> +if test "$_vidix_internal" = no ; then
> +  _vidix=no
> +  _def_vidix='#undef CONFIG_VIDIX'
> +fi

Hmm, why do you need to set this here at all?  Check out the FAAD test,
it should be possible to make two tests, set common variables and derive
the CONFIG_ etc settings from there.

> +echocheck "VIDIX (external)"
> +if test "$_vidix_internal" = no && test "$_vidix_external" != no; then

This test is wrong, the check should only be run if _vidix_external is
set to 'auto'.

> + _vidix_external=no
> + _def_vidix="#undef CONFIG_VIDIX"
> + _vidix=no

Same as above.

Diego