1194085 - MP3Demuxer downloads the whole file before doing anything

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Eugen Sawin [:esawin]]

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.

Comment 2

[Jan Henning [:JanH]]

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.

Comment 3

[Jean-Yves Avenard [:jya]]

plays fine with the AppleMP3Reader

Comment 4

[Jean-Yves Avenard [:jya]]

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

Comment 6

[Jean-Yves Avenard [:jya]]

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.

Comment 7

[Eugen Sawin [:esawin]]

(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.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 9

[Eugen Sawin [:esawin]]

(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.

Comment 10

[Jan Henning [:JanH]]

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?

Comment 11

[Eugen Sawin [:esawin]]

(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

[Jan Henning [:JanH]]

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?

Comment 13

[Jean-Yves Avenard [:jya]]

(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?

Comment 14

[Eugen Sawin [:esawin]]

> 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.

Comment 15

[Jean-Yves Avenard [:jya]]

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?

Comment 16

[Eugen Sawin [:esawin]]

> 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.

Comment 17

[Eugen Sawin [:esawin]]

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.

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 19

[Pulsebot]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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