[MPlayer-dev-eng] [PATCH]VO_VDPAU, round 2

[Uoti Urpala]

On Tue, 2009-01-06 at 16:59 +0100, Reimar Döffinger wrote:
> On Tue, Jan 06, 2009 at 03:06:01AM +0100, Carl Eugen Hoyos wrote:
> > +VdpDeviceDestroy * vdp_device_destroy;
> > +VdpVideoSurfaceCreate * vdp_video_surface_create;
> > +VdpVideoSurfaceDestroy * vdp_video_surface_destroy;
> 
> If you don't mind, I think it would be better to align these and place the *
> consistently, preferably without a space to the right.
> More importantly though, should/need these really all be global (no
> "static")?

The comments talk about using the functions elsewhere like in filters.
Relying on the VO to initialize them first has some obvious problems
though.

> And would it maybe be nicer to put them all into a struct, making it
> easier to allow multiple vo instances "somewhen"?

You'd probably get a nicer result by rewriting the VO based on the
general code from vo_xv in the git tree (which does avoid globals). I
may do that at some point if I install the beta drivers needed by VDPAU,
but OTOH I also have other tasks queued up already so I'm not sure
whether I'll get to that.

> > +    const struct vdp_function vdp_func[] = {
> 
> As long as the function pointer addresses are known at compile time,
> "static" would be better.

Shouldn't make any difference. The compiler can use the same constant
table whether the variable is static or not. The only difference would
be documenting that the contents do not change between calls in case
that is not otherwise obvious.

> > +    if (vdp_st != VDP_STATUS_OK)
> > +        printf("Error %d at %s:%d\n", vdp_st, __FILE__, __LINE__);

> mp_msg of course. VDPAU has no way to get an error string?

Since now all the function calls repeat the same error-printing code, I
think it would be better to replace the function pointers with wrapper
functions that print the errors instead. Of course the error handling
strategy might need to be improved at some point.

> > +    if (vo_config_count)
> > +    {
> > +        // FIXME: We should really check for matching parameters here,
> > +        // and uninit/re-config if they have changed? 
> > +        return 0;
> > +    }
> 
> This comment sounds like it will break horribly with -fixed-vo and
> playing e.g. a VC-1 file after a H.264 file...
> 
> > +    image_format = format;
> [...]
> > +    int_pause = 0;
> > +    visible_buf = -1;
> 
> These sure make above return 0 look like complete nonsense...

How so? Initializing those is independent of reinitialization, because
they are static variables and so can have old values even if the
previous instance was closed down. You would need to at least run some
part of the uninit() code to make reconfig of an existing instance work.


> > +    mp_input_rm_event_fd(ConnectionNumber(mDisplay));
> 
> Huh? I just saw vo_xv has this too, but... what does it do?
> (I mean, I know the function, but that
> mp_input_add_event_fd(ConnectionNumber(mDisplay), check_events);
> seems like a weird hack, I wonder what its purpose is, does it still
> serve one?...).

The purpose is to allow getting events from X without polling. Without
that you can't detect user keypresses (or any other X events) unless
MPlayer constantly wakes up to poll for them with vo_check_events();
registering the input file descriptor allows MPlayer to detect those
events in the main input loop code.

That functionality should be added to all the other X VOs too, and the
default sleeping mechanism changed to sleep in the input select() to
allow instant response to user or slave events.