Add timeline size check to avoid IndexOutOfBoundsException.

[jschamburger]

Closes #1865.

[ojw28]

I'm not convinced this is the correct fix for the ref'd issue. Whatever is calling getSegmentDurationUs is doing something wrong, since it's passing an index that's outside the bounds of the index. This is analogous to code trying to index into an array with an index >= its length. I also don't think it makes sense to fall through to the else block the problematic case.

The correct fix is to repair the calling code to not try and obtain the segment duration of a segment that's outside the bounds of the index. Would it be possible for you to take a look at applying that kind of a fix?

[jschamburger]

I get your point. But I don't think the analogy fits perfectly here: The calling code would also have to do the null check, wouldn't it? Since addressing an index in an array which might be null would also be a mistake on the caller's side. But for that case the method seems to have a valid else functionality.

The problem with the current implementation is that the code can throw an IndexOutOfBoundsException which cannot be caught by the App using the ExoPlayer and then leads to an app crash (which is of course the worst possible outcome). So this was merely supposed to be a quick fix to avoid the app crash, but - as you pointed out correctly - probably not the most reasonable thing to do.

The desired behavior would be to inform the calling app that there has been an error via EventListener.onPlayerError, right? Then the app could actually deal with the error instead of crashing because of the uncaught exception.

I will take a look!

[ojw28]

The desired behavior is that the internal code that makes the call that fails shouldn't be making the call. There wont be any error to report to the app once this is the case.

[jschamburger]

I believe that the stream cannot be played if the timeline is empty. So a playback error should be reported such that the app can react accordingly.
Of course, this call should not be made, but if the ExoPlayer fails to play the stream, an error should be reported.

[ojw28]

Not really. If it's a live stream correct behavior is to wait for a segment to be added to the timeline. If it's an on-demand stream correct behavior is to treat the content as having 0 duration (and so transition to the next piece of content or the ended state).

[jschamburger]

Fair enough! I thought the empty timeline would prohibit playback.

[ojw28]

It's actually not that trivial to fix this. DashSegmentIndex doesn't have a way to indicate that it's empty. I think getFirstSegmentNum probably ends up returning 0 and getLastSegmentNum probably ends up returning -1. But -1 is defined to be INDEX_UNBOUNDED, which isn't right. getLastSegmentNum probably needs changing to be exclusive rather than inclusive, so both returning 0 (or the same value) indicates an empty index.

[ojw28]

I can push a fix for the ref'd issue sometime this week; I think it touches quite a lot of files (about 7)? Closing this PR for now. Thanks!

[jschamburger]

Nice, thanks a lot!