[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