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

Status: RESOLVED FIXED

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

Description

[Kilik Kuo [:kikuo] (inactive)]

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 1

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1378852 - Always return a value >= 0 for getBufferedPosition in GeckoHlsPlayer.

-1L will be returned if there's no estimated buffered position for now,
it implies that we should treat any returned negative value in the same way.

Review commit: https://reviewboard.mozilla.org/r/155010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155010/

Comment 2

[James Cheng[:JamesCheng]]

[3] http://google.github.io/ExoPlayer/doc/reference-v1/com/google/android/exoplayer/ExoPlayer.html#getBufferedPosition--

Comment 3

[James Cheng[:JamesCheng]]

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.

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8884071 [details]
Bug 1378852 - Always return a value >= 0 for getBufferedPosition in GeckoHlsPlayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155010/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52d810352774
Always return a value >= 0 for getBufferedPosition in GeckoHlsPlayer. r=JamesCheng

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52d810352774