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

[Carl Eugen Hoyos]

Reimar Döffinger <Reimar.Doeffinger <at> stud.uni-karlsruhe.de> writes:

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

They are "static aligned" now;-)

> And would it maybe be nicer to put them all into a struct, making it
> easier to allow multiple vo instances "somewhen"?

Not done for the moment. Are they not function pointers that are identical for
each instance?

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

Then you know already more than I do;-)

[...]

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

It doesn't break yet: No -vc vdpau yet
I don't know how to fix it correctly: What about failing?

> > +#ifdef HAVE_NEW_GUI
[a lot of code]
> > +    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.

I did not touch this yet: It looks a bit different in vdpau, xv and xvmc, so I
need more advice.

[...]

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

Changed, I hope it's truly correct now.

[...]

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

Not understood...

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

I did not look at this yet.

New patch will follow soon, thank you for reviewing, Carl Eugen