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)
Firefox for Android Graveyard
Audio/Video
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.
Assignee | ||
Updated•7 years ago
|
Summary: [Fennec][HLS] Pause ExoPlayer when we cached enough data to avoid OOM. → [Fennec][HLS] Pause ExoPlayer when player is paused to avoid OOM.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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()()
Assignee | ||
Updated•7 years ago
|
Attachment #8888236 -
Flags: review?(kikuo)
Comment 3•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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 6•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9679bdd8b0a3 Pause Exoplayer when mediaelement is paused. r=kikuo
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9679bdd8b0a3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•