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

[Alban Bedel]

On Mon, 23 Jul 2007 20:27:22 +0200
Balatoni Denes <dbalatoni at programozo.hu> wrote:

> + * no double-buffering, as it would slow down playback (waiting for vertical retraces)

Why that? In the typical case the wait time is negligable enouth, and
the user can still disable it if he want. It doesn't matter much atm
but you should definitly consider implementing double buffering.

> +/* This actually doesn't change a thing, width greater than 1536 still won't work... */

You should consider putting a check for the image size in config() until
this issue is solved.

> +
> +static int config(uint32_t width, uint32_t height, uint32_t d_width,
> +                  uint32_t d_height, uint32_t flags, char *title,
> +                  uint32_t format) {
> +
> +    pfb_srcwidth=width;
> +    pfb_srcheight=height;
> +
> +    if (!(flags & VOFLAG_XOVERLAY_SUB_VO)) {
> +        aspect_save_orig(width,height);
> +        aspect_save_prescale(d_width,d_height);
> +        aspect_save_screenres(pfb_xres,pfb_yres);
> +        if( flags&VOFLAG_FULLSCREEN) { /* -fs */
> +            aspect(&pfb_dstwidth,&pfb_dstheight, A_ZOOM);
> +            pfb_fs = 1;
> +        } else {
> +            aspect(&pfb_dstwidth,&pfb_dstheight, A_NOZOOM);
> +            pfb_fs = 0;
> +        }
> +    } else {
> +        pfb_dstwidth=d_width;
> +        pfb_dstheight=d_height;
> +    }
> +
> +    if (pfb_xres > pfb_dstwidth) {
> +        pfb_wx0 = (pfb_xres - pfb_dstwidth) / 2;
> +        pfb_wx1 = pfb_wx0 + pfb_dstwidth;
> +    }
> +    else {
> +        pfb_wx0 = 0;
> +        pfb_wx1 = pfb_xres;
> +    }
> +
> +    if (pfb_yres > pfb_dstheight) {
> +        pfb_wy0 = (pfb_yres - pfb_dstheight) / 2;
> +        pfb_wy1 = pfb_wy0 + pfb_dstheight;
> +    }
> +    else {
> +        pfb_wy0 = 0;
> +        pfb_wy1 = pfb_yres;
> +    }
> +
> +    pfb_stride1=(pfb_srcwidth+15) & ~15;
> +    pfb_stride2=pfb_stride3=(((pfb_srcwidth+1)>>1)+15) & ~15;
> +
> +    pfb_buffer1 = pfb_free_top - (pfb_stride1*pfb_srcheight+pfb_stride2*((pfb_srcheight+1) & ~1));
> +    pfb_buffer2 = pfb_buffer1 + pfb_stride1*pfb_srcheight;
> +    pfb_buffer3 = pfb_buffer2 + pfb_stride2*((pfb_srcheight+1)>>1);
> +
> +    pfb_overlay_on();
> +
> +    return 0;
> +}
> +
>   [...]
> +
> +static uint32_t pfb_fullscreen() {
> +    if (!pfb_fs) {
> +        aspect(&pfb_dstwidth,&pfb_dstheight, A_ZOOM);
> +        pfb_fs = 1;
> +    } else {
> +        aspect(&pfb_dstwidth,&pfb_dstheight, A_NOZOOM);
> +        pfb_fs = 0;
> +    }
> +
> +    if (pfb_xres > pfb_dstwidth) {
> +        pfb_wx0 = (pfb_xres - pfb_dstwidth) / 2;
> +        pfb_wx1 = pfb_wx0 + pfb_dstwidth;
> +    }
> +    else {
> +        pfb_wx0 = 0;
> +        pfb_wx1 = pfb_xres;
> +    }
> +
> +    if (pfb_yres > pfb_dstheight) {
> +        pfb_wy0 = (pfb_yres - pfb_dstheight) / 2;
> +        pfb_wy1 = pfb_wy0 + pfb_dstheight;
> +    }
> +    else {
> +        pfb_wy0 = 0;
> +        pfb_wy1 = pfb_yres;
> +    }
> +
> +    pfb_overlay_on();
> +
> +    return VO_TRUE;
> +}

This could be done with less code duplication.

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

What is not working exactly?

> +    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;
> +
> +    mpi->planes[0]=pfb_vbase + pfb_buffer1;
> +    if (mpi->flags&MP_IMGFLAG_SWAPPED){
> +        mpi->planes[1]=pfb_vbase + pfb_buffer3;
> +        mpi->planes[2]=pfb_vbase + pfb_buffer2;
> +    } else {
> +        mpi->planes[1]=pfb_vbase + pfb_buffer2;
> +        mpi->planes[2]=pfb_vbase + pfb_buffer3;
> +    }

This is wrong. MP_IMGFLAG_SWAPPED mean that the planes are swaped in
memory. However the mpi->planes pointers are always in the same order
(YUV), so this flag is irrelevant unless you access the 3 planes as a
single chunk of memory.

> +static int query_format(uint32_t format)
> +{
> +    switch(format){
> +    case IMGFMT_YV12:

I420 should work just fine too.

> +        return VFCAP_CSP_SUPPORTED | VFCAP_CSP_SUPPORTED_BY_HW |VFCAP_HWSCALE_UP|VFCAP_HWSCALE_DOWN|VFCAP_ACCEPT_STRIDE;
> +    }
> +    return 0;

The rest look ok afaict.

	Albeu