[MPlayer-dev-eng] [PATCH] SUN XVR-100 VO driver 3. try

[Reimar Döffinger]

Hello,

You use a pfb_ prefix everywhere IMO that is really pointless and just
bloats the text, and might even be really confusing since name of the vo
is xvr100, not pfb.

On Mon, Jul 23, 2007 at 08:27:22PM +0200, Balatoni Denes wrote:
> @@ -144,6 +147,9 @@
>  
>  vo_functions_t* video_out_drivers[] =
>  {
> +#ifdef HAVE_XVR100
> +        &video_out_xvr100,
> +#endif
>  #ifdef HAVE_TDFX_VID
>          &video_out_tdfx_vid,
>  #endif

I doubt it is justified putting this before all other vos, IMO at least xv
if available should be tried first.

> +#define PFB_OV0_V_INC 0x109
> +#define PFB_OV0_P1_V_ACCUM_INIT 0x10A
> +#define PFB_OV0_P23_V_ACCUM_INIT 0x10B
> +#define PFB_OV0_P1_BLANK_LINES_AT_TOP 0x10C
> +#define PFB_OV0_P23_BLANK_LINES_AT_TOP 0x10D

Where do these come from? They aren't in some system or vidix header
already somewhere?

> +static int pfb_buffer1, pfb_buffer2, pfb_buffer3;
> +static int pfb_stride1, pfb_stride2, pfb_stride3;

Why not use an array?

> +static short int pfb_wx0, pfb_wy0, pfb_wx1, pfb_wy1;
> +static int pfb_xres,pfb_yres;
> +static int pfb_free_top;
> +static int pfb_fs;
> +static int pfb_usecolorkey=0;
> +static uint32_t pfb_colorkey;

Why not reuse vo_fs, vo_dx, vo_dy, vo_screenwidth, vo_screenheight and
vo_colorkey?
vo_colorkey contains the information of both pfb_colorkey and
pfb_usecolorkey, if (vo_colorkey & 0xff000000) != 0 then colorkey is
disabled.

> +void pfb_overlay_on() {

All functions (unless part of the vo API like reinit, config etc.) should
have doxygen-compatible comments.

> +/* This actually doesn't change a thing, width greater than 1536 still won't work... */
> +#if 0
> +    /* if the source width was larger than what would fit in overlay scaler increase step_by values */

Well, I'd be surprised if the scaler didn't always read a full line at
once into its buffer, so this just can't work. IMO remove it.

> +    pfb_vregs[PFB_OV0_REG_LOAD_CNTL] = PFB_OV0_REG_LOAD_LOCK;
> +    while (!(pfb_vregs[PFB_OV0_REG_LOAD_CNTL] & PFB_OV0_REG_LOAD_LOCK_READBACK)) {}

This is not time critical I think, so you should sleep here instead of
doing such a tight busy loop.

> +    pfb_vregs[PFB_OV0_P1_H_ACCUM_INIT] = (((0x00005000 + h_inc) << 7) & 0x000f8000) | (((0x00005000 + h_inc) << 15) & 0xf0000000);
> +    pfb_vregs[PFB_OV0_P23_H_ACCUM_INIT] = (((0x0000A000 + h_inc) << 6) & 0x000f8000) | (((0x0000A000 + h_inc) << 14) & 0x70000000);

I am quite sure this can be simplified, and esp. in a way that makes it
clearer how to extend it to non-YV12.
I mean e.g.
(0x0000A000 + h_inc) << 6 == (0x00005000 + h_inc/2) << 7.
And it can be & 0xf0000000 in both cases, does not make any difference.
I do not know how you came to that code, so maybe it makes sense how you
did it (then you should add comments to explain it), otherwise this is
simpler (making heavy use of h_inc <= 0x1fff):
// handle high-part of h_inc
h_inc >>= 8;
pfb_vregs[PFB_OV0_P1_H_ACCUM_INIT] = ((h_inc ^ 0x10) << 15) | ((h_inc & 0x10) << 24) | (1 << 29);
h_inc >>= 1;
pfb_vregs[PFB_OV0_P23_H_ACCUM_INIT] = ((h_inc ^ 0x10) << 15) | ((h_inc & 0x10) << 24) | (1 << 29);


> +    if (pfb_usecolorkey) {
> +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_LOW] = pfb_colorkey;
> +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_HIGH] = pfb_colorkey | 0xff000000;
> +        pfb_vregs[PFB_OV0_KEY_CNTL] = PFB_OV0_KEY_EN;
> +    } else {
> +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_LOW] = 0;
> +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_HIGH] = 0 | 0xff000000;
> +        pfb_vregs[PFB_OV0_KEY_CNTL] = 0x010;
> +    }

Why is PFB_OV0_KEY_EN a define but 0x010 not? And do you really need to
set the colour key when you disable it?

> +    pfb_buffer1 = pfb_free_top - (pfb_stride1*pfb_srcheight+pfb_stride2*((pfb_srcheight+1) & ~1));

You should check that this does not underflow.

> +    pfb_free_top = attr.fbtype.fb_size - 0x2000;

Hm... this seems really weird to me, please comment the idea behind
this.

> +    page_size = sysconf(_SC_PAGE_SIZE);
> +
> +    if ((pfb_vbase = mmap(NULL, (((PFB_VRAM_MMAPLEN + page_size - 1) / page_size) * page_size), PROT_READ | PROT_WRITE, MAP_SHARED, pfb_devfd, PFB_VRAM_MMAPBASE)) == MAP_FAILED) {

According to the man page there is no reason to page-align the length
argument of mmap. And for VRAM it is probably a given anyway.

> +    long page_size;
> +
> +    if (!vo_config_count)
> +        return;
> +
> +    pfb_overlay_off();
> +    page_size = sysconf(_SC_PAGE_SIZE);
> +    munmap(pfb_vbase, (((PFB_VRAM_MMAPLEN + page_size - 1) / page_size) * page_size));
> +    munmap(pfb_vregs, (((PFB_REGS_MMAPLEN + page_size - 1) / page_size) * page_size));

Same here.

> +static uint32_t get_image(mp_image_t *mpi){
> +    return VO_FALSE;
> +#if 0 /* doesn't work for some reason */

If it doesn't work you should find out why, it might hide some bug. If
you need help, explain more precisely why it doesn't work.
And anyway, this code does not have to work 100% flawless, it can be
disable at runtime via -nodr (which btw. is default) anyway.

> +    if (mpi->flags&MP_IMGFLAG_READABLE) return VO_FALSE;
> +    if (!(mpi->flags&MP_IMGFLAG_PLANAR)) return VO_FALSE; // FIXME: impossible for YV12, right?
> +    if (!(mpi->flags&MP_IMGFLAG_ACCEPT_STRIDE)) return VO_FALSE;

Probably your error is that you did not exclude MP_IMGTYPE_IPB and
MP_IMGTYPE_IP. You can't handle those since they require you to provide
multiple buffers (if mplayer was sane this would have been covered by
mpi->flags&MP_IMGFLAG_READABLE already :-( ).

> +static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y)
> +{
> +    mem2agpcpy_pic(pfb_vbase + pfb_buffer1 + pfb_stride1 * y + x, src[0], w, h, pfb_stride1, stride[0]);
> +    mem2agpcpy_pic(pfb_vbase + pfb_buffer2 + pfb_stride2 * (y>>1) + (x>>1), src[1], (w>>1), (h>>1), pfb_stride2, stride[1]);
> +    mem2agpcpy_pic(pfb_vbase + pfb_buffer3 + pfb_stride2 * (y>>1) + (x>>1), src[2], (w>>1), (h>>1), pfb_stride2, stride[2]);

Just do
x >>= 1; y >>= 1;
w >>= 1; h >>= 1;
after the part handling the Y case IMO.
Makes it easier to extend to non-YV12 as well.

Greetings,
Reimar Döffinger