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

[Diego Biurrun]

On Mon, Apr 20, 2009 at 08:30:16AM +0200, Dan Oscarsson wrote:
> 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.

Look into git, mercurial, quilt, bazaar or do it by hand.  It's not
impossible to create a patch series by hand either.

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

There are plenty.

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

As I said, I know this.  However, it is nonetheless wrong, should be
fixed and will be fixed eventually.  In the meantime, we should not
exacerbate the problem.

> You need a typedef if you want to avoid having to write stuct
> everywhere.

Sure.

> > > @@ -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?

the reindentation

> > > @@ -528,6 +597,108 @@
> > >  
> > > +static void get_vdpau_timing(void) {
> > 
> > K&R function declarations please.
> 
> You want the { on next line or what?

yes

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

I tend to disagree, but I don't mind much.

> > > --- 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?

uint64_t is the standard type, see stdint.h.  Dunno where you got
u_int64_t from...

Diego