Closed Bug 1216460 Opened 9 years ago Closed 8 years ago

Firefox on Android buffers HTML5 video content until OOM

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

42 Branch
defect

Tracking

(firefox41 ?, firefox42 affected, firefox43 ?, firefox44 ?, firefox48 fixed, fennec+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox41 --- ?
firefox42 --- affected
firefox43 --- ?
firefox44 --- ?
firefox48 --- fixed
fennec + ---

People

(Reporter: linuxhippy, Assigned: esawin)

References

Details

Attachments

(4 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151015151621
Firefox for Android

Steps to reproduce:

When watching HTML5 video content on my low/mid-end Android phone (1GB RAM, Snapdragon 410), Firefox buffers HTML5 content in RAM until it is killed by the Android Runtime because of consuming too much memory.


Actual results:

1. Start firefox on a low-end phone (512/768mb RAM)
2. Open youtube.com in Desktop mode and start playback of a large video
3. After streaming a for a while, firefox is suddenly closed (during playback)

The actual use-case why I am using youtube this way is that Firefox is able to play youtube content even with the screen turned off or FF in background - a feature the official youtube player only supports with a paid music subscription.


Expected results:

Firefox should limit its buffering depending on the available memory and instead buffer large content to disk/SD card instead of RAM.
Component: Untriaged → Audio/Video
Product: Firefox → Core
Assignee: nobody → ajones
Component: Audio/Video → Audio/Video: Playback
We haven't been able to address this. Passing to the Android team.
Component: Audio/Video: Playback → Audio/Video
Product: Core → Firefox for Android
Assignee: ajones → nobody
Eugen, can you see if it's reasonable to set a fixed buffer size?
Assignee: nobody → esawin
tracking-fennec: --- → ?
Media cache size is set to 32MB on Android, total (system) heap usage while watching an HD HTML5 video varies between 70-140MB on my 1GB device (500MB heap space).

So far, I couldn't reproduce OOM crashing on Nightly.
tracking-fennec: ? → +
Clemens, can you please report which device you use, a video URL and the specific steps to reproduce this issue?
Flags: needinfo?(linuxhippy)
Device: Alcatel One Touch 5042D (Android 4.4.4, Snapdragon 410, 1GB RAM)
Firefox: 45
Video-URL: https://www.youtube.com/watch?v=00xDWfad_3Y

I've also attached a screenshot of about:memory a few minutes before FF was terminated, which lists 293mb "explicit allocations" of which 192mb are "media".
Flags: needinfo?(linuxhippy)
Attached image about:memory
See data from #5 and #6
Flags: needinfo?(esawin)
I can reproduce this.

The size constrain set by "media.cache_size" is only enforced for streams going through MediaCache, but ignored for MediaSource streams. SourceBuffer needs to respect the pref in its eviction strategy or provide other means to configure the maximum buffer size for different platforms.

Non-MSE streams behave as expected and don't show the same behavior.

jya, do we have plans to implement this or is the current implementation supposed to handle this already?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(esawin) → needinfo?(jyavenard)
@Clemens, thanks for the report. As a workaround, you can disable MSE via the pref "media.mediasource.enabled", which should fix the OOM issues for the time being.
the size allowed for MSE is 100MB per stream (1 audio and 1 video).

We do not have anything in place to vary this settings based on the platform.
https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBuffer.cpp#298

and https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#102

having said that, I would mark this bug as a dupe of bug 1191712

Going lower than 100MB for video could cause issue with youtube though. They assume a minimal buffer size.

Could someone use chrome on one of those low mem device and go to https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?timestamp=1458082922867

and select the test "29. VideoBufferSize"

in the log window you will see a text like:
"TestRunner:  checkGE passed: Estimated source buffer size is (94)."

We know that chrome uses 12MB for audio.
In my experience lowering the video threshold to less than 75MB caused issue playing 4K streams (it keeps resetting the buffer and reloading)
https://en.wikipedia.org/wiki/Nexus_S 1 GHz ARM CPU, PowerVR SGX 540 GPU, 512 MB RAM (split 128MB GPU / 384MB OS)

https://en.wikipedia.org/wiki/Samsung_Galaxy_S_III 1.4 GHz quad-core ARM CPU, ARM Mali-400 MP4, 1 GB RAM

I cannot get a buffer size using WebM/VP9 with Chrome using a Galaxy S3 (Android 4.3) or a Nexus S (Android 4.1). I can with a Nexus 5x (Android N) buffer size 142.

Using WebM/VP9 off both the Galaxy S3 (Android 4.3) buffer size 156 and the Nexus S (Android 4.1) buffer size 152.
(In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> https://en.wikipedia.org/wiki/Nexus_S 1 GHz ARM CPU, PowerVR SGX 540 GPU,
> 512 MB RAM (split 128MB GPU / 384MB OS)
> 
> https://en.wikipedia.org/wiki/Samsung_Galaxy_S_III 1.4 GHz quad-core ARM
> CPU, ARM Mali-400 MP4, 1 GB RAM
> 
> I cannot get a buffer size using WebM/VP9 with Chrome using a Galaxy S3
> (Android 4.3) or a Nexus S (Android 4.1). I can with a Nexus 5x (Android N)
> buffer size 142.

what do you mean you can't get a buffer size using WebM/VP9?

the instructions I provided above are for MSE with h264 (mp4)

the values you mentions, are following those steps?

interesting that it changes depending on the phone.,
Flags: needinfo?(jyavenard)
In the test runner I need to toggle the 'link' WebM/VP9: on to off. If the default WebM/VP9: on is set then the test never completes. Runner bar stays yellow, no buffer data is provided. Definition of never is ~5 min.
> In my experience lowering the video threshold to less than 75MB caused issue 
> playing 4K streams (it keeps resetting the buffer and reloading)

The platforms affected by this issue won't play 4k anyway ;)
This patch changes all SourceBuffer size types to int64_t (based on mSizeSourceBuffer) to prevent hazardous integer promotions in arithmetic operations and unnecessary type conversions.
Attachment #8732389 - Flags: review?(jyavenard)
Currently, both SourceBuffer and TrackBuffersManager manage the eviction threshold. This patch moves it into the latter, which reduces some of the complexity and allows for easier handling of dynamic thresholds (e.g., duration-based).

This patch also introduces dedicated audio and video thresholds to avoid buffering 100MB of audio data.

Ideally, we wouldn't need that by making the threshold duration-based, but given the current non-conformant MSE implementations on the web, a dynamic threshold implementation seems to be too risky.
Attachment #8732393 - Flags: review?(jyavenard)
Attachment #8732389 - Flags: review?(jyavenard) → review+
Comment on attachment 8732393 [details] [diff] [review]
0002-Bug-1216460-2.1-Refactor-SourceBuffer-frame-eviction.patch

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

LGTM, this should have been lodged into bug 1191712 really. or make 1191712 a dup of this one.

r=me with changes addressed.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +207,4 @@
>  
> +  const TimeIntervals buffered = Buffered();
> +
> +  if (!buffered.Length()) {

this seems redundant to me and actually wrong. if buffered range is empty, that means two things:

1- Either GetSize() will return 0
or 2- if it's an audio and video source buffer, then we may have just video at this stage and not audio. You may need to evict in this case.

In any case, toEvict will be negative and it will abort early just below.

So please remove this test

::: dom/media/mediasource/TrackBuffersManager.h
@@ +345,4 @@
>  
>    // Global size of this source buffer content.
>    Atomic<int64_t> mSizeSourceBuffer;
> +  Atomic<int64_t> mVideoEvictionThreshold;

those members are never modified and only set in the constructor. just make them const int64_t instead
Attachment #8732393 - Flags: review?(jyavenard) → review+
Addressed comments.
Attachment #8732393 - Attachment is obsolete: true
Attachment #8733425 - Flags: review+
I've missed to make the members const, doing so in a separate push.
Attachment #8733553 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6495286a5790
https://hg.mozilla.org/mozilla-central/rev/9ffa65c64e14
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1259916
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.