1169142 - New MP3 demuxer can fail on live (unknown length) streams

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[isladelsol]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150513174244
Firefox for Android

Steps to reproduce:

Visit http://jplayer.org/latest/demo-08/


Actual results:

The following message is display: "Update Required.  To play the media you will need to either update your browser to a recent version or update your flash plugin"


Expected results:

No error message should be displayed and a Play button would be displayed.  When you click play, streaming html5 audio should be heard.   On the same Android 5.0.2 tablet, this player works fine using Chrome, Android Stock browser, Opera and Dolphin browsers.  Only Firefox for Android fails.  It also fails when trying to play streaming html5 audio on klove.com

Comment 1

[Eugen Sawin [:esawin]]

MP3 playback is broken on Firefox 38-40 running Android 5, but is going to be fixed in 41 (bug 1093815).

Comment 2

[isladelsol]

(In reply to Eugen Sawin [:esawin] from comment #1)
> MP3 playback is broken on Firefox 38-40 running Android 5, but is going to
> be fixed in 41 (bug 1093815).
> 
> *** This bug has been marked as a duplicate of bug 1093815 ***

Thanks Eugen.  Should it be working now with Nightly build 42.0a1 (2015-06-29)?

Comment 3

[Matthew Gregan [:kinetik]]

(In reply to isladelsol from comment #2)
> Thanks Eugen.  Should it be working now with Nightly build 42.0a1
> (2015-06-29)?

It should.  If it's not, please let us know!

Comment 4

[isladelsol]

(In reply to Matthew Gregan [:kinetik] from comment #3)
> (In reply to isladelsol from comment #2)
> > Thanks Eugen.  Should it be working now with Nightly build 42.0a1
> > (2015-06-29)?
> 
> It should.  If it's not, please let us know!

Unfortunately, streaming mp3 audio still doesn't work.  eg.  http://jplayer.org/latest/demo-08/ continues to fail with FF nighty 42.0a1.  On the very same Android 5.0.2 tablet (with no flash support), all other browsers play the streaming mp3 audio just fine: Chrome, stock browser, Opera and Dolphin.

Comment 5

[Matthew Gregan [:kinetik]]

Thanks, I tested it on a desktop build with the same MP3 code enabled and it worked, but it is indeed failing on my Nexus 5.  Reopening this bug, I'll dig into this further soon if Eugen doesn't get to it before me.

Comment 6

[Matthew Gregan [:kinetik]]

Actually, this *does* fail on desktop, but it depends on the timing of the network.

It looks like MP3TrackDemuxer::Init was failing because ReadAt was clamping the read size to 0 due to GetStreamLength returned -1... which you would expect to see on a stream without a Content-Length.  I'm not sure why we made this code try to clamp the size, as MediaResource::ReadAt appears to already have the behaviour that we need.

Pushed a simple potential fix to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f354ae4035

Comment 7

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f92f99cf75

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f92f99cf75

Testing with the APK from that build (here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a9f92f99cf75/try-android-api-11/), that seems to work.  There's a secondary issue that pausing and resuming doesn't work -- after resuming, you get a short burst of audio and then it pauses again.

Comment 9

[Eugen Sawin [:esawin]]

Thank you Matthew for investigating this!

Indeed, the read size clamping is causing the bug here. The reason to introduce it was to make the demuxer usable with MSE (addressing https://bugzilla.mozilla.org/show_bug.cgi?id=1166779#c6). However, the code handled this case (unknown content length) incorrectly.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1169142-Fix-MP3-demuxer-initialization-for-resou.patch

With this patch we keep the read size clamping for the case where the content length is known, but don't clamp otherwise.

I've also refactored the initialization a bit by introducing a hard reset for the demuxer.

I haven't noticed any issues with pausing and resuming on klove.com and jplayer.org with this patch.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8629479 [details] [diff] [review]
0001-Bug-1169142-Fix-MP3-demuxer-initialization-for-resou.patch

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

Cool, thanks for the explanation/reminder.

Comment 12

[Eugen Sawin [:esawin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70398280f72

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd95975b642d

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/dd95975b642d

Comment 15

[isladelsol]

Confirm working with Nightly Android Firefox 42.0a1 (2015-07-08)...streaming MP3 audio returns after MANY releases!! Thanks all! Any way this could still make it into version 41?

Comment 16

[Eugen Sawin [:esawin]]

(In reply to isladelsol from comment #15)
> Confirm working with Nightly Android Firefox 42.0a1 (2015-07-08)...streaming
> MP3 audio returns after MANY releases!! Thanks all! Any way this could still
> make it into version 41?

I prefer to keep it on Nightly for a bit before requesting an uplift, but that's planned.

Comment 17

[Eugen Sawin [:esawin]]

Comment on attachment 8629479 [details] [diff] [review]
0001-Bug-1169142-Fix-MP3-demuxer-initialization-for-resou.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1093815
[User impact if declined]: broken MP3 (live) stream playback
[Describe test coverage new/current, TreeHerder]: on Nightly for weeks, new tests added in bug 1182100
[Risks and why]: low, simple change that affects only resources with unknown lengths
[String/UUID change made/needed]: none

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8629479 [details] [diff] [review]
0001-Bug-1169142-Fix-MP3-demuxer-initialization-for-resou.patch

The fix was verified on nightly, has an automated test and been in m-c for a few weeks. It should be safe to land in Aurora.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a1094c86f4ab

Comment 20

[isladelsol]

Confirmed working on 128kbps stream @ docradio.org with build 41.0a2 (2015-07-28).  Thanks Ritu & Ryan!

Comment 21

[Catalin Suciu [:csuciu]]

Verifying as fixed on latest Nightly and Aurora builds