[MPlayer-dev-eng] [PATCH] VIA Unichrome support

[Ivor Hewitt]

On Saturday 07 May 2005 22:15, Reimar Döffinger wrote:
> Hi,
>
> On Sat, May 07, 2005 at 09:27:04PM +0100, Ivor Hewitt wrote:
> > Attached is the patch to add support for the open source driver for
> > unichrome and unichrome-pro chipsets to mplayer updated for current CVS.
>
> Here come my typical quick-check remarks *g*
>
> >  endif
> > -
> > +ifeq ($(HAVE_XXMC_ACCEL),yes)
> > +CODEC_LIBS += $(X_LIB)
> > +endif
> >  ALL_PRG = $(PRG)
> >
> >    int next_free_data_block_num;//used in add_mv_block, pointer to next
> > free block
> >
> > +
> > +
> >  	}
> >
> > -#ifdef HAVE_XVMC
> > -
> > +#if defined(HAVE_XVMC)||defined(HAVE_XVMC_VLD)
> >  #ifdef CODEC_CAP_HWACCEL
>
> What did those poor newlines do to you so that you have to push them
> around like that? :-)
>
They were lonely split up like that so they've huddled together for wamth.


> > -      XvMCDestroyMacroBlocks(mDisplay,&mv_blocks);
> > -      XvMCDestroyBlocks(mDisplay,&data_blocks);
> > -
> > +      if (!hasVLDAcceleration())
> > +      {
> > +         XvMCDestroyMacroBlocks(mDisplay,&mv_blocks);
> > +         XvMCDestroyBlocks(mDisplay,&data_blocks);
> > +      }
>
> I personnally would prefer it if you didn't change the indentation of
> these two lines...
>
But thats crazy. The lines are now inside a conditional block, they 'should' 
be indented.

> >          //one 1 we memcpy blocks in xvmcvideo
> > -        if(s->avctx->xvmc_acceleration > 1)
> > +        if(s->avctx->xvmc_acceleration == 2)
>
> You're doing this in a lot of places. Why? If its a bugfix you might
> consider making this a seperate patch, so it can be applied, reversed
> etc. independantly.
>
The condition only needs changing if the VLD acceleration is included. There's 
no point making it a separate patch if the VLD part isn't going to be 
applied.

> > -#ifdef HAVE_XVMC
> > -    if(s->avctx->xvmc_acceleration)
> > +#if defined(HAVE_XVMC)|defined(HAVE_XVMC_VLD)
> > +    if(s->avctx->xvmc_acceleration)
>
> Hm.. What's the difference in these two if lines?
>
"_VLD" 


-- 
Ivor Hewitt.
http://www.ivor.it - tech | http://www.ivor.org - hedge