[MPlayer-dev-eng] [PATCH] VIA Unichrome support
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Sat May 7 23:15:35 CEST 2005
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...
More information about the MPlayer-dev-eng
mailing list