MP3Demuxer downloads the whole file before doing anything

RESOLVED FIXED in Firefox 43

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: esawin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
All
Android
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Playing the MP3 found there:
http://podiobooks.com/title/a-coffee-crusade/

which is:
http://hwcdn.libsyn.com/p/f/4/3/f434e89b9a46f426/PB-CoffeeCrusade-01.mp3?c_id=536624&expiration=1439435909&hwt=d0c27a2d14831297bf6fa729755ddf55

After demuxing the first frame, I see the demuxer downloading and parsing the entire content ; only to return an error that it has reached EOS.

It should only parsed content as it arrives upon a call to NotifyDataArrived

It shouldn't attempt to read the entire file.
Blocks: 1172419
No longer blocks: 1194079
(Assignee)

Comment 1

2 years ago
This is not a recent regression, playback on this site has been broken for a long time (before the introduction of the new MP3Demuxer).

The most likely cause for this is that the stream is malformed or not recognizable as an MP3 stream for other reasons, which would explain the behaviour.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1188873

Comment 2

2 years ago
Playback on podiobooks.com seems to have been broken for a long time, however that MP3 file itself looks like a recent regression to me as well.
On FF40, it's showing up with a length of 39:01 and playback works, on Nightly on the other hand it's appearing with a length of 22:41:13 (!) and playback doesn't work.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 3

2 years ago
plays fine with the AppleMP3Reader
(Reporter)

Comment 4

2 years ago
Couldn't we abort early after failing to decode some samples after a few MB?

downloading the entire file (here over 30MB) seems like excessive
(Reporter)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1188873
(Reporter)

Comment 6

2 years ago
actually, I think the issue is more generic than this particular mp3 not playing.

it's not about that it's not playing, but that an non recognised file is downloaded in it's entirety.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 7

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> actually, I think the issue is more generic than this particular mp3 not
> playing.
> 
> it's not about that it's not playing, but that an non recognised file is
> downloaded in it's entirety.

That's a good point. We should abort parsing after X bytes parsed without a successful frame detection.
(Assignee)

Comment 8

2 years ago
Created attachment 8648097 [details] [diff] [review]
0001-Bug-1194085-Abort-frame-parsing-if-no-valid-audio-st.patch

With this patch, we abort frame parsing after reading 12kB (minus detected ID3 header size) without detecting the first frame header.
Attachment #8648097 - Flags: review?(jyavenard)
(Assignee)

Comment 9

2 years ago
(In reply to JanH from comment #2)
> Playback on podiobooks.com seems to have been broken for a long time,
> however that MP3 file itself looks like a recent regression to me as well.
> On FF40, it's showing up with a length of 39:01 and playback works, on
> Nightly on the other hand it's appearing with a length of 22:41:13 (!) and
> playback doesn't work.

This is worth investigating, too. On which platform are you testing this? On FF40 for Android, I only get the option to download the MP3 file, it won't play it in the browser.
(Assignee)

Updated

2 years ago
OS: Unspecified → Android
Hardware: Unspecified → All

Comment 10

2 years ago
Yes, I was testing on Android and for me, the file opened directly within the browser. Maybe downloading it to your device and opening it via a file:// URL works for you?
(Assignee)

Comment 11

2 years ago
(In reply to JanH from comment #10)
> Yes, I was testing on Android and for me, the file opened directly within
> the browser. Maybe downloading it to your device and opening it via a
> file:// URL works for you?

We're not registered to handle that intent, so opening the file with FF won't be an option.
I can only get the URL to open directly in the browser in FF41+, where it fails to play.

Comment 12

2 years ago
That's weird. Even on Firefox 40, I can simply type e.g. /storage/extSdCard/Music into the URL bar, which shows the directory listing for that folder on my phone. I can then pick any MP3 file at random, and it's starts playing using Firefox's internal player.
Which Android version are you using?
(Reporter)

Comment 13

2 years ago
(In reply to Eugen Sawin [:esawin] from comment #8)
> Created attachment 8648097 [details] [diff] [review]
> 0001-Bug-1194085-Abort-frame-parsing-if-no-valid-audio-st.patch
> 
> With this patch, we abort frame parsing after reading 12kB (minus detected
> ID3 header size) without detecting the first frame header.

what's the logic behind the choice of 12kB?
(Assignee)

Comment 14

2 years ago
> what's the logic behind the choice of 12kB?

There is no particular logic behind it, I've just increased the heuristic length from 1x buffer size from the old frame parser to 3x to give the parser some more leeway in the case of a failed detection of an (long but realistic) ID3 header.

Pushing the value much higher up won't give us more reliability, since a single buffer read (4kB) should contain at least 2 complete audio frames.
(Reporter)

Comment 15

2 years ago
Comment on attachment 8648097 [details] [diff] [review]
0001-Bug-1194085-Abort-frame-parsing-if-no-valid-audio-st.patch

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

this still seems super short to me..

but if you think that's sufficient.

How will this handle a case where we have a valid audio and suddenly some corruption in the stream and then valid again?
Attachment #8648097 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 16

2 years ago
> this still seems super short to me..
> 
> but if you think that's sufficient.
> 
> How will this handle a case where we have a valid audio and suddenly some
> corruption in the stream and then valid again?

We can safely increase the value, if you prefer. It's difficult to say what's best without knowing the probability and characteristics of stream corruptions.

The mechanics from the patch would not abort reading once we've identified it as a MP3 stream, so mid-stream corruptions would be simply skipped over. Only corruptions before the first frame could break the parsing if we pass the 12kB limit of unidentifiable stream data.

As for regular non-corrupted streams which don't form valid MPEG audio, the minimum required range that needs to be parsed to identify it is ID3-header-size[10 bytes] + ID3-data-size[<=256MB] + frame-header-size[4 bytes]. With the maximum ID3 data size being practically unbound, we can not use it as a base for our value.
(Assignee)

Comment 17

2 years ago
The patch does not count the ID3-data-size from comment 16 towards the 12kB. Which means that the ID3 header parsing is critical, since a wrongly identified ID3 tag size may abort the parsing pre-maturely.

Due to the unbound max ID3 tag size, the only way to prevent aborting in such a case would be to accept a valid ID3 header as validation for the stream, without requiring parsing a valid MPEG frame. But that would mean we would parse streams (until EOS) which might not contain any MPEG frames.
(Assignee)

Updated

2 years ago
Assignee: nobody → esawin
(Assignee)

Comment 18

2 years ago
Created attachment 8649433 [details] [diff] [review]
0001-Bug-1194085-Abort-frame-parsing-if-no-valid-audio-st.patch

Increased MAX_SKIPPED_BYTES from 3x to 10x buffer size (40kB) addressing review comment.
Attachment #8648097 - Attachment is obsolete: true
Attachment #8649433 - Flags: review+

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f195230a7c5d
https://hg.mozilla.org/mozilla-central/rev/f195230a7c5d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

2 years ago
See Also: → bug 1197985
You need to log in before you can comment on or make changes to this bug.