Closed Bug 1382151 Opened 7 years ago Closed 7 years ago

[Fennec][HLS] Pause ExoPlayer when player is paused to avoid OOM.

Categories

(Firefox for Android Graveyard :: Audio/Video, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(1 file)

Fork from Bug 1379866 comment 12,

Switching tab will not invoke suspend/resume of MediaResource...

I think we should auto-pause Exoplayer when we queued enough data.
Summary: [Fennec][HLS] Pause ExoPlayer when we cached enough data to avoid OOM. → [Fennec][HLS] Pause ExoPlayer when player is paused to avoid OOM.
The patch intends to pause the Exoplayer when the player is paused to avoid OOM crash.

When pausing, mediaelement allows seeking, so we should resume Exoplayer to get the enough chunks and pause again when it stop fetching data.



The patch also handles 


** CID 157198:  Concurrent data access violations  (GUARDED_BY_VIOLATION)
/mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java: 716 in org.mozilla.gecko.media.GeckoHlsPlayer.getNextKeyFrameTime()()
Attachment #8888236 - Flags: review?(kikuo)
Comment on attachment 8888236 [details]
Bug 1382151 - Pause Exoplayer when mediaelement is paused.

https://reviewboard.mozilla.org/r/159188/#review165056

::: dom/media/hls/HLSDecoder.cpp:98
(Diff revision 1)
>  }
>  
> +nsresult
> +HLSDecoder::Play()
> +{
> +  HLS_DEBUG("HLSDecoder", "Player called Play");

Assertion for where it should be called.

::: dom/media/hls/HLSDecoder.cpp:106
(Diff revision 1)
> +}
> +
> +void
> +HLSDecoder::Pause()
> +{
> +  HLS_DEBUG("HLSDecoder", "Player called Pause");

ditto

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:314
(Diff revision 1)
>  
>      // Called on GeckoHlsPlayerThread from ExoPlayer
>      @Override
>      public synchronized void onLoadingChanged(boolean isLoading) {
>          if (DEBUG) { Log.d(LOGTAG, "loading [" + isLoading + "]"); }
>          if (!isLoading) {

Shouldn't we call setPlayerWhenReady(false) while |mSuspend| is true and the incoming |isLoading| is true ?

That should stop the player ASAP, wouldn't that be more precise ?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:685
(Diff revision 1)
>      }
>  
>      // Called on HLSDemuxer's TaskQueue
>      @Override
>      public synchronized boolean seek(long positionUs) {
> +        if (mSuspended) {

Since mSuspend is guarded by '''synchronized''', is volatile still required ?
Comment on attachment 8888236 [details]
Bug 1382151 - Pause Exoplayer when mediaelement is paused.

https://reviewboard.mozilla.org/r/159188/#review165056

> Shouldn't we call setPlayerWhenReady(false) while |mSuspend| is true and the incoming |isLoading| is true ?
> 
> That should stop the player ASAP, wouldn't that be more precise ?

Thank you.

I seperate the flag into two. One is for Exoplayer and another is for MediaElement.
Comment on attachment 8888236 [details]
Bug 1382151 - Pause Exoplayer when mediaelement is paused.

https://reviewboard.mozilla.org/r/159188/#review166160

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:686
(Diff revision 2)
> +        if (mExoplayerSuspended) {
> +            resumeExoplayer();
> +        }

Please document the purpose why you must to resume ExoPlayer when it's suspened.
Attachment #8888236 - Flags: review?(kikuo) → review+
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9679bdd8b0a3
Pause Exoplayer when mediaelement is paused. r=kikuo
https://hg.mozilla.org/mozilla-central/rev/9679bdd8b0a3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.