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

[Reimar Döffinger]

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? :-)

> @@ -498,6 +523,13 @@ static uint32_t vm_height;
>        surface_render[i].chroma_format = surface_info.chroma_format;
>        surface_render[i].unsigned_intra = (surface_info.flags & XVMC_INTRA_UNSIGNED) == XVMC_INTRA_UNSIGNED;
>        surface_render[i].p_surface = &surface_array[i];
> +
> +        surface_render[i].state = 0;
> +
> +        surface_render[i].disp = mDisplay;
> +        surface_render[i].ctx = &ctx;
> +
> +

Hmmm... You have a different indentation here. Looks like you're using
tabs and the rest of the code uses spaces (or the other way round).
Better keep it consistent.

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

>  // MPV_frame_start will call this function too,
>  // but we need to call it on every field
>      if(s->avctx->xvmc_acceleration)
>           XVMC_field_start(s,avctx);
>  #endif
> -
>      return 0;

... poor newlines *sniff*

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

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

>  
>  void XVMC_field_end(MpegEncContext *s){
> -xvmc_render_state_t * render;
> +    xvmc_render_state_t * render;
> +    

Looks a lot like cosmetics to me...

>  
> -    if(render->filled_mv_blocks_num > 0){
> -//        printf("xvmcvideo.c: rendering %d left blocks after last slice!!!\n",render->filled_mv_blocks_num );
> -        ff_draw_horiz_band(s,0,0);
> +#ifdef HAVE_XVMC_VLD
> +    if (s->avctx->xvmc_acceleration == 4)
> +    {
> +        XvMCFlushSurface(render->disp, render->p_surface);
> +        XvMCSyncSurface(render->disp, render->p_surface);
> +
> +    	s->error_count = 0;
> +    }
> +    else
> +#endif
> +    {
> +        if(render->filled_mv_blocks_num > 0){
> +//          printf("xvmcvideo.c: rendering %d left blocks after last slice!!!\n",render->filled_mv_blocks_num );
> +            ff_draw_horiz_band(s,0,0);
> +        }

Please presever the indentation of this
if(render->filled_mv_blocks_num...

Greetings,
Reimar Döffinger

P.S.: Please keep in mind that this is the result of a really quick
check, done without any understanding of the code concerned...