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

[Alban Bedel]

On Wed, 25 Jul 2007 21:34:40 +0200
Balatoni Denes <dbalatoni at interware.hu> wrote:

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

You don't have to vsync the flip, it's probably preferable, but not
requiered. However as Uoti pointed out you will need double buffer
for a proper timing.

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

Is the image scaled down or a part missing? If a part is missing I
would not consider that usefull and imho config() should fail. But if
it's scaled down, you could compute the effective size and pass it back
to xover to allow it to correctly resize the window.

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

How trivial it is doesn't matter imho, as soon as it exceed 2 or 3
lines duplicated code just make maitenance harder.

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

You probably hit this pb:
http://article.gmane.org/gmane.comp.video.mplayer.devel/1130
It's a limitation of lavc, if it work fine with TEMP buffers you
should enable it imho.

	Albeu