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

[Balatoni Denes]

Hi!

Wednesday 25 July 2007 20:04-kor Alban Bedel ezt írta:
> > + * 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.

I did try it, it was much slower, so I didn't like it. Maybe in the future 
(btw it seems two vertical retraces are needed until the page is flipped, and 
that is slow).

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

It works, but there is a pink stripe on the right side. So IMO the user should 
choose whether he/she wants pink stripe (-vo xover:xvr100) or unusably slow 
playback (-vo x11).

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

Like moving 15 trivial lines of code to a separate function? What's the point 
of that?

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

-vfm ffmpeg -dr always says "stride changed" or something like that (I don't 
have the message at hand), and some frames are not shown.

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

Ok, than this will be removed.

> > +static int query_format(uint32_t format)
> > +{
> > +    switch(format){
> > +    case IMGFMT_YV12:
>
> I420 should work just fine too.

This will be included.

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

Thanks, patch will be coming tomorrow.

>
> 	Albeu

bye
Denes