Closed Bug 1256590 Opened 8 years ago Closed 8 years ago

Firefox can't play some MP3 files (crashes on Android)

Categories

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

44 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + wontfix
firefox47 + verified
firefox48 + verified

People

(Reporter: herr.ernst, Assigned: JanH, Mentored)

References

Details

(Keywords: regression)

Attachments

(3 files, 4 obsolete files)

Firefox doesn't play some MP3 files (on Android, it even crashes). In around one out of ten cases, for example, it doesn't start playing the shoutcast stream <http://mp3stream1.apasf.apa.at:8000/;>. Attached are dumps from the shoutcast stream. The red-bordered files are not playable in Firefox (tested on FF 44 and 45 on Win7/Win10/OS X/Android). Sometimes you hear only garbage/artifacts, sometimes you hear nothing. (On some system combination it even works, but I haven't found the decisive factor.)
Also not that it works everywhere else I have tested (IE11/Win7, Chrome on x86 and Android, old Android Stock Browser, Safari), so it's unfortunately a subpar experience only on Firefox.
Component: General → Audio/Video
Product: Firefox → Core
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7933be0b031390aaa4e70676124ba28f9cb6278&tochange=ecbe6589d36e490fe8d3c9b5b2d88de394f6918c

Bug 1209410 - Use MP3Demuxer/MediaFormatReader by default when available
Blocks: 1209410
Status: UNCONFIRMED → NEW
Component: Audio/Video → Audio/Video: Playback
Ever confirmed: true
Flags: needinfo?(jyavenard)
Keywords: regression
OS: Unspecified → All
Version: 45 Branch → 44 Branch
Looks like those that are not playable in Firefox all have valid frame header patterns near the beginning and MP3Demuxer falsely recognized them as 1st frame headers. Since MP3Demuxer checks the frame consistency based on what 1st frame header contains [1], it will consider following valid frames false positives.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3Demuxer.cpp#430
I concur with that analysis. If we want to keep the consistency checks (which came about through bug 1219178), we have to try finding the actual frame header before committing to using it for all further consistency verifications.
Assignee: nobody → jh+bugzilla
I've worked something up over the weekend and it seems it's not too dissimilar to the solutions used by Chromium and libstagefright.

The number of consecutive frames required is up for debate - initially I set it at 5, MP3 Diags (http://mp3diags.sourceforge.net) requires 10 frames, whereas if I'm reading those sources correctly, libstagefright requires 4 consecutive frames and Chromium only 3. So for now, I've fixed it at 4.

The frameSeparation check could be made more lenient to cope with somewhat more broken MP3 streams, but currently I've set it to 0, i.e. successive frames need to follow each other directly without any gap between them. This also seems to be the approach taken by libstagefright and Chromium, and herrernst's files play fine with those settings.
Attachment #8732799 - Flags: feedback?(esawin)
This adds a modified version of small-shot.mp3 as a test case, which includes a false frame sync between the ID3v2 header and the actual first true frame sync.

Instead of adding an additional file, it would be possible to just directly replace the original copy of small-shot.mp3 with my modified version and use it to check for both things (recognising the true first frame sync when initialising the demuxer, and skipping over false positives when seeking within the file) in one test. Which approach would you prefer?
Attachment #8732801 - Flags: feedback?(esawin)
Blocks: 1219178
No longer blocks: 1209410
Comment on attachment 8732801 [details] [diff] [review]
Bug 1256590 - Part 2 - Add a test case.patch

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

I would prefer not to interfere with the web audio tests, it's safer to add a new file.
Attachment #8732801 - Flags: feedback?(esawin) → feedback+
Comment on attachment 8732799 [details] [diff] [review]
Bug 1256590 - Part 1 - Try excluding false positives.patch

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

This will help with a lot of MP3 streams, great work!
The general approach is correct, let's just simplify the implementation a bit.

::: dom/media/MP3Demuxer.cpp
@@ +376,5 @@
> +MediaByteRange
> +MP3TrackDemuxer::FindFirstFrame() {
> +  static const int MIN_SUCCESSIVE_FRAMES = 4;
> +
> +  bool foundFirstFrame = false;

This can be derived from numSuccFrames >= MIN_SUCCESSIVE_FRAMES.

@@ +388,5 @@
> +    MediaByteRange currentFrame = candidateFrame;
> +    MediaByteRange prevFrame;
> +    int64_t frameSeparation;
> +
> +    do {

The inner loop seems unnecessary.
Something like this should work:

MediaByteRange firstFrame = FindNextFrame();
int numSuccFrames = firstFrame.Length() > 0;

while (firstFrame.Length() && numSuccFrames < MIN_SUCCESSIVE_FRAMES) {
  MediaByteRange candidateFrame = FindNextFrame();
  numSuccFrames += candidateFrame.Length() > 0;

  if (!candidateFrame.Length()) {
    mParser.ResetFrameData();
    mOffset = firstFrame.mStart + 1;
    firstFrame = FindNextFrame();
    numSuccFrames = firstFrame.Length() > 0;
    continue;
  } 
}

::: dom/media/MP3Demuxer.h
@@ +308,5 @@
>  
>    // Returns the parsed VBR header info. Note: check for validity by type.
>    const VBRHeader& VBRInfo() const;
>  
> +  // Resets the parser.

Reset still resets frame data, why remove the comment?

@@ +397,5 @@
>  
>    // Seeks by scanning the stream up to the given time for more accurate results.
>    media::TimeUnit ScanUntil(const media::TimeUnit& aTime);
>  
> +  // Attempts to find the first valid frame, especially if a stream begins mid-frame.

Better: Finds the first valid frame and returns its byte range if found or a null-byte range otherwise.
Attachment #8732799 - Flags: feedback?(esawin) → feedback+
Mentor: esawin
> The inner loop seems unnecessary.
> Something like this should work:

Good point, I've eliminated the inner loop along the lines of your example.

> ::: dom/media/MP3Demuxer.h
> @@ +308,5 @@
> >  
> >    // Returns the parsed VBR header info. Note: check for validity by type.
> >    const VBRHeader& VBRInfo() const;
> >  
> > +  // Resets the parser.
> 
> Reset still resets frame data, why remove the comment?

As discussed, Reset() is called during seeking as well, so it is no longer allowed to reset the first frame data.

> @@ +397,5 @@
> >  
> >    // Seeks by scanning the stream up to the given time for more accurate results.
> >    media::TimeUnit ScanUntil(const media::TimeUnit& aTime);
> >  
> > +  // Attempts to find the first valid frame, especially if a stream begins mid-frame.
> 
> Better: Finds the first valid frame and returns its byte range if found or a
> null-byte range otherwise.

Also fixed up the neighbouring comment about FindNextFrame() to follow the same pattern.
Attachment #8732799 - Attachment is obsolete: true
Attachment #8733090 - Flags: review?(esawin)
I've only added a small comment explaining what the new file is intended to test.
Attachment #8732801 - Attachment is obsolete: true
Attachment #8733091 - Flags: review?(esawin)
Attachment #8733091 - Flags: review?(esawin) → review+
Tracked for Fx46+ given the MP3 playback issues and crashes on Fennec.
Comment on attachment 8733090 [details] [diff] [review]
Bug 1256590 - Part 1v2 - Try excluding false positives.patch

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

r=me with comments addressed.

Nice work!

::: dom/media/MP3Demuxer.cpp
@@ +390,5 @@
> +
> +    // FindNextFrame here will only return frames consistent with our candidate frame.
> +    currentFrame = FindNextFrame();
> +    numSuccFrames += currentFrame.Length() > 0;
> +    // Because a stream might contain multiple similar false positives,

I find this comment unclear, what do you mean by similar false positives?
We just try to enforce successive frame without gaps, or is there another issue?

@@ +392,5 @@
> +    currentFrame = FindNextFrame();
> +    numSuccFrames += currentFrame.Length() > 0;
> +    // Because a stream might contain multiple similar false positives,
> +    // we need to check the distance between consecutive frames as well.
> +    int64_t frameSeparation = currentFrame.mStart - prevFrame.mEnd;

Can be const.

@@ +407,5 @@
> +      numSuccFrames = candidateFrame.Length() > 0;
> +      currentFrame = candidateFrame;
> +      MP3LOGV("FindFirst() new candidate frame: mOffset=%" PRIu64 " Length()=%" PRIu64,
> +              candidateFrame.mStart, candidateFrame.Length());
> +      continue;

continue is redundant here, I had it in my example since I've assumed more code would follow the if block.

@@ +412,5 @@
> +    }
> +  }
> +
> +  if (numSuccFrames >= MIN_SUCCESSIVE_FRAMES) {
> +    MP3LOGV("FindFirst() accepting candidate frame: "

This may be a critical log for troubleshooting and is only displayed once per playback, make this MP3LOG.

@@ +415,5 @@
> +  if (numSuccFrames >= MIN_SUCCESSIVE_FRAMES) {
> +    MP3LOGV("FindFirst() accepting candidate frame: "
> +            "successiveFrames=%d", numSuccFrames);
> +  } else {
> +    MP3LOGV("FindFirst() no suitable first frame found");

See above.
Attachment #8733090 - Flags: review?(esawin) → review+
Comments addressed and carrying forward r+ by esawin.
 
> ::: dom/media/MP3Demuxer.cpp
> @@ +390,5 @@
> > +
> > +    // FindNextFrame here will only return frames consistent with our candidate frame.
> > +    currentFrame = FindNextFrame();
> > +    numSuccFrames += currentFrame.Length() > 0;
> > +    // Because a stream might contain multiple similar false positives,
> 
> I find this comment unclear, what do you mean by similar false positives?
> We just try to enforce successive frame without gaps, or is there another
> issue?

No, I just wanted to clarify that while FindNextFrame() will only return frames that pass the consistency checks in relation to the candidate frame, it is possible that a file might contain multiple similar false positives (in fact, those Austrian radio recordings do, for example). Therefore we need to check that not only can we find multiple putative frames which pass the VerifyFrameConsistency() check, but also the gap between them - which for a normal MP3 stream should be zero.

> @@ +392,5 @@
> > +    currentFrame = FindNextFrame();
> > +    numSuccFrames += currentFrame.Length() > 0;
> > +    // Because a stream might contain multiple similar false positives,
> > +    // we need to check the distance between consecutive frames as well.
> > +    int64_t frameSeparation = currentFrame.mStart - prevFrame.mEnd;
> 
> Can be const.

Indeed, noticed this myself today.

> @@ +407,5 @@
> > +      numSuccFrames = candidateFrame.Length() > 0;
> > +      currentFrame = candidateFrame;
> > +      MP3LOGV("FindFirst() new candidate frame: mOffset=%" PRIu64 " Length()=%" PRIu64,
> > +              candidateFrame.mStart, candidateFrame.Length());
> > +      continue;
> 
> continue is redundant here, I had it in my example since I've assumed more
> code would follow the if block.

In my initial rewrite I had indeed initially put a little bit of code there, but then I realised I could move it to the top of the loop, but then didn't think of the "continue;".

> This may be a critical log for troubleshooting and is only displayed once
> per playback, make this MP3LOG.

Done for both log statements.
Attachment #8733090 - Attachment is obsolete: true
Attachment #8733578 - Flags: review+
No change, just keeping in sync with parent commit from Part 1.
Carrying forward r+ by esawin.
Attachment #8733091 - Attachment is obsolete: true
Attachment #8733579 - Flags: review+
Component: Audio/Video: Playback → Audio/Video
Priority: -- → P2
Product: Core → Firefox for Android
Sigh, I should have rebased to avoid all those web-gl failures (presumably from bug 1255176), but anyway, the rest looks good...

@kentuckyfriedtakahe
Why the product change? Android might be the only one that actually crashes, but playback is affected on all platforms.
Keywords: checkin-needed
Component: Audio/Video → Audio/Video: Playback
Product: Firefox for Android → Core
(In reply to Jan Henning (:JanH) from comment #16)
> Why the product change? Android might be the only one that actually crashes,
> but playback is affected on all platforms.

I didn't look at the bug closely enough.
Priority: P2 → P1
https://hg.mozilla.org/mozilla-central/rev/bb8b386dc367
https://hg.mozilla.org/mozilla-central/rev/5c3d1ed446ef
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Looks really good, asking QE for verification. 
Jan, would this be good for uplift to Aurora? I think it is too late for beta at this point. We can also let it ride with 48.
Flags: qe-verify+
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8733578 [details] [diff] [review]
Bug 1256590 - Part 1v3 - Try excluding false positives.patch

Sure, should be good for uplift.

Approval Request Comment
[Feature/regressing bug #]: MP3 playback, bug 1219178
[User impact if declined]: Some MP3 files - especially live streams or recordings of live streams - will fail to play or, on Android, possibly even crash Firefox.
[Describe test coverage new/current, TreeHerder]: MP3Demuxer GTest (extended with new test file for this bug), general media tests and some manual testing
[Risks and why]: Low, this only affects how MP3Demuxer looks for the first frame during initialisation. Normal MP3 files will pass this without problems and Chrome and libstagefright use a very similar algorithm in their MP3 parsers.
[String/UUID change made/needed]: none
Flags: needinfo?(jh+bugzilla)
Attachment #8733578 - Flags: approval-mozilla-aurora?
herr.ernst, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(herr.ernst)
Comment on attachment 8733578 [details] [diff] [review]
Bug 1256590 - Part 1v3 - Try excluding false positives.patch

Fixes MP3 playback issues on Fennec, Aurora47+
Attachment #8733578 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
@ritu: I've tested the current nightly on OS X and Android and can confirm it works now.
Thank you for the work, really happy this was fixed so fast!
Flags: needinfo?(herr.ernst)
(In reply to herrernst from comment #25)
> @ritu: I've tested the current nightly on OS X and Android and can confirm
> it works now.
> Thank you for the work, really happy this was fixed so fast!

Awesome! Thank you for a prompt verification.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jyavenard)
Reproduced with 48.0a1 from 2016-03-19.
Verified fixed with 47.0b2 (Build ID: 20160502152141), across platforms [1].

[1] Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit
Flags: qe-verify+
Alexandra, did you test on Android because according to the bug author, it crashed Firefox for Android.
Flags: needinfo?(alexandra.lucinet)
(In reply to Loic from comment #28)
> Alexandra, did you test on Android because according to the bug author, it
> crashed Firefox for Android.

Nope, I only took care of Desktop side.  
herrernst, would you mind checking 47 on Android? Thanks in advance!
Flags: needinfo?(alexandra.lucinet) → needinfo?(herr.ernst)
Hi,
I've tried the current (47.0) Firefox Beta from the Play Store [1], and can confirm that it works now!
Thank you!
[1] https://play.google.com/store/apps/details?id=org.mozilla.firefox_beta
Flags: needinfo?(herr.ernst)
You need to log in before you can comment on or make changes to this bug.