1423834 - Wrong calculation for mp3 time length as we prevent overflow

Status: NEW

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[C.M.Chang[:chunmin]]

The way we prevent overflow will cause the miscalculation of mp3 length.

Suppose we have x_1, x_2, ..., x_n and the new data `x_{n+1}` will lead to overflow.

The average of these n+1 data is: (x_1 + x_2 + ... + x_n + x_{n+1}) / (n + 1)

Currently, we'll halve the summation and the number of data before counting `x_{n+1}` and use the *half* summation and the number to calculate average by[0]:
(X + x_{n+1}) / (N + 1), where X = (x_1 + x_2 + ... + x_n) / 2 and N = n / 2.

Let E be the above estimation, E = (X + x_{n+1}) / (N + 1) = 
((x_1 + x_2 + ... + x_n)/2 + x_{n+1}) / (n/2 + 1) =
((x_1 + x_2 + ... + x_n + 2*x_{n+1})/ 2) / ((n+2)/2) = 
(x_1 + x_2 + ... + x_n + 2*x_{n+1}) / (n + 2)

Let Avg be the true average, Avg = (x_1 + x_2 + ... + x_n + x_{n+1}) / (n + 1)

We could compare this two values to see if they are equal:

Avg 
= (n + 2)(x_1 + x_2 + ... + x_n + x_{n+1}) / (n + 1)(n + 2)
= [(n + 1)(x_1 + x_2 + ... + x_n + x_{n+1}) + (x_1 + x_2 + ... + x_n + x_{n+1})] / (n + 1)(n + 2)

E 
= (n + 1)(x_1 + x_2 + ... + x_n + x_{n+1} + x_{n+1}) / (n + 1)(n + 2)
= [(n + 1)(x_1 + x_2 + ... + x_n + x_{n+1}) + (n + 1) * x_{n+1}] / (n + 1)(n + 2)

Avg - E = [(x_1 - x_{n+1}) + (x_2 - x_{n+1}) + ... + (x_n - x_{n+1})] / (n + 1)(n + 2).
 
They are equal *only* when x_{n+1} = x_1 = x_2 = ... = x_n.


Cumulative moving average[1] is a better choice to avoid the overflow.

Avg(n+1) 
= (n * Avg(n) + new-data) / (n+1) 
= ((n+1) * Avg(n) - Avg(n) + new-data) / (n+1)
= Avg(n) + (new-data - Avg(n)) / (n+1)

It's a better way to get average if the `n` won't overflow.

(Sorry about the ugly format. I wish I can use mathjax or latex here)

[0] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/mp3/MP3Demuxer.cpp#709-728,756-760
[1] https://en.wikipedia.org/wiki/Moving_average#Cumulative_moving_average

Comment 1

[Jean-Yves Avenard [:jya]]

there's a few classes to calculate an average that won't have the problem you describe.
https://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.h#587

there's a few more like that.

Comment 2

[C.M.Chang[:chunmin]]

To evaluate if this is worth fixing, I test several mp3 files to see if we can have a larger mTotalFrameLen[0] than StreamLength()[1]. The answer is yes. The mTotalFrameLen may be larger than StreamLength() in some file.

(If mTotalFrameLen is always smaller than StreamLength(), StreamLength() will also overflow when mTotalFrameLen overflows. Then the calculation for mp3 duration will be incorrect since StreamLength() is a overflowed value, regardless of the estimated average frame length is correct or not[2].)

[0] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/mp3/MP3Demuxer.h#143
[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/mp3/MP3Demuxer.h#50
[2] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/mp3/MP3Demuxer.cpp#389,395