[FFmpeg-devel] [PATCH]VDPAU patch for VC1 decoding, round 1

[Aurelien Jacobs]

Diego Biurrun wrote:

> On Mon, Jan 12, 2009 at 12:51:48AM +0100, Aurelien Jacobs wrote:
> > Diego Biurrun wrote:
> > 
> > > On Mon, Jan 12, 2009 at 12:12:41AM +0100, Aurelien Jacobs wrote:
> > > > Aurelien Jacobs wrote:
> > > > 
> > > > > Diego Biurrun wrote:
> > > > > 
> > > > > > On Wed, Jan 07, 2009 at 10:50:39PM +0100, Carl Eugen Hoyos wrote:
> > > > > > >
> > > > > > > --- libavcodec/vc1.c	(revision 16481)
> > > > > > > +++ libavcodec/vc1.c	(working copy)
> > > > > > > @@ -4317,3 +4338,35 @@
> > > > > > > +
> > > > > > > +#if ENABLE_WMV3_VDPAU_DECODER
> > > > > > 
> > > > > > Using ENABLE_ here is still wrong...
> > > > > 
> > > > > I disagree. Using #if ENABLE_ is better than #ifdef CONFIG_.
> > > > > It is more readable, shorter, and more consistent especially
> > > > > in cases such as:
> > > > >   #if   ENABLE_A
> > > > >   #elif ENABLE_B
> > > > >   #endif
> > > > > or
> > > > >   #if ENABLE_A || ENABLE_B
> > > > > 
> > > > > Moreover, #if gives a chance to the compiler to complain if
> > > > > there is a typo in the symbol name. With #ifdef, such a
> > > > > typo could stay unnoticed for a long time.
> > > > > 
> > > > > Now I expect some answer about consistency because the code
> > > > > supposedly contains more occurence of #ifdef CONFIG_ than
> > > > > #if ENABLE_.
> > > > > The attached patch solves this by replacing all usage of
> > > > > CONFIG_ by ENABLE_.
> > > > > With this applied, we could also drop all CONFIG_ from
> > > > > config.h.
> > > > 
> > > > It seems there is no objection to this patch anymore ?
> > > > If so, I will commit soon.
> > > 
> > > The patch replacing all CONFIG_s with ENABLE_s?
> > > 
> > > I don't like it.  However, we can discuss a patch that gets rid of
> > > the ENABLE_s.
> > 
> > What do you propose exactly ?
> 
> What do you propose exactly?

The patch I posted in this thread ;-)

> > Do you want to replace this:
> > 
> > if (ENABLE_XXX && condition) {
> > }
> > 
> > by this:
> > 
> > #ifdef CONFIG_XXX
> > if (condition) {
> > }
> > #endif
> > 
> > If that's what you mean, I'm opposed to it.
> 
> That's not what I mean.
> 
> I'm suggesting that CONFIG_s could always be set to 0 or 1 and then
> used instead of the ENABLE_s.

As I've already said in this thread, I don't care much about
the naming.
It's easier to go through an intermediate step using enable as it
don't forces us to do all the #ifdef => #if at once (I might have
missed some), but I can probably try to craft a new patch changing
the #ifdef CONFIG_* to #if CONFIG_* while changing the CONFIG_
semantic at the same time.

Aurel