Closed Bug 1378852 Opened 7 years ago Closed 7 years ago

[Fennec][HLS] getBufferedPosition in GeckoHlsPlayer should always return a value >= 0.

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: kikuo, Assigned: kikuo)

References

Details

Attachments

(1 file)

ExoPlayer starts each playback with a base pts 60000000. Occasionally, when seek to a position near to playback start, ExoPlayer may adjust itself to a final reset position which is less than PTS 60000000. In that case, getBufferedPosition() will return a negative value, e.g. if ExoPlayer says the final reset position is 59500000 after a seek operation, we may get -500000 (59500000 - 60000000) for buffered position, and that will lead to an assertion |MOZ_ASSERT(aStart <= aEnd);| [1] while constructing a TimeInterval object [2]. According to the description for ExoPlayer API |getBufferedPosition| [3], UNKNOWN_TIME (a negative value, -1L) is returned when there's no estimated buffered position at that time, so we should return 0 in GeckoHlsPlayer::getBufferedPosition(). [1] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/media/Intervals.h#56 [2] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/media/hls/HLSDemuxer.cpp#647-648
Comment on attachment 8884071 [details] Bug 1378852 - Always return a value >= 0 for getBufferedPosition in GeckoHlsPlayer. https://reviewboard.mozilla.org/r/155010/#review160126 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:586 (Diff revision 1) > @Override > public long getBufferedPosition() { > assertTrue(mPlayer != null); > // Value returned by getBufferedPosition() is in milliseconds. > - long bufferedPos = mPlayer.getBufferedPosition() * 1000; > + long bufferedPos = Math.max(0, mPlayer.getBufferedPosition() * 1000); > if (DEBUG) { Log.d(LOGTAG, "getBufferedPosition : " + bufferedPos + "(Us)"); } You solved the intermittent crash issue! To fulfill Math.max(long, long), please modify the code to 0L, mPlayer.getBufferdPosition() * 1000L. And also amend the http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java#576 since the API has the same behavior as this one. Thanks.
Attachment #8884071 - Flags: review?(jacheng) → review+
(In reply to James Cheng[:JamesCheng] from comment #3) > Comment on attachment 8884071 [details] > Bug 1378852 - Always return a value >= 0 for getBufferedPosition in > GeckoHlsPlayer. > > https://reviewboard.mozilla.org/r/155010/#review160126 > > ::: > mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/ > GeckoHlsPlayer.java:586 > (Diff revision 1) > > @Override > > public long getBufferedPosition() { > > assertTrue(mPlayer != null); > > // Value returned by getBufferedPosition() is in milliseconds. > > - long bufferedPos = mPlayer.getBufferedPosition() * 1000; > > + long bufferedPos = Math.max(0, mPlayer.getBufferedPosition() * 1000); > > if (DEBUG) { Log.d(LOGTAG, "getBufferedPosition : " + bufferedPos + "(Us)"); } > > You solved the intermittent crash issue! > > To fulfill Math.max(long, long), please modify the code to 0L, > mPlayer.getBufferdPosition() * 1000L. > > And also amend the > > http://searchfox.org/mozilla-central/rev/ > f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/mobile/android/geckoview/src/main/ > java/org/mozilla/gecko/media/GeckoHlsPlayer.java#576 > > since the API has the same behavior as this one. > > Thanks. Thanks for review. Since the meaning of getDuration / getBufferedPosition are not exactly the same, I'll check the code flow, and handle it in Bug 1379056.
Pushed by kikuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/52d810352774 Always return a value >= 0 for getBufferedPosition in GeckoHlsPlayer. r=JamesCheng
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.

Attachment

General

Created:
Updated:
Size: