Closed Bug 1432195 Opened 6 years ago Closed 6 years ago

Video format or MIME type is not supported for very short MP3 file

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: roel, Assigned: bryce)

Details

Attachments

(2 files)

empty_0.013_goldwave_6.30.mp3
834 bytes, audio/mpeg
Details
Bug 1432195 - Accept Mp3 streams with only 2 frames if both are valid.
59 bytes, text/x-review-board-request
JanH
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

Opened very short (~52ms) MP3 file (128kb/s, 44.1kHz, joint stereo) directly in Firefox.
Said file is attached to this topic, or can be downloaded here: https://home.kompjoefriek.nl/mozilla/empty_0.013_goldwave_6.30.mp3
This file was generated by Goldwave v6.30 from a 13ms silent wave file, but this also happens when the wave file does contain sound.

All media players and browsers i have can play this MP3, except for FireFox.
Players: Goldwave v6.30, Audacity v2.2.0, Windows Media Player v12.0
Browsers: Chrome v63.0.3239.132, Edge v41.16299.15.0, Internet Explorer v11.125.16299.0

To narrow down what might cause this, i generated multiple files with multiple encoders. But this only happens when i use Goldwave to encode the MP3.
Source audio and other samples can be found here: https://home.kompjoefriek.nl/mozilla/
Please note that smaller wave files produce the exact same output in Goldwave, which leads me to believe that there is also something wrong with the output from Goldwave.


Actual results:

The media player is shown with the message "Video format or MIME type is not supported." and does not play the MP3 file.


Expected results:

The media player should play the MP3 file.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Thanks for the report! Bryce, do you mind tracing through and seeing why we're rejecting this file? Maybe we fail to sniff?
Assignee: nobody → bvandyk
Priority: -- → P2
Able to repro, digging deeper:

- The file is correctly identified as an Mp3 and given to the demuxer. So the issue is not that we're not identifying the file as an mp3.
- During demuxer init we try and read imetadata from the first frame.
- FindFirstFrame()[1] is failing to identify a valid candidate for first frame as it wants to find 3 successive frames, but since the file is so short it only finds 2 and falls over.
- This causes the setup of metadata to fail and we don't init the demuxer and end up with the failure seen here.
- See Bug 1363647 for the change that set the check to 3 frames.
- Looking at Chromium code[2], I wouldn't have thought they'd parse this either, but I'll dig deeper. There's no ID3 tag, so I'd expect them to fall back on their detection, which I would expect to fail as there are only 2 frames (based on stepping our code).

[0] https://searchfox.org/mozilla-central/source/dom/media/mp3/MP3Demuxer.cpp#125
[1] https://searchfox.org/mozilla-central/source/dom/media/mp3/MP3Demuxer.cpp#434
[2] https://cs.chromium.org/chromium/src/media/base/container_names.cc
I wonder if Chromium are only falling back to their above code if ffmpeg does not probe the format for them[0]. Failing my overlooking something, I don't see that their other, non-ffmpeg, code would detect an mp3. However, ffprobe will determine the provided file as an mp3.

2 ways that come to mind to address this:
- Quicker, easier: we further reduce the min number of samples. This could be gated such that we only allow for a 2 sample minimum in the case where the file only has two samples. We'll still look for 3 in other files. Something similar to what's described here[1].
- Longer time frame, harder, benefits outside of the scope of this bug: We wire libavformat in and have it help with detection. We already have some ff*/libav* components in tree. Provided there are no license issues we could use bring in format detection. Wiring would take time, but could be useful for sniffing formats in the general case.

:jya, thoughts?

[0] https://cs.chromium.org/chromium/src/media/filters/ffmpeg_glue.cc?l=133
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1363647#c11
Flags: needinfo?(jyavenard)
As :bryce stated, the MP3 demuxer expects a certain length in order to exclude false positive.

3 frames seems reasonable to me


:janh, what do you think? whenever someone comes with a shorter MP3 we're going to have a problem...
Flags: needinfo?(jyavenard) → needinfo?(jh+bugzilla)
(In reply to KompjoeFriek from comment #0)
> Opened very short (~52ms) MP3 file (128kb/s, 44.1kHz, joint stereo) directly
> in Firefox.
> Said file is attached to this topic, or can be downloaded here:
> https://home.kompjoefriek.nl/mozilla/empty_0.013_goldwave_6.30.mp3
> This file was generated by Goldwave v6.30 from a 13ms silent wave file, but
> this also happens when the wave file does contain sound.
> 
> All media players and browsers i have can play this MP3, except for FireFox.
> Players: Goldwave v6.30, Audacity v2.2.0, Windows Media Player v12.0
> Browsers: Chrome v63.0.3239.132, Edge v41.16299.15.0, Internet Explorer
> v11.125.16299.0

Also VLC. On the other hand it doesn't work in the last Winamp version, and libstagefright on Android requires four frames if I'm reading http://androidxref.com/6.0.1_r10/xref/frameworks/av/media/libstagefright/MP3Extractor.cpp#160 correctly - at least one of the media players on my phone (Rocket Player) actually complains when playing that file. Audacity opens it, but seems to swallow one of the frames and claims the file is only ~26 ms long.

So files that small are a bit finicky, anyway.

(In reply to KompjoeFriek from comment #0)
> To narrow down what might cause this, i generated multiple files with
> multiple encoders. But this only happens when i use Goldwave to encode the
> MP3.

Looking at your other sample file, FFMPEG also adds a XING header to the generated file. Because the XING header is technically an additional MPEG frame, this takes the total frame count up to three, i.e. enough to pass our consistency check.
Audacity (using LAME) does the same thing and adds a XING header to all files, even CBR files. Goldwave apparently doesn't, even though according to https://www.goldwave.com/faq.php#mpeg it uses LAME as well.

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> 3 frames seems reasonable to me

I'd agree, although if somebody wants to spend the time to allow two frames iff we hit EOS right after the second frame, that's fine with me.

> whenever someone comes with a shorter MP3 we're going to have a problem...

Given that neither FFMEPG nor LAME seem to produce a file with less than two actual content frames even when the input would actually fit into one frame, I think that case can be discounted (which I guess means that five minutes after me writing this somebody will arrive complaining that we can't play their one-frame MP3 file :-P).
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8954586 [details]
Bug 1432195 - Accept Mp3 streams with only 2 frames if both are valid.

https://reviewboard.mozilla.org/r/223682/#review230116

Thanks, looks fine, although there's one small tweak to at least handle ID3v2 tags as well.

::: dom/media/mp3/MP3Demuxer.cpp:481
(Diff revision 1)
> -  if (numSuccFrames >= MIN_SUCCESSIVE_FRAMES) {
> -    MP3LOG("FindFirst() accepting candidate frame: "
> +      MP3LOG("FindFirst() accepting candidate frame: "
> -           "successiveFrames=%d", numSuccFrames);
> +             "successiveFrames=%d", numSuccFrames);
> -    mFrameLock = true;
> +      mFrameLock = true;
> -  } else {
> -    MP3LOG("FindFirst() no suitable first frame found");
> +      return candidateFrame;
> +    } else if (prevFrame.mStart == 0 &&

If you want to verify the start position of the first frame as well, rather use `prevFrame.mStart == mParser.ID3Header().TotalTagSize()` to allow files with an ID3v2 tag to pass our verification, too. 

Minimal files with an ID3v1 tag (at the end of the file) still won't work, but since right now we don't have any infrastructure for parsing/detecting those, I'm willing to punt on that for now.
Attachment #8954586 - Flags: review?(jh+bugzilla) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d90e4a0e881
Accept Mp3 streams with only 2 frames if both are valid. r=JanH
https://hg.mozilla.org/mozilla-central/rev/8d90e4a0e881
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
QA Whiteboard: [good first verify]
I have successfully reproduced the issue on Firefox Nightly 59.0a1 (2018-01-03) under Windows 10 (x64) using the attachment from Comment 0.

The issue is no longer reproducible on Firefox 60.0b9 and latest Nightly 61.0a1 (2018-04-02) under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: