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

[The Wanderer]

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.
> 
> There should not be any tabs or tailing whitespace unless my Linux tools
> add them when I copy between windows.

At a quick scan, I see trailing whitespace in only one place, in the
fifth hunk of the mplayer.c patch:

> +if (match_vsync_speed && vo_vsync_interval > 0) {
> +    if (!vsynced_speed) {
> +        float fps_interval, vo_interval;
> +
> +        fps_interval = 1000000.0f/vo_fps;
> +        vo_interval  = vo_vsync_interval/1000.0f;
> +        if (fps_interval*0.95 < vo_interval && vo_interval < fps_interval*1.05) {
> +            matched_playback_speed = fps_interval/vo_interval;
> +            mp_msg(MSGT_CPLAYER,MSGL_INFO,"Matching speed to vsync %3.10f\n",matched_playback_speed);
> +        } else if (fps_interval/2*0.95 < vo_interval && vo_interval < fps_interval/2*1.05) {        

on the last line above.

The only tabs I see (except on unchanged lines) are in the PLAY VIDEO
section, which (now that I look closer) actually seems to be likewise
unchanged - just moved. I don't recall whether standard practice is to
remove tabs on lines which are being moved but otherwise not modified,
but it would at the least seem like a cosmetic change to me...

>>> @@ -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 vdp_st = vdp_output_surface_create() call, and the following
mp_msg(), seem unchanged except for being reindented. That reindentation
constitutes a cosmetic change, and so needs to be done as a separate
patch following the functional changes.

-- 
       The Wanderer

Warning: Simply because I argue an issue does not mean I agree with any
side of it.

Secrecy is the beginning of tyranny.