Closed Bug 1169142 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

41 Branch
All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox41 --- verified
firefox42 --- verified

People

(Reporter: isladelsol, Assigned: esawin)

References

Details

Attachments

(1 file)

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
MP3 playback is broken on Firefox 38-40 running Android 5, but is going to be fixed in 41 (bug 1093815).
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Component: Untriaged → Video/Audio
OS: Unspecified → Android
Product: Firefox → Core
Hardware: Unspecified → All
Resolution: --- → DUPLICATE
Duplicate of bug: 1093815
(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)?
(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!
(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.
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
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
Assignee: nobody → kinetik
Status: REOPENED → ASSIGNED
Summary: Firefox Android 38 no longer supports html5 mp3 streaming audio → New MP3 demuxer can fail on live (unknown length) streams
(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/mgregan@mozilla.com-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.
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.
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.
Attachment #8629479 - Flags: feedback?(kinetik)
Version: 38 Branch → 41 Branch
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.
Attachment #8629479 - Flags: feedback?(kinetik) → review+
Assignee: kinetik → esawin
https://hg.mozilla.org/mozilla-central/rev/dd95975b642d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: in-testsuite?
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?
(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.
Flags: in-testsuite? → in-testsuite+
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
Attachment #8629479 - Flags: approval-mozilla-aurora?
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.
Attachment #8629479 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Confirmed working on 128kbps stream @ docradio.org with build 41.0a2 (2015-07-28).  Thanks Ritu & Ryan!
Verifying as fixed on latest Nightly and Aurora builds
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.