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

[Reimar Döffinger]

On Sat, Jan 31, 2009 at 11:47:22PM +0100, Carl Eugen Hoyos wrote:
> +#define CHECK_ST_ERROR \
> +    if (vdp_st != VDP_STATUS_OK) { \
> +        mp_msg(MSGT_VO, MSGL_ERR, "[vdpau] Error at line %d: %s\n", \
> +               __LINE__, vdp_get_error_string(vdp_st)); \
> +        return -1; \
> +    }
> +
> +#define CHECK_ST_WARNING \
> +    if (vdp_st != VDP_STATUS_OK) \
> +        mp_msg(MSGT_VO, MSGL_WARN, "[vdpau]Warning at line %d: %s\n", \
> +               __LINE__, vdp_get_error_string(vdp_st));

Missing space after "[vdpau]".
And those macros are still a hack because obviously nobody can be
bothered to write messages that are actually helpful.
The line number is going to change all the time and neither humans nor
Google handle numbers very well, so after using vdp_get_error_string
this is now probably the second-worst way to handle it.

> +    for (dsc = vdp_func; dsc->pointer; dsc++) {
> +        vdp_st = vdp_get_proc_address(vdp_device, dsc->id, dsc->pointer);
> +        CHECK_ST_ERROR

No information on which function could not be found.

> +    static const VdpVideoMixerParameter parameters[VDP_NUM_MIXER_PARAMETER] = {
> +        VDP_VIDEO_MIXER_PARAMETER_VIDEO_SURFACE_WIDTH,
> +        VDP_VIDEO_MIXER_PARAMETER_VIDEO_SURFACE_HEIGHT,
> +        VDP_VIDEO_MIXER_PARAMETER_CHROMA_TYPE
> +    };
> +    void const* const parameter_values[VDP_NUM_MIXER_PARAMETER] = {&vid_width,
> +                                                                   &vid_height,
> +                                                                   &vdp_chroma_type

const void *, not void const*.
Also start with &vid_width on a new line, just like the array above.

> +    if (vo_config_count)
> +        return 0;

Still wrong. I won't review any further version unless they were tested
(preferably with valgrind to detect leaks) with -fixed-vo and different video format files.

> +static void OSD_init(void)
> +{
> +    int i;
> +
> +    if (vid_width == 0 || vid_height == 0)
> +        return;
> +
> +    /* create palette table */
> +    palette = calloc(PALETTE_SIZE, sizeof(*palette));
> +
> +    // full grayscale palette.
> +    for (i = 0; i < PALETTE_SIZE; ++i)
> +        palette[i] = (i << 16) | (i << 8) | i;
> +
> +    index_data = NULL;
> +    index_data_size = 0;
> +    osd_alloc = 1;

What's the point of the vid_width/vid_height check?

> +    vdp_st = vdp_presentation_queue_display(vdp_flip_queue, output_surface,
> +                                            FFMIN(max_width, FFMAX(out_rect.x1, out_rect_vid.x1)),
> +                                            FFMIN(max_width, FFMAX(out_rect.y1, out_rect_vid.y1)),

As noticed on the forums, there are quite a few width/height messups as
this one.

Greetings,
Reimar Döffinger