1216460 - Firefox on Android buffers HTML5 video content until OOM

Status: RESOLVED FIXED

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

Description

[Clemens Eisserer]

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.

Comment 1

[Ralph Giles (:rillian)]

We haven't been able to address this. Passing to the Android team.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen, can you see if it's reasonable to set a fixed buffer size?

Comment 3

[Eugen Sawin [:esawin]]

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.

Comment 4

[Eugen Sawin [:esawin]]

Clemens, can you please report which device you use, a video URL and the specific steps to reproduce this issue?

Comment 5

[Clemens Eisserer]

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

Comment 6

[Clemens Eisserer]

Attachment: about:memory

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

See data from #5 and #6

Comment 8

[Eugen Sawin [: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?

Comment 9

[Eugen Sawin [:esawin]]

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

Comment 10

[Jean-Yves Avenard [:jya]]

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)

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 14

[Clemens Eisserer]

> 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 ;)

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1216460-1.1-Refactor-data-types-fix-logs-and-pre.patch

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.

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1216460-2.1-Refactor-SourceBuffer-frame-eviction.patch

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.

Comment 17

[Jean-Yves Avenard [:jya]]

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

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1216460-2.2-Refactor-SourceBuffer-frame-eviction.patch

Addressed comments.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6495286a5790
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffa65c64e14

Comment 21

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1216560-3.1-Make-eviction-thresholds-const.-r-jy.patch

I've missed to make the members const, doing so in a separate push.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6495286a5790
https://hg.mozilla.org/mozilla-central/rev/9ffa65c64e14

Comment 23

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1216460-3.2-Make-eviction-thresholds-const.-r-jy.patch

Fixed bug number in patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/602b90a3250a

https://hg.mozilla.org/mozilla-central/rev/602b90a3250a