[MPlayer-users] wrong length and bitrate with FLAC

[Reimar Döffinger]

On Sun, Nov 25, 2012 at 07:33:50PM +0100, Ilja Sekler wrote:
> Am 25.11.2012 16:41, schrieb Reimar Döffinger:
> 
> > On Sun, Nov 25, 2012 at 04:13:19PM +0100, Ilja Sekler wrote:
> > 
> >> Am 25.11.2012 12:59, schrieb Reimar Döffinger:
> >> 
> >>> On Fri, Nov 23, 2012 at 11:39:43AM +0100, Ilja Sekler wrote:
> >>> 
> >>>>>> <http://lists.mplayerhq.hu/pipermail/mplayer-users/2012-November/085621.html>,
> >>>>>> reverting the checkin from r35333 fixes the issue with long
> >>>>>> FLAC files.
> >>>>> 
> >>>>> That would be wrong, you just don't notice because now
> >>>>> _your_ files are too short :-) (the issue would become
> >>>>> visible for files around 100 hours long).
> >>>> 
> >>>> It is IMHO quite safe to assume that no one would ever get
> >>>> FLAC files with such an incredible length :-) For real life
> >>>> cases, it just works right.
> >>> 
> >>> Yes, but there is a difference between "working right" and 
> >>> "correct", the former one usually making the code a nightmare to 
> >>> maintain in the long run.
> >> 
> >> I'm aware of it and very sorry if it sounded like neglecting
> >> flawless logic. The problem is that I mostly can't judge what is
> >> logically correct in C, but can easily see whether something is
> >> "working right" when using an application for my everyday computing
> >> needs.
> > 
> > In this case, the main reason is that coverity complained about the 
> > code, and using stream_read_int24 to then discard everything but the 
> > lowest 16 bits is senseless.
> 
> Did it happen at libmpdemux/demux_audio.c:606
> 
> -	      num_samples |= stream_read_word(s);
> 
> prior to the present fix, where stream_read_word and stream_read_int24
> were defined in stream/stream.h?

No, the problem is with
stream_read_int24(s) << 16;
The problem is that without the cast, the left shift is done as a
32 bit operation.
Shifting a 24 bit value 16 to the left thus resulted in the top 8
"falling off the cliff".
Which was kind of lucky since the top 4 would have caused trouble
(in the way that happened when I added the cast, thus making the
shift a 64 bit operation), but it would still lose 4 bits.

> > So the thing is that the number of audio samples value in FLAC is 36 
> > bits (i.e. 5 bytes but 4 bits used for something else).
> 
> So this is 2^36-1 = 68719476735 max and the definition comes from
> <http://flac.sourceforge.net/format.html#metadata_block_streaminfo>. If
> this buffer is filled with 44100 samples per second or 44100*2 samples
> per second for two channels (I don't know),

It should be independent of the channel count.

> it will be full if the
> lenght of the file is 432,8 or 216,4 hours correspondingly. It is still
> at least twice as long as aforementioned 100 hours.

So your 400 hour number should be correct. But 36 bits is the _stored_,
correct value - FLAC files longer than this will not work 100% right
no matter what.
But before the changes we read only 32 bit, i.e. for a sample count of
2^32 we would already think the file has 0 samples.
So if like you assuming 44100 as sample-rate that would be something
around 27 hours. The reason that it does not match my 100 hours is that
I did not use a calculator and I ended up rounding up quite a bit.