[MPlayer-dev-eng] [PATCH] Correct rounding for libavcodec/mpeg12.c

[Svante Signell]

On Tue, 2004-02-10 at 15:25, Michael Niedermayer wrote:
> Hi
> 
> On Tuesday 10 February 2004 13:52, Svante Signell wrote:
> > For mencoder using libavcodec some .avi files with a frame rate of e.g.
> > 23.976 the function find_frame_rate_index() incorrectly rounds the
> no, 23.976 is not a legal framerate for mpeg1/2 use -ofps 24000/1001
> 
> > calculated frame rate and compares with the standardised rates giving
> > output as shown below:
> >
> > videocodec: libavcodec (480x576 fourcc=3267706d [mpg2])
> > [mpeg2video @ 0x856d480]MPEG1/2 doesnt support 239759/10000 fps
> > Could not open codec.
> >
> > The correct value should have been 239760/10000.
> >
> no
The patch given rounds any given number to a given number of significant 
figures, it's easily settable.
With the numbers given, the value given above is correct to six
significant figures, since 24000/1001=23.976024 with eight significant
figures.

The patch did not change the frame rate. It did only make a fully
correct .avi file with a rate of 24000/1001 to be accepted by mencoder
(i.e. mpeg12.c using lavc). The number 23.976 is reported by mplayer
via: mplayer -identify  -vo null -ao null -frames 0 file.avi

This value is then given as input with a script to mencoder (and
truncated to 23.9759). Maybe mplayer should report 24000/1001 instead of
23.976??

The exact calculations are:
d = ABS(MPEG1_FRAME_RATE_BASE*(int64_t)s->avctx->frame_rate -
frame_rate_tab[i]*(int64_t)s->avctx->frame_rate_base);
where the first term is 239998759 and the other is 240000000.
The first term to 5,6,7,8 and 9 significant numbers are: 24000000,
239999000, 239998800, 239998760 and 239998759, respectively. Rounding to
five significant figures makes the file accepted, all other values
rejects it. 
I'll change to 24000/1001 to mencoder when mplayer reports 23.976, and
similar for 29.97 i.e. 30000/1001=29.97003.

> > Enclosed please find a small patch to libavcodec/mpeg12.c which
> > correctly rounds the calculations to five significant figures, resulting
> > in a frame_rate_index() of zero, instead of negative.
> 0 is not a legal frame_rate_index at all, and the idea of roundig the 
> framerate behind the applications back is very bad, it will lead to AV desync
Sorry for not being clear enough. Of course it is the return value that is either zero 
or negative not the index itself, just look at the patch.
> [...]