[MPlayer-dev-eng] [PATCH] vdpau fixes and timing enhancements

[Dan Oscarsson]

On 2009-04-18 at 16:34 +0200 Diego Biurrun wrote:
> > The patches include:
> 
> ... a lot of things ..
> 
> Please split the patch into manageable pieces.  Also, please avoid tabs
> and trailing whitespace.

I did that before but got nowhere. It is a lot of work to split it into
parts, though the mplayer.c patches are fairly easy to split out. That I
did before.

There should not be any tabs or tailing whitespace unless my Linux tools
add them when I copy between windows.


> > +} output_surface_t;
> 
> _t is POSIX-reserved namespace.  I don't think you need a typedef..
> 
> Yes, we have many identifiers that violate this, but we should not add
> more.

I just followed the standard I saw in many (yes many) places in mplayer.
You need a typedef if you want to avoid having to write stuct
everywhere.

> 
> > @@ -288,26 +352,21 @@
> >      if (output_surface_width < vo_dwidth || output_surface_height < vo_dheight) {
> >          if (output_surface_width < vo_dwidth) {
> >              output_surface_width += output_surface_width >> 1;
> > -            output_surface_width = FFMAX(output_surface_width, vo_dwidth);
> > +            output_surface_width = FFMIN(vo_screenwidth,FFMAX(output_surface_width, vo_dwidth));
> >          }
> >          if (output_surface_height < vo_dheight) {
> >              output_surface_height += output_surface_height >> 1;
> > -            output_surface_height = FFMAX(output_surface_height, vo_dheight);
> > -        }
> > -        // Creation of output_surfaces
> > -        for (i = 0; i <= NUM_OUTPUT_SURFACES; i++) {
> > -            if (output_surfaces[i] != VDP_INVALID_HANDLE)
> > -                vdp_output_surface_destroy(output_surfaces[i]);
> > -            vdp_st = vdp_output_surface_create(vdp_device, VDP_RGBA_FORMAT_B8G8R8A8,
> > -                                               output_surface_width, output_surface_height,
> > -                                               &output_surfaces[i]);
> > -            CHECK_ST_WARNING("Error when calling vdp_output_surface_create")
> > -            mp_msg(MSGT_VO, MSGL_DBG2, "OUT CREATE: %u\n", output_surfaces[i]);
> > +            output_surface_height = FFMIN(vo_screenheight,FFMAX(output_surface_height, vo_dheight));
> >          }
> > +        // Creation of osd surface
> > +        if (osd_surface != VDP_INVALID_HANDLE)
> > +            vdp_output_surface_destroy(osd_surface);
> > +        vdp_st = vdp_output_surface_create(vdp_device, VDP_RGBA_FORMAT_B8G8R8A8,
> > +                                           output_surface_width, output_surface_height,
> > +                                           &osd_surface);
> > +        CHECK_ST_WARNING("Error when calling vdp_output_surface_create")
> > +        mp_msg(MSGT_VO, MSGL_DBG2, "OUT CREATE OSD: %u\n", osd_surface);
> 
> cosmetics

What is bad here?

> 
> > @@ -528,6 +597,108 @@
> >  
> > +static void get_vdpau_timing(void) {
> 
> K&R function declarations please.

You want the { on next line or what?

> > +    if (max_delay) {
> > +        num_output_surfaces = 3+((int)(max_delay+0.05));
> > +    } else {
> > +        num_output_surfaces = 2;
> > +    }
> 
> pointless {}

Yes but much clearer for the eye. Also much easier to add new lines in
each conditional part.

> 
> > --- osdep/timer.h.org	2009-04-18 15:14:05.000000000 +0200
> > +++ osdep/timer.h	2009-04-18 15:14:50.000000000 +0200
> > @@ -21,6 +21,8 @@
> >  
> > +u_int64_t get_time(void);
> 
> uint64_t?

What is best name? Is uint64_t more portable?