[MPlayer-users] wrong length and bitrate with FLAC

[Ilja Sekler]

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?

If my question is completely stupid as of any person ignorant of C,
please just tell me to stop asking before having learned the basics,
which I should do anyway.

>> Could you please shed a bit of light on how you calculated the
>> lenght of a FLAC file sufficient to make things go wrong?
> 
> I can't fully parse that sentence.

I referred to the sentence "the issue would become visible for files
around 100 hours long" and wanted to know how you could estimate this
duration, which should be enough to make some variables wrap, overflow
or whatever.

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

> The old code read this by reading 3 bytes and appending the next 2
> bytes.
> However of the first 3 bytes read it actually ended up discarding the
> first byte, thus actually only using 32 of the 36 bits of the sample
> count.
> After my first "fix" that actually broke it, it would use all bytes 
> read, which is 5*8 = 40 bits. If those extra bits were not 0, things 
> would break.
> The latest version will now instead read 1 byte, but discard the
> highest 4 bits, and then append the next 4 bytes, giving the desired
> 8 - 4 + 32 = 36 bits.

Thank you very much for your patient and comprehensive explanation.

-- 
Regards

Ilja