1222866 - Audio pause and distortion in MP3 stream due to rounding error since Firefox 41

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Alex Pokotilo]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151015172900

Steps to reproduce:

I'm Nimble Streamer developer responsible for icecast support. I've checked my icecast implementation with FF regularly and everything was fine till 41 release. Please take a look at the following example http://104.131.143.22/ff41issue/stream/icecast.audio it's mp3 icecast stream.



Actual results:

Stream plays fine for 38,39, 40.0.3 releases but start reproducing strange noises(drop-outs and jitter/stuttering) on 41 version.


Expected results:

I'd expect current (41) version plays my icecast stream correctly

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ce11a9ce760091dfd2a0acd0dd3724cace8bcd6a&tochange=a5eb0b1fcf3954327d52436d0f99350479ea4768

Suspected bug: Bug 1172264

Comment 2

[Jean-Yves Avenard [:jya]]

Could you re-test with 42?

I believe all mp3 related problems following bug 1172264 are finally gone.

Comment 3

[Loic]

I tested in Nighty, it's reproducible, music cut-offs are audible.

Comment 4

[Jean-Yves Avenard [:jya]]

in 42: I hear some glitches as it attempts to play at the end, when the bandwidth is too low (like when I'm downloading in parallel)

not as bad as 41.. but still there :(

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Loic from comment #3)
> I tested in Nighty, it's reproducible, music cut-offs are audible.

41 and 42; but nightly is using the new MP3Demuxer, so while there may be a similar symptom, it's unlikely to be the same. Eugen could you have a look?

Comment 6

[Alex Pokotilo]

I've just checked with 44.0a2 and problem is the same.
As Nimble server developer I can describe how icecast playback works if necessary.

Comment 7

[Jean-Yves Avenard [:jya]]

It only shows glitch at the start with 42. After a minute or so I never hear more glitches.

Comment 8

[Alex Pokotilo]

Could you please provide download link for 42 release. When I tried to get development release I got 44.0a2.
But as I reported I see this issue there as well

Comment 9

[Jean-Yves Avenard [:jya]]

42 is the current release ; this is what you get when going to www.mozilla.com and downloading the current release.

41 is the previous release.

Comment 10

[Alex Pokotilo]

I'll check with 42 and get back to you with the results

Comment 11

[Alex Pokotilo]

I've restarted playback several times and
I had glitches during the first minute and every several minutes after that. I've checked the same stream again with FF 40 and don't hear such issues

Comment 12

[tomer]

Having the same issue on 41 and 42 . on 42 it takes around a minute until the glitches start

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this here on a Nexus 6P, but that might also have to do with 6P weirdness.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

I guess we need to figure out if this is a decoder underrun or output underrun?

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think we can rule out network problems given that I just had stuttering right up until the end of the track.

Comment 16

[Eugen Sawin [:esawin]]

The stuttering is caused by repeated mid-stream seeking. Ignoring the seemingly randomly emitted seeking requests (in MediaDecoderStateMachine) fixes the issue.

Comment 17

[Jean-Yves Avenard [:jya]]

Where does the seeking comes from? Why would it be seeking?

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch

(In reply to Jean-Yves Avenard [:jya] from comment #17)
> Where does the seeking comes from?

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1131

> Why would it be seeking?

Rounding error.

This patch would fix it.

Comment 19

[Alex Pokotilo]

when can we see this patch released ?

Comment 20

[Alejandro]

Hi team, I has the same issue, we loose a lot of traffic into our radios from all the users that use FF, some delivery date for this fix?

Into icecast-kh media server, was reported the same issue, here the issue: https://github.com/karlheyes/icecast-kh/issues/123

Possible help the information reported there too.

Thanks

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8686288 [details] [diff] [review]
0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch

Review of attachment 8686288 [details] [diff] [review]:
-----------------------------------------------------------------

With MP3, the duration being so much subject to estimation (seeing that's it's based on average bitrates). There's still big potential for the error to still occur even if allocating for that rounding error.

Why do we even bother to seek I wonder.
JW can you shed some lights on the logic behind this? e.g. what is duration ever expected to be shorten so much that it requires to seek backward. I can only foresee this to be a requirement for MSE. Not with an other media (in fact I'm pretty sure this used to only be done with MSE earlier on)

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

The duration is calculated by MP3FrameParser, right? It might give wrong duration when there is a gap in the data fed to the parser.

Comment 23

[Jean-Yves Avenard [:jya]]

Here there should be no gap as no seeking is occurring.
However, being VBR, the average bitrate will slightly vary over time ; which would cause the calculated duration to slightly change.

Comment 24

[Alex Pokotilo]

icecast-kh author removed "Accept-Ranges: byte" from icecast header response and it's fixed the problem.
I did the same and once I removed "Accept-Ranges: byte" FF starts working fine.
Are you sure problem is in rounding after all? I'll release new version with fix today so this case not a problem for me any more. Please check your analysis about rounding

Comment 25

[Alejandro]

Hi all, I can confirm after test both products Icecast-KH and Nimble, after they removed "Accept-Ranges: byte" now in FF is working well again the streaming.

Comment 26

[Jean-Yves Avenard [:jya]]

This just disabled seeking. 
At least that's a quick turn around.

Comment 27

[Eugen Sawin [:esawin]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> With MP3, the duration being so much subject to estimation (seeing that's
> it's based on average bitrates). There's still big potential for the error
> to still occur even if allocating for that rounding error.

The MP3Demuxer sets sample time stamps and overall duration based on the same metrics, so the error should not occur due to VBR and that's not what is happening here.

(In reply to Alex Pokotilo from comment #24)
> icecast-kh author removed "Accept-Ranges: byte" from icecast header response
> and it's fixed the problem.
> I did the same and once I removed "Accept-Ranges: byte" FF starts working
> fine.
> Are you sure problem is in rounding after all? I'll release new version with
> fix today so this case not a problem for me any more. Please check your
> analysis about rounding

This change would simply prevent us from seeking, there is still a Gecko bug when we are allowed to seek on (stream) resources with steadily increasing stream lengths.

Comment 28

[Alex Pokotilo]

First of all I'd say it's cool FF dev team works very fast and professional !
It's cool I reported real issue and this will make FF better.
When are you planning to release this fix ?

Comment 29

[Alex Pokotilo]

BTW this is how I calculated VBR mp3 duration in our product
const uint32_t MPEG_SAMPLING_RATES[4][4] = {
    //MPEG2.5   #INVALID  MPEG 2   MPEG 1
    {11025,    0,        22050,   44100}, // 00
    {12000,    0,        24000,   48000}, // 01
    {8000 ,    0,        16000,   32000}, // 10
    {0,        0,           0,        0}  // 11
};
uint64_t MP3Duration::getMpeg2tsDuration() {
    return (uint64_t)
            (((double)mp3_samples_bucket_[0][0] / MPEG_SAMPLING_RATES[0][0]) +
             ((double)mp3_samples_bucket_[0][2] / MPEG_SAMPLING_RATES[0][2]) +
             ((double)mp3_samples_bucket_[0][3] / MPEG_SAMPLING_RATES[0][3]) +
             ((double)mp3_samples_bucket_[1][0] / MPEG_SAMPLING_RATES[1][0]) +
             ((double)mp3_samples_bucket_[1][2] / MPEG_SAMPLING_RATES[1][2]) +
             ((double)mp3_samples_bucket_[1][3] / MPEG_SAMPLING_RATES[1][3]) +
             ((double)mp3_samples_bucket_[2][0] / MPEG_SAMPLING_RATES[2][0]) +
             ((double)mp3_samples_bucket_[2][2] / MPEG_SAMPLING_RATES[2][2]) +
             ((double)mp3_samples_bucket_[2][3] / MPEG_SAMPLING_RATES[2][3]));
}
so I add each sample into appropriate mp3_samples_bucket and I always know exact and precise duration even in case of VBR

Comment 30

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686288 [details] [diff] [review]
0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch

Review of attachment 8686288 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +1127,4 @@
>      mOwner->DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
>    }
>  
> +  if (CurrentPosition() > 1 + TimeUnit::FromSeconds(mDuration).ToMicroseconds()) {

This doesn't seem the right place. We have MediaDecoder::UpdateEstimatedMediaDuration() to handle fuzzy values.

Comment 31

[Eugen Sawin [:esawin]]

(In reply to JW Wang [:jwwang] from comment #30)
> This doesn't seem the right place. We have
> MediaDecoder::UpdateEstimatedMediaDuration() to handle fuzzy values.

This isn't an estimation error, it's the result of floating point rounding, so I don't think this is related.

DurationChanged() also uses equality comparison on floating points, which is also unsafe, but unrelated to this bug.

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

Can you show me the numbers so I have better understanding about what actually happened?

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression (from 41), tracking for current versions, looks like we have a possible fix. Once  this lands please test it out and see if you think it's a good candidate for uplift.

Comment 34

[Jean-Yves Avenard [:jya]]

(In reply to Alex Pokotilo from comment #24)
> icecast-kh author removed "Accept-Ranges: byte" from icecast header response
> and it's fixed the problem.
> I did the same and once I removed "Accept-Ranges: byte" FF starts working
> fine.
> Are you sure problem is in rounding after all? I'll release new version with
> fix today so this case not a problem for me any more. Please check your
> analysis about rounding

Would there be any chance for you to provide a stream with the original code and no work-around?

just so we can check that the fix is valid ?

thank you

Comment 35

[tomer]

stream with previous (before fix) nimble server
http://54.69.139.39/test/mp3/icecast.audio

Comment 36

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686288 [details] [diff] [review]
0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch

Review of attachment 8686288 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +1127,4 @@
>      mOwner->DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
>    }
>  
> +  if (CurrentPosition() > 1 + TimeUnit::FromSeconds(mDuration).ToMicroseconds()) {

For CurrentPosition()==4169470 and mDuration== 4.169470, it is surprising |TimeUnit::FromSeconds(mDuration).ToMicroseconds()| returns 4169469. Please add a comment why we need the "+1" to cope with the rounding error.

Comment 37

[JW Wang [:jwwang] [:jw_wang]]

Btw, I think we should avoid using floating points internally to avoid rounding errors.

Comment 38

[Alex Pokotilo]

(In reply to Jean-Yves Avenard [:jya] from comment #34)
> (In reply to Alex Pokotilo from comment #24)
> > icecast-kh author removed "Accept-Ranges: byte" from icecast header response
> > and it's fixed the problem.
> > I did the same and once I removed "Accept-Ranges: byte" FF starts working
> > fine.
> > Are you sure problem is in rounding after all? I'll release new version with
> > fix today so this case not a problem for me any more. Please check your
> > analysis about rounding
> 
> Would there be any chance for you to provide a stream with the original code
> and no work-around?
> 
> just so we can check that the fix is valid ?
> 
> thank you

Absolutely !
http://162.243.216.185/ff41issue/stream/icecast.audio

Comment 39

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #36)
> Comment on attachment 8686288 [details] [diff] [review]
> 0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch
> 
> Review of attachment 8686288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +1127,4 @@
> >      mOwner->DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
> >    }
> >  
> > +  if (CurrentPosition() > 1 + TimeUnit::FromSeconds(mDuration).ToMicroseconds()) {
> 
> For CurrentPosition()==4169470 and mDuration== 4.169470, it is surprising
> |TimeUnit::FromSeconds(mDuration).ToMicroseconds()| returns 4169469. Please
> add a comment why we need the "+1" to cope with the rounding error.

welcome to BCD vs power of 2 arguments...
There's no way to express 4.169470 exactly in binary representation.

TimeUnit uses microseconds, which is a int64_t

Alternatively when creating a TimeUnit from a double, we could round the value to the nearest integer. We currently simply perform calculation using ints which would always round to the lesser integer.

It would be more elegant I think than this approach.

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8686288 [details] [diff] [review]
0001-Bug-1222866-Avoid-rounding-error-in-seek-position-co.patch

let's fix it in TimeUnit instead. Need to check with Gerald if it could have unforeseen consequences.

As JW mentioned, we should never be using doubles internally, only when reporting the value out to JS

Comment 41

[Jean-Yves Avenard [:jya]]

gerald, what do you think about rounding up seconds like here:
4.169470

The internal double representation is actually 4.169469833374023 so, using ints to perform the calculation that gives 4169469 us

Can you foresee a problem?

Comment 42

[Jean-Yves Avenard [:jya]]

(above I used floats 32 bits), representation is 0x40856c4c

Comment 43

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Jean-Yves Avenard [:jya] from comment #41)
> gerald, what do you think about rounding up seconds like here:
> 4.169470
> 
> The internal double representation is actually 4.169469833374023 so, using
> ints to perform the calculation that gives 4169469 us
> 
> Can you foresee a problem?

Can't see anything wrong, unless these times started getting very very big, such that the sub-microsecond precision would be lost. But with doubles, that'd be about 285 years, so I think we should be fine for a while.

Ideally, it'd be great to store integral values internally, and convert them to/from floating-point numbers when talking to Javascript. But it would probably be too much work, and might introduce other issues (e.g. js sets a value, but reading it back would give a slightly different value -- Would that be a bad thing?)

Comment 44

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #43)

> Ideally, it'd be great to store integral values internally, and convert them
> to/from floating-point numbers when talking to Javascript. But it would
> probably be too much work, and might introduce other issues (e.g. js sets a
> value, but reading it back would give a slightly different value -- Would
> that be a bad thing?)

there are many cases where that won't be possible.
currentTime being the official play time, setting currentTime to a value in JS means you have to be able to read it back and get the same value.
Same when setting the duration.
if you were to do video.duration = 4.169470 and read back 4.169469 you wouldn't be per spec.

Comment 45

[Jean-Yves Avenard [:jya]]

Attachment: P1. Round second to closest microseconds.

Due to the internal double representation as per IEEE 754, during conversion the use of ints would have rounded down our value.

Comment 46

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add gtest checking on seconds -> microseconds -> seconds.

Comment 47

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8689808 [details] [diff] [review]
P2. Add gtest checking on seconds -> microseconds -> seconds.

Review of attachment 8689808 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for writing tests.

Comment 48

[Jean-Yves Avenard [:jya]]

Confirmed that the stream now plays without interruption.

The joy of computer arithmetic really..

double val = (aValue + .0000005) * USECS_PER_S giving different result to double val = aValue * USECS_PER_S + 0.5;

or that (4.169470 / 1000000) * 1000000 != 4.169470

Comment 49

[Alex Pokotilo]

why don't you implement such things using method described https://bugzilla.mozilla.org/show_bug.cgi?id=1222866#c29 ? or I don't get something ?

Comment 50

[Jean-Yves Avenard [:jya]]

(In reply to Alex Pokotilo from comment #49)
> why don't you implement such things using method described
> https://bugzilla.mozilla.org/show_bug.cgi?id=1222866#c29 ? or I don't get
> something ?

the issue isn't just with mp3 or how mp3 duration was read, but that we use in some instances 64 bits floating point to store time, and in other using 64 bits integer and work in microseconds.

When performing the conversion seconds -> microseconds -> seconds we would get a different value that the original time.

In the W3C spec, when currentTime is > element.duration we are to seek backward to element.duration

So this is what was happening here, due to the rounding error we would start seeking within the stream. And as seeks in MP3 aren't 100% accurate it would cause the pause and distortion you were hearing.

Fixing the rounding issue, makes it so that we don't attempt to start seeking.

The reason the work around of removing Accept-Ranges: byte support on the server worked; was that it made the media resource unseekable. So even when the rounding issue occurred, it would attempt to seek, fail (or more accurately do nothing) -> no audio interruption.

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/002714e2ccba
https://hg.mozilla.org/integration/mozilla-inbound/rev/74caf0456e1e

Comment 52

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdf5bae3a63
https://hg.mozilla.org/integration/mozilla-inbound/rev/96d02c2defe1

Comment 53

[Carsten Book [:Tomcat]]

sorry backedout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17590338&repo=mozilla-inbound

Comment 54

[JW Wang [:jwwang] [:jw_wang]]

I believe this bug is also the cause of bug 1222271.

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4d0148fe03
https://hg.mozilla.org/integration/mozilla-inbound/rev/534855e49822
https://hg.mozilla.org/integration/mozilla-inbound/rev/459fc2c3b86e

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/002714e2ccba
https://hg.mozilla.org/mozilla-central/rev/74caf0456e1e

Comment 57

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/827100d647e3 (backout)
https://hg.mozilla.org/mozilla-central/rev/f1f5133ac4d1 (backout)
https://hg.mozilla.org/mozilla-central/rev/cc124f809d4a
https://hg.mozilla.org/mozilla-central/rev/81d47372c43a
https://hg.mozilla.org/mozilla-central/rev/0b2b0570777f

Comment 58

[Carsten Book [:Tomcat]]

(In reply to Pulsebot from comment #57)
> https://hg.mozilla.org/mozilla-central/rev/827100d647e3 (backout)
> https://hg.mozilla.org/mozilla-central/rev/f1f5133ac4d1 (backout)
> https://hg.mozilla.org/mozilla-central/rev/cc124f809d4a
> https://hg.mozilla.org/mozilla-central/rev/81d47372c43a
> https://hg.mozilla.org/mozilla-central/rev/0b2b0570777f

landed this directly on m-c with the fix since i screwed that up :( sorry

Comment 59

[Jean-Yves Avenard [:jya]]

(In reply to Loic from comment #1)
> Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=ce11a9ce760091dfd2a0acd0dd3724cace8bcd6a&tochange=a5eb
> 0b1fcf3954327d52436d0f99350479ea4768
> 
> Suspected bug: Bug 1172264

For the record: this is exactly what caused the regression and in particular https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1fd02713e2

Which replaced the direct use of microseconds and instead does microseconds->seconds->microseconds that introduced the rounding issue.

Comment 60

[Jean-Yves Avenard [:jya]]

Comment on attachment 8689807 [details] [diff] [review]
P1. Round second to closest microseconds.

Approval Request Comment
[Feature/regressing bug #]:1172264
[User impact if declined]: playback stuttering, unecessary seeks being performed
[Describe test coverage new/current, TreeHerder]: in central, we have lots of mochitests covering this area of the code. 
[Risks and why]: Low. The problem has been there for a long time but only exposed in bug 1172264. The use of floating points with integers and converting them back and forth always had potentially rounding issues. Some code may rely on the incorrect behaviour. However, all of those should have been caught by the mochitests (and it did hence the backout and the follow up fix). In any cases, any regressions would be extremely minor (variation of 1 microsecond)
[String/UUID change made/needed]: None

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc4d0148fe03
https://hg.mozilla.org/mozilla-central/rev/534855e49822
https://hg.mozilla.org/mozilla-central/rev/459fc2c3b86e

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8689807 [details] [diff] [review]
P1. Round second to closest microseconds.

Fix for recent regression in video seeking, ok to uplift to aurora and beta.

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

Are we landing the right patches here?

Comment 64

[Jean-Yves Avenard [:jya]]

The patch required are 
https://hg.mozilla.org/mozilla-central/rev/cc4d0148fe03
and https://hg.mozilla.org/mozilla-central/rev/459fc2c3b86e which is an add-on to fix some existing mochitests.

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a55efc59eae
https://hg.mozilla.org/releases/mozilla-aurora/rev/124aaf8a8940

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d2073dcbd5a
https://hg.mozilla.org/releases/mozilla-beta/rev/044c31c25c3c

Comment 67

[Liz Henry (:lizzard) (relman/hg->git project)]

One more question.... Was it correct to land https://hg.mozilla.org/mozilla-central/rev/534855e49822 at all in m-c? Should we be backing that out?

Comment 68

[Jean-Yves Avenard [:jya]]

Yes. It's part of the list of gtest ensuring the rounding is done properly.

Comment 69

[Chris Mills [:cmills]]

I've added a note to the release notes about this fix:

https://developer.mozilla.org/en-US/Firefox/Releases/45#AudioVideo

Is this sufficient?

Comment 70

[Kohei Yoshino]

Posted the site compatibility doc for the record: https://www.fxsitecompat.com/en-US/docs/2016/mp3-streaming-is-awkward-on-firefox-41-and-later/