Closed Bug 1222866 Opened 4 years ago Closed 4 years ago

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

Categories

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

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- affected
firefox42 --- affected
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed

People

(Reporter: support, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(2 files, 1 obsolete file)

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
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ce11a9ce760091dfd2a0acd0dd3724cace8bcd6a&tochange=a5eb0b1fcf3954327d52436d0f99350479ea4768

Suspected bug: Bug 1172264
Blocks: 1172264
Status: UNCONFIRMED → NEW
Component: Untriaged → Audio/Video: Playback
Ever confirmed: true
Flags: needinfo?(bobbyholley)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Flags: needinfo?(bobbyholley) → needinfo?(jyavenard)
Could you re-test with 42?

I believe all mp3 related problems following bug 1172264 are finally gone.
Flags: needinfo?(jyavenard) → needinfo?(support)
Assignee: nobody → jyavenard
I tested in Nighty, it's reproducible, music cut-offs are audible.
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 :(
(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?
Flags: needinfo?(esawin)
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.
Flags: needinfo?(support)
It only shows glitch at the start with 42. After a minute or so I never hear more glitches.
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
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.
I'll check with 42 and get back to you with the results
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
Priority: -- → P1
Having the same issue on 41 and 42 . on 42 it takes around a minute until the glitches start
I can reproduce this here on a Nexus 6P, but that might also have to do with 6P weirdness.
I guess we need to figure out if this is a decoder underrun or output underrun?
I think we can rule out network problems given that I just had stuttering right up until the end of the track.
The stuttering is caused by repeated mid-stream seeking. Ignoring the seemingly randomly emitted seeking requests (in MediaDecoderStateMachine) fixes the issue.
Flags: needinfo?(esawin)
Where does the seeking comes from? Why would it be seeking?
(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.
Attachment #8686288 - Flags: feedback?(jyavenard)
when can we see this patch released ?
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 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)
Attachment #8686288 - Flags: feedback?(jwwang)
The duration is calculated by MP3FrameParser, right? It might give wrong duration when there is a gap in the data fed to the parser.
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.
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
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.
This just disabled seeking. 
At least that's a quick turn around.
(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.
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 ?
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 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.
Attachment #8686288 - Flags: feedback?(jwwang)
(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.
Can you show me the numbers so I have better understanding about what actually happened?
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.
(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
Flags: needinfo?(support)
stream with previous (before fix) nimble server
http://54.69.139.39/test/mp3/icecast.audio
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.
Attachment #8686288 - Flags: review+
Btw, I think we should avoid using floating points internally to avoid rounding errors.
(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
Flags: needinfo?(support)
(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 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
Attachment #8686288 - Flags: feedback?(jyavenard) → feedback-
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?
Flags: needinfo?(gsquelart)
(above I used floats 32 bits), representation is 0x40856c4c
(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?)
Flags: needinfo?(gsquelart)
(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.
Due to the internal double representation as per IEEE 754, during conversion the use of ints would have rounded down our value.
Attachment #8689807 - Flags: review?(gsquelart)
Attachment #8689807 - Flags: review?(gsquelart) → review+
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.
Attachment #8689808 - Flags: review?(gsquelart) → review+
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
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 ?
(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.
Summary: my icecast stream stops playing in FF 41. worked with 38, 39,40. what should I do? → Audio pause and distortion in MP3 stream due to rounding error since Firefox 41
sorry backedout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17590338&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
I believe this bug is also the cause of bug 1222271.
https://hg.mozilla.org/mozilla-central/rev/002714e2ccba
https://hg.mozilla.org/mozilla-central/rev/74caf0456e1e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
Flags: needinfo?(jyavenard)
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
Attachment #8689807 - Flags: approval-mozilla-beta?
Attachment #8689807 - Flags: approval-mozilla-aurora?
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.
Attachment #8689807 - Flags: approval-mozilla-beta?
Attachment #8689807 - Flags: approval-mozilla-beta+
Attachment #8689807 - Flags: approval-mozilla-aurora?
Attachment #8689807 - Flags: approval-mozilla-aurora+
Are we landing the right patches here?
Flags: needinfo?(jyavenard)
Attachment #8686288 - Attachment is obsolete: true
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.
Flags: needinfo?(jyavenard)
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?
Flags: needinfo?(jyavenard)
Yes. It's part of the list of gtest ensuring the rounding is done properly.
Flags: needinfo?(jyavenard)
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?
You need to log in before you can comment on or make changes to this bug.