[MPlayer-dev-eng] [PATCH] Fractional part of time stamp in OSD

[Nicolas George]

Le nonidi 9 pluviôse, an CCXIX, Christian a écrit :
> Attached patch allows to optionally also show the fractional part of the
> time stamp in the OSD.

Seems potentially useful.

> +.B \-osd-fractions <0\-2>
           ^
You forgot that one.

> +show approximated frame count within current second. This frame count is not
> +accurate but only an approximation. For variable fps, the approximation is 
> +known to be far off the correct frame count.

I think the rule is tu start sentences on new lines.

>  Setzt die Anzeigedauer der OSD-Meldungen in ms (Standard: 1000).

My German is too rusty for me to give advice on that part.

> +                snprintf(fractions_text, 5, "%4.2f", mpctx->sh_video->pts - pts);
                                            ^
Potential desync between one part of the code and the other. Better use
sizeof.

> +                fractions_text_start++; //This is to skip beyond the leading 0 in eg 0.12

That seems awkward. Couldn't you use "%02d" and
(int)(diff * 100 + 0.5) % 100?

> +            } else if (osd_fractions==2)
> +                //print fractions by estimating the frame count within the second
> +                //
> +                //rounding or cutting off numbers after the decimal point causes problems
> +                //because of float's precision and movies, whose first frame is not exactly at timestamp 0
> +                //Therefore, we add 0.2 and cut off at the decimal point, which proved as good heuristic
> +                snprintf(fractions_text, 4, ".%02d", (int) ( ( mpctx->sh_video->pts - pts ) * mpctx->sh_video->fps + 0.2 ) );

Nitpick: your lines are too long, I had to enlarge my xterm to read them
comfortably. Also, I fear a multiline comment before a brace-less if/else
could lead to future bugs (people see several indented lines, they assume
they are grouped by braces).

But I grant you that the surrounding code is not a perfect example of
readability to start with.

Apart from that, it seems fine.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110128/7d6a5ee6/attachment.pgp>