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

Reimar Döffinger Reimar.Doeffinger at stud.uni-karlsruhe.de
Tue Jan 6 16:59:51 CET 2009


On Tue, Jan 06, 2009 at 03:06:01AM +0100, Carl Eugen Hoyos wrote:
> +#define ARSIZE(x) (sizeof(x) / sizeof((x)[0]))

Isn't such a thing in libavutil? Anyway I think it might be better not
to use it, see below.

> +/* Declaration for  all varialbles of win_x11_init_vdpau_procs() and 

variables

> +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")?
And would it maybe be nicer to put them all into a struct, making it
easier to allow multiple vo instances "somewhen"?

> +int surfaceNum;

Why not static?

> +extern uint32_t num_reference_surfaces;

Considering a different way to pass this information, e.g. a VOCTRL
might be nicer.

> +static uint32_t max_width = 0, max_height = 0; // zero means: not set

I have some idea, but I am not completely sure what these are there for.

> +static void calc_drwXY_dWH(uint32_t *drwX, uint32_t *drwY, uint32_t *d_wh, uint32_t *d_ht)
> +{
> +    *drwX = *drwY = 0;
> +    if (vo_fs) {
> +        aspect(&vo_dwidth, &vo_dheight, A_ZOOM);
> +        vo_dwidth = FFMIN(vo_dwidth, vo_screenwidth);
> +        vo_dheight = FFMIN(vo_dheight, vo_screenheight);
> +        *drwX = (vo_screenwidth - vo_dwidth) / 2;
> +        *drwY = (vo_screenheight - vo_dheight) / 2;
> +        mp_msg(MSGT_VO, MSGL_V, "[vdpau-fs] dx: %d dy: %d dw: %d dh: %d\n",
> +               *drwX, *drwY, vo_dwidth, vo_dheight);
> +    } else if (WinID == 0) {
> +        *drwX = vo_dx;
> +        *drwY = vo_dy;
> +    }

Identical with vo_xv code, would be preferable to factor it out.

> +    const struct vdp_function vdp_func[] = {

As long as the function pointer addresses are known at compile time,
"static" would be better.

> +    // Create Device
> +    vdp_st = vdp_device_create_x11(
> +        mDisplay, //x_display,
> +        mScreen, //x_screen,
> +        &vdp_device,
> +        &vdp_get_proc_address
> +    );

The comments seem useless to me.

> +    if (vdp_st != VDP_STATUS_OK)
> +        printf("Error %d at %s:%d\n", vdp_st, __FILE__, __LINE__);
> +
> +    for(dsc = vdp_func; dsc->pointer; dsc++){
> +        vdp_st = vdp_get_proc_address(vdp_device, dsc->id, dsc->pointer);
> +        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?
Also the error messages should have _some_ meaning, we are not
Microsoft and think that undocumented error numbers are helpful I hope
;-)...

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

> +#ifdef HAVE_NEW_GUI
> +    if (use_gui)
> +        guiGetEvent(guiSetShVideo, 0);  // let the GUI to setup/resize our window
> +    else
> +#endif
> +    {
> +#ifdef HAVE_XF86VM
> +        if (vm)
> +            vo_vm_switch();
> +        else
> +#endif
> +        XGetWindowAttributes(mDisplay, DefaultRootWindow(mDisplay),
> +                             &attribs);
> +        depth = attribs.depth;
> +        if (depth != 15 && depth != 16 && depth != 24 && depth != 32)
> +            depth = 24;
> +        XMatchVisualInfo(mDisplay, mScreen, depth, TrueColor, &vinfo);
> +
> +        xswa.background_pixel = 0;
> +        xswa.border_pixel = 0;
> +        xswamask = CWBackPixel | CWBorderPixel;
> +
> +        vo_x11_create_vo_window(&vinfo, vo_dx, vo_dy, d_width, d_height,
> +                                flags, CopyFromParent, "x11", title);
> +        XChangeWindowAttributes(mDisplay, vo_window, xswamask, &xswa);
> +
> +        XSync(mDisplay, False);
> +
> +#ifdef HAVE_XF86VM
> +        if (vm)
> +        {
> +            /* Grab the mouse pointer in our window */
> +            if (vo_grabpointer)
> +                XGrabPointer(mDisplay, vo_window, True, 0,
> +                             GrabModeAsync, GrabModeAsync,
> +                             vo_window, None, CurrentTime);
> +            XSetInputFocus(mDisplay, vo_window, RevertToNone, CurrentTime);
> +        }
> +#endif
> +    }
> +
> +    if ((flags & VOFLAG_FULLSCREEN) && WinID <= 0)
> +        vo_fs = 1;

Duplicated fro vo_xv, and the last one should probably not have the
"WinID <= 0" check, I just have not yet thought of a nice way to clean
this up.

> +    /* -----VDPAU related code here -------- */
> +    if (num_video_surfaces)
> +        videoSurfaces = (VdpVideoSurface *)malloc(sizeof(VdpVideoSurface)*num_video_surfaces);
> +    else
> +        videoSurfaces = NULL;

Useless cast, setting it to NULL should not be necessary unless there is
a memleak somewhere and calloc IMO would be a bit nicer here (and gives
you a integer overflow check for free, in case num_video_surfaces comes
unchecked from input).

> +    if (num_video_surfaces) {
> +        // Creation of VideoMixer.
> +        VdpVideoMixerParameter parameters[] = {
> +            VDP_VIDEO_MIXER_PARAMETER_VIDEO_SURFACE_WIDTH,
> +            VDP_VIDEO_MIXER_PARAMETER_VIDEO_SURFACE_HEIGHT,
> +            VDP_VIDEO_MIXER_PARAMETER_CHROMA_TYPE
> +        };

const

> +        void const * parameter_values[] = {
> +            &vid_width,
> +            &vid_height,
> +            &vdp_chroma_type
> +        };

When they can (i.e. if it still compiles), they should be "static".
Also the type should be e.g. const void * const

> +        surface_render = malloc(num_video_surfaces*sizeof(struct vdpau_render_state));
> +        memset(surface_render,0,num_video_surfaces*sizeof(struct vdpau_render_state));

calloc!

> +        vdp_st = vdp_video_mixer_create(
> +            vdp_device,
> +            0,
> +            0,
> +            ARSIZE(parameters),
> +            parameters,
> +            parameter_values,
> +            &videoMixer
> +        );

Since the size of parameters and parameter_values must be the same, I
would prefer a way that enforces that, e.g.
#define NUM_MIXER_PARAMS 3
static const VdpVideoMixerParameter parameters[NUM_MIXER_PARAMS]
static const void * const parameter_values[NUM_MIXER_PARAMS]
etc.


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

If it is not already NULL, something probably went wrong (memleak), so
if anything it should be an assert(!surface_render), and above all that,
not in the else part.

> +    vo_directrendering = 1;

This needs to be explained, since the dr API is not supported by this vo...

> +    if (w == 0 || h == 0)
> +        return;

!w || !h
seems more readable to me.

> +    indexData_size_required = 2*w*h;
> +    if (indexData_size < indexData_size_required) {
> +        indexData = (unsigned char *)realloc(indexData, indexData_size_required);

useless cast.

> +    // indexData creation, component order - I, A, I, A, .....
> +    for (i = 0; i < h; i++) {
> +        for (j = 0; j < w; j++) {
> +            indexData[i*2*w + j*2] = src[i*stride+j]; // index for palette-table
> +            a = srca[i*stride+j]; // alpha_data
> +            indexData[i*2*w + j*2 + 1] = (a == 0) ? 255 : a;

This looks like the classical misunderstood formula.
vo_direct3d should have things right.
It should be just
indexData[i*2*w + j*2 + 1] = -srca[i*stride+j];

> +    blendState.blend_factor_source_color = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA;
> +    blendState.blend_factor_source_alpha = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA;

And these factors should be something like
VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE.

> +    blendState.blend_factor_destination_color = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_SRC_ALPHA;
> +    blendState.blend_factor_destination_alpha = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_SRC_ALPHA;

And these VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA

> +    /* create palette table */
> +    palette = (uint32_t *)malloc(PALETTE_SIZE * sizeof(*palette));
> +    if (palette == NULL)
> +        return;

calloc, no cast, and why is this the only place in the whole file where
the malloc return value is checked?

> +static uint32_t start_slice(mp_image_t * mpi)
> +{
> +    mpi->flags &= ~MP_IMGFLAG_DRAW_CALLBACK;
> +    return VO_TRUE;
> +}

This looks suspicious...

> +    switch (format) {
> +    // non-VDPAU specific format. used in non-accelerated video.
> +    case IMGFMT_YV12:
> +    case IMGFMT_BGRA:
> +        return VFCAP_CSP_SUPPORTED | VFCAP_CSP_SUPPORTED_BY_HW | VFCAP_HWSCALE_UP | VFCAP_HWSCALE_DOWN;
> +    }

Looks to me like it supports OSD, so VFCAP_OSD should be here...

> +    if (videoSurfaces) {
> +        free(videoSurfaces);
> +        videoSurfaces = NULL;
> +    }
> +
> +    if (surface_render) {
> +        free(surface_render);
> +        surface_render = NULL;
> +    }
> +
> +    if (indexData) {
> +        free(indexData);
> +        indexData = NULL;
> +    }
> +
> +    if (palette) {
> +        free(palette);
> +        palette = NULL;
> +    }

IMO drop the if(), they server a documentation purpose at best, here
they just look like clutter to me.

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

> +static int control(uint32_t request, void *data, ...)
> +{
> +    switch (request)
> +    {
> +    case VOCTRL_PAUSE:
> +        return (int_pause = 1);
> +    case VOCTRL_RESUME:
> +        return (int_pause = 0);
> +    case VOCTRL_QUERY_FORMAT:
> +        return query_format(*((uint32_t *)data));

A few () too many IMHO.

> +    case VOCTRL_DRAW_IMAGE:
> +        return draw_image(data);
> +    case VOCTRL_START_SLICE:
> +        return start_slice(data);
> +    case VOCTRL_GUISUPPORT:
> +        return VO_TRUE;
> +    case VOCTRL_FULLSCREEN:
> +        vo_x11_fullscreen();
> +    case VOCTRL_SET_EQUALIZER:
> +    {
> +        va_list ap;
> +            int value;
> +
> +            va_start(ap, data);
> +            value = va_arg(ap, int);
> +
> +            va_end(ap);
> +            return vo_x11_set_equalizer(data, value);
> +    }
> +    case VOCTRL_GET_EQUALIZER:
> +    {
> +            va_list ap;
> +            int *value;
> +
> +            va_start(ap, data);
> +            value = va_arg(ap, int *);
> +
> +            va_end(ap);
> +            return vo_x11_get_equalizer(data, value);

Weird indentation.

> +    }
> +    case VOCTRL_ONTOP:
> +        vo_x11_ontop();
> +        return VO_TRUE;
> +    case VOCTRL_UPDATE_SCREENINFO:
> +        update_xinerama_info();
> +        return VO_TRUE;
> +    }

The code looks like panscan is implemented, but
VOCTRL_GET_PANSCAN/VOCTRL_SET_PANSCAN is missing here...

Greetings,
Reimar Döffinger



More information about the MPlayer-dev-eng mailing list