[MPlayer-dev-eng] [PATCH] EOSD support for VDPAU

[Reimar Döffinger]

On Sun, Feb 22, 2009 at 08:00:01AM +0100, Grigori G wrote:
> Power-of-two-sized surfaces are used to make it possible to reuse more  
> of the allocated surfaces. This is good since surface allocation is  
> expensive.

Do you have some actual benchmark numbers? Also trying to find a surface that
matches closely in size instead of just matching somehow might greatly
improve this without being much effort.
I'd also like to get some idea how the perfomance is like compared to
the vo gl method - if you do not want to implement the method just some
-benchmark numbers for

- gl with and without subtitles on
- vdpau with and without subtitles and with subtitles and the POT code
removed

Would be good enough for now. Disabling vsync might give more useful
numbers.

Also I think that a dynamically allocated array for EOSD should be just
a few modified lines, as I indicated below, so unless I missed something
there is no excuse to not implement it the first time :-)
Apart from that (and some additional comments below) the code does look quite good to me.

> +// EOSD
> +// Pool of surfaces
> +static struct {
> +    VdpBitmapSurface surface;
> +    int w;
> +    int h;
> +    char in_use;
> +} eosd_surfaces[EOSD_MAX_SURFACES];
> +
> +// List of surfaces to be rendered
> +static struct {
> +    VdpBitmapSurface surface;
> +    VdpRect source;
> +    VdpRect dest;
> +    VdpColor color;
> +} eosd_targets[EOSD_MAX_SURFACES];

Make them *eosd_...
static int eosd_max_surfaces;

> +static void eosd_init(void) {
> +    int i;
> +
> +    for (i=0; i<EOSD_MAX_SURFACES; i++)
> +        eosd_surfaces[i].surface = VDP_INVALID_HANDLE;
> +
> +    eosd_initialized = 1;
> +}

That is no longer necessary then, though you should set
eosd_max_surfaces, eosd_surfaces and eosd_targets to 0 in preinit.
Also, you do not free those surfaces in uninit it seems?

> +        // None found, allocate a new surface
> +        if (!found) {
> +            ax = ROUND_POT(i->w);
> +            ay = ROUND_POT(i->h);
> +            for (j=0; j<EOSD_MAX_SURFACES; j++) {
> +                if (eosd_surfaces[j].in_use == 0) {
> +                    if (eosd_surfaces[j].surface != VDP_INVALID_HANDLE)
> +                        vdp_bitmap_surface_destroy(eosd_surfaces[j].surface);
> +                    found = 1;
> +                    vdp_st = vdp_bitmap_surface_create(vdp_device, VDP_RGBA_FORMAT_A8,
> +                        ax, ay, VDP_TRUE, &eosd_surfaces[j].surface);
> +                    CHECK_ST_WARNING("EOSD: error when creating surface")
> +                    eosd_surfaces[j].w = ax;
> +                    eosd_surfaces[j].h = ay;
> +                    eosd_surfaces[j].in_use = 1;
> +                    eosd_targets[eosd_render_count].surface = eosd_surfaces[j].surface;
> +                    break;
> +                }
> +            }

if (!found) {
    j = eosd_max_surfaces;
    eosd_surfaces = realloc(eosd_surfaces, eosd_max_surfaces * sizeof(*eosd_surfaces));
    eosd_targets  = realloc(eosd_targets,  eosd_max_surfaces * sizeof(*eosd_targets));
}
and move the vdp_bitmap_surface_create and code below after here.

> +        vdp_st = vdp_bitmap_surface_putbits_native(eosd_targets[eosd_render_count].surface,
> +            (const void *) &(i->bitmap), &(i->stride), &destRect);

Those () are not needed.

> +eosd_skip_upload:
> +    eosd_render_count = 0;
> +    for (i = img; i; i = i->next) {
> +        // Render dest, color, etc.
> +        eosd_targets[eosd_render_count].color.alpha = 1.0 - ((i->color >> 0) & 0xff) / 255.0;
> +        eosd_targets[eosd_render_count].color.blue = ((i->color >> 8) & 0xff) / 255.0;
> +        eosd_targets[eosd_render_count].color.green = ((i->color >> 16) & 0xff) / 255.0;
> +        eosd_targets[eosd_render_count].color.red = ((i->color >> 24) & 0xff) / 255.0;
> +        eosd_targets[eosd_render_count].dest.x0 = i->dst_x;
> +        eosd_targets[eosd_render_count].dest.y0 = i->dst_y;
> +        eosd_targets[eosd_render_count].dest.x1 = i->w + i->dst_x;
> +        eosd_targets[eosd_render_count].dest.y1 = i->h + i->dst_y;
> +        eosd_targets[eosd_render_count].source.x0 = 0;
> +        eosd_targets[eosd_render_count].source.y0 = 0;
> +        eosd_targets[eosd_render_count].source.x1 = i->w;
> +        eosd_targets[eosd_render_count].source.y1 = i->h;
> +        eosd_render_count++;
> +
> +        if (eosd_render_count >= EOSD_MAX_SURFACES) {
> +            mp_msg(MSGT_VO, MSGL_WARN, "[vdpau] EOSD: too many surfaces!\n");
> +            break;
> +        }

I don't mind, but this check should never be possible, so IMO you might
as well remove it or use an assert instead.