1062514 - Some MP4 files cause firefox to slow to a crawl and lockup when fragmented MP4s are enabled

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Trevor Rowbotham [:rowbot]]

Attachment: fragmented_mp4_crash.html

In the attached test case, the script inserts a <div> into the page to visually show each TimeRange in a media's buffered object.  However, ever since the media.fragemented-mp4.enabled and media.fragmented-mp4.exposed prefs were enabled, the video in the test case continuously spawns new TimeRanges in the media's buffered object.  Disabling both of those prefs makes the problem go away.

When my browser locked up and became unresponsive, the media's buffered object contained a little over 30,000 TimeRanges.  This is really really bad.  A media's progress event is dispatched approximately every 250ms, which means that my browser was trying to restyle 30,000 elements every 250ms when it locked up.  The excessive number of TimeRanges in the buffered object also causes firefox to slow to a crawl long before it locks up giving a really poor user experience.

Chrome, Opera, and IE, on Windows 7, do not exhibit this issue. I have not tested this is on Linux or Mac.

Please let me know if I can provide anymore information.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The file looks like this:

mp4file dynamic-nl-dcon.m4v, track 1, samples 142085, timescale 90000
sampleId      1, size 21044 duration     5760 time        0 00:00:00.000 S
sampleId      2, size  4617 duration     2970 time     5760 00:00:00.064  
sampleId      3, size  1952 duration     5760 time     8730 00:00:00.097  
sampleId      4, size   249 duration     2970 time    14490 00:00:00.161  
sampleId      5, size   245 duration     2970 time    17460 00:00:00.194  
sampleId      6, size   114 duration     3060 time    20430 00:00:00.227  
sampleId      7, size   817 duration     2970 time    23490 00:00:00.261  
sampleId      8, size  7213 duration     3060 time    26460 00:00:00.294  
...

It is probably messing up due to different durations in combination with out of order frames.

Comment 2

[Trevor Rowbotham [:rowbot]]

So, what you are saying is that essentially the file is malformed/corrupt and this isn't really a firefox problem?  I knew that it was possible that the problem may have been the file and not firefox, but I wanted to report it anyways since there are probably loads of malformed/corrupted mp4s out there that firefox might have to one day deal with.  Plus, since both Chrome and IE play this file without an issue and support fragmented mp4s I felt it may have been a bug in firefox.

Comment 3

[Matthew Gregan [:kinetik]]

Even if the file is malformed, we shouldn't construct pathological buffered ranges.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The file isn't malformed. This is just a case that isn't handled properly.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

BTW - the file also isn't fragmented. We handle this case OK for fragmented. I should be able to get a patch up today or early next week.

Comment 6

[Trevor Rowbotham [:rowbot]]

I misunderstood what you were saying in comment 1, sorry about that.

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix range handling for regular MP4 with b-frames

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8485589 [details] [diff] [review]
Fix range handling for regular MP4 with b-frames

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +3763,5 @@
>        index.add(indice);
>    }
> +
> +  // Fix up composition durations so we don't end up with any unsightly gaps.
> +  if (index.size() != 0) {

if (!index.empty())

@@ +3765,5 @@
> +
> +  // Fix up composition durations so we don't end up with any unsightly gaps.
> +  if (index.size() != 0) {
> +      Indice* array = index.editArray();
> +      Vector<Indice*> composition_order;

composition_order.reserve(index.size()) will save some allocation overhead.

@@ +3766,5 @@
> +  // Fix up composition durations so we don't end up with any unsightly gaps.
> +  if (index.size() != 0) {
> +      Indice* array = index.editArray();
> +      Vector<Indice*> composition_order;
> +      for (uint32_t i = 0; i < index.size(); i++) {

Vector::size() returns size_t. Please use that type for i as well, like you would with nsTArray.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Ralph Giles (:rillian) from comment #8)
> > +  // Fix up composition durations so we don't end up with any unsightly gaps.
> > +  if (index.size() != 0) {
> 
> if (!index.empty())

I'm trying to follow the local style.

> > +      for (uint32_t i = 0; i < index.size(); i++) {
> 
> Vector::size() returns size_t. Please use that type for i as well, like you
> would with nsTArray.

Again following the local style. The size is equal to uint32_t SampleTable::CountSamples() from above.

Comment 10

[Ralph Giles (:rillian)]

Ok, nevermind then.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=cd8aae82a997

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8485589 [details] [diff] [review]
Fix range handling for regular MP4 with b-frames

Approval Request Comment

[Feature/regressing bug #]:
Issue for new feature of MP4 playback on OSX; Regression for MP4 playback on Windows Vista+

[User impact if declined]:
Problems playing some MP4 files. Likely a significant number of videos.

[Describe test coverage new/current, TBPL]:
MP4 playback is covered by mochitests but there are no specific tests for range handling. These are blocked on 976733.

[Risks and why]: 
The risks are that this could make range estimation worse rather than better, or that this fix is not sufficient for all cases. The change is both small and simple so I don't expect any issues.

[String/UUID change made/needed]:
None.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8485589 [details] [diff] [review]
Fix range handling for regular MP4 with b-frames

Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44d46e903286

Comment 17

[Sudhir]

Can I pick up this bug for testing, if so can someone update test-steps for the same?

PS: Assuming this bug is fixed and ready for QA