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

[Diego Biurrun]

On Thu, Jan 22, 2009 at 03:34:27AM +0100, Carl Eugen Hoyos wrote:
>
> Attached is the fifth round of the vo_vdpau patch.
>
> --- libvo/vo_vdpau.c	(revision 0)
> +++ libvo/vo_vdpau.c	(revision 0)
> @@ -0,0 +1,876 @@
> +/*
> + * VDPAU Renderer for MPlayer.
> + * vo_vdpau.c - VDPAU with X11 interface.
> + *
> + * Copyright (C) 2008 NVIDIA.

Pointless periods, drop the filename.

> + * Actual decoding and presentation are implemented here.
> + * All necessary frame informations are collected through
> + * "vdpau_render_state" structure after parsing all headers

Information has no plural, "the structure".

> + * etc. in libavcodec for different codecs.
> + *
> + * @{
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "config.h"
> +#include "mp_msg.h"
> +#include "help_mp.h"
> +#include "video_out.h"
> +#include "video_out_internal.h"
> +
> +#include <X11/Xlib.h>
> +#include <X11/Xutil.h>
> +#include <errno.h>
> +
> +#include "x11_common.h"
> +
> +#include "libavcodec/vdpau.h"
> +
> +#include "fastmemcpy.h"
> +#include "sub.h"
> +#include "aspect.h"
> +
> +#include "subopt-helper.h"
> +
> +#include "input/input.h"

Are all of these needed or just copy and paste?

> +#ifdef HAVE_NEW_GUI
> +#include "gui/interface.h"
> +#endif

This preprocessor directive does not exist, so this codepath appears
to be untested.

> +/* Numbers of video and output Surfaces */

number, surfaces

> +/* Numbers of palette entries */

number

> +/* VideoMixer puts videoSurface data to displayable ouputSurface. */

s/to/on/, ouTputSurface?

> +/* OutputSurfaces[2] is used in composite-picture. */
> +static VdpOutputSurfaceRenderOutputSurface       *vdp_output_surface_render_output_surface;
> +static VdpOutputSurfacePutBitsIndexed    *vdp_output_surface_put_bits_indexed;

Align.

> +static VdpVideoSurface *videoSurfaces;
> +static VdpOutputSurface outputSurfaces[NUM_OUTPUT_SURFACES];
> +static VdpVideoSurface videoSurface;
> +static VdpOutputSurface outputSurface;
> +
> +static VdpDecoder decoder;
> +static VdpVideoMixer videoMixer;

ditto

Also, camelcase variable names suck.

> +static struct vdpau_render_state * surface_render;

inconsistent * placement

> +static void calc_drwXY_dWH(uint32_t *drwX, uint32_t *drwY, uint32_t *d_wh, uint32_t *d_ht)

long line, more below

> +/* Initialize Get Proc Address, called from config() */

Get Proc Address?

> +/* Destroy vdpau procs, called from uninit() */

processes, procedures?

> +#ifdef HAVE_XF86VM
> +    int vm = flags & VOFLAG_MODESWITCHING;
> +#endif

again, wrong preprocessor directive

> +        /* Non VDPAU specific formats
> +        * No HW acceleration. VdpDecoder will not be created and
> +        * there will be no call for VdpDecoderRender

call to?

> +#ifdef HAVE_NEW_GUI
> +    if (use_gui)
> +        guiGetEvent(guiSetShVideo, 0);  // let the GUI setup/resize our window
> +    else
> +#endif
> +    {
> +#ifdef HAVE_XF86VM
> +        if (vm)
> +            vo_vm_switch();
> +        else
> +#endif

see above

> +#ifdef HAVE_XF86VM
> +        if (vm)
> +        {

inconsistent indentation

> +        vdp_st = vdp_video_surface_create(vdp_device,
> +                                          vdp_chroma_type,
> +                                          vid_width,
> +                                          vid_height,
> +                                          &videoSurfaces[i]);

No need to use so many lines, more below.

> +#if TRACE_SURFACES
> +        printf("VID CREATE: %u\n", videoSurfaces[i]);
> +#endif

debug junk?  same below

> +    max_width = FFMAX(max_width, d_width);
> +    max_height = FFMAX(max_height, d_width);

align

> +            vdp_st = vdp_presentation_queue_display(
> +                vdp_flip_queue,
> +                outputSurface, // outputSurfaces[0 / 1],
> +                FFMIN(max_width, FFMAX(outRect.x1, outRectVid.x1)),
> +                FFMIN(max_width, FFMAX(outRect.y1, outRectVid.y1)),
> +                0);

weird indentation, more below

> +    blendState.struct_version = VDP_OUTPUT_SURFACE_RENDER_BLEND_STATE_VERSION;
> +    blendState.blend_factor_source_color = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE;
> +    blendState.blend_factor_source_alpha = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE;
> +    blendState.blend_factor_destination_color = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA;
> +    blendState.blend_factor_destination_alpha = VDP_OUTPUT_SURFACE_RENDER_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA;
> +    blendState.blend_equation_color = VDP_OUTPUT_SURFACE_RENDER_BLEND_EQUATION_ADD;
> +    blendState.blend_equation_alpha = VDP_OUTPUT_SURFACE_RENDER_BLEND_EQUATION_ADD;

align

> +static void OSD_init()

(void)

> +static void DestroyVdpauObjects()

ditto

> +static int preinit(const char *arg)
> +{
> +    if (arg)
> +    {

indentation

> +static int control(uint32_t request, void *data, ...)
> +{
> +    switch (request)
> +    {

ditto

> --- configure	(revision 28347)
> +++ configure	(working copy)
> @@ -4243,6 +4247,30 @@
> +int main(void) {
> +  (void)vdp_device_create_x11(0, 0, 0, 0);
> +  return 0; }

Do you need that cast?  Also, this fits on one line.

> +  _vosrc="$_vosrc vo_vdpau.c"

outdated

Diego