Allow specifying a Looper when instantiating ExoPlayer #4278

kishido05 · 2018-05-22T04:54:01Z

Issue description

java.lang.IndexOutOfBoundsException on Timeline.getWindow

We're using SimpleExoPlayer to playback audio on our app. We didn't have any problems on our initial launch with the integration. However, when we upgrade the target SDK version to 27 (previously it was 25), then we started getting crashes. Also note that we were using version 2.7.3 of Exoplayer on our previous version.

For the usage, this is how we initialize the player:

BandwidthMeter bandwidthMeter = new DefaultBandwidthMeter();
TrackSelection.Factory videoTrackSelectionFactory = new AdaptiveTrackSelection.Factory(bandwidthMeter);
TrackSelector trackSelector = new DefaultTrackSelector(videoTrackSelectionFactory);
player = ExoPlayerFactory.newSimpleInstance(context, trackSelector);
player.setRepeatMode(Player.REPEAT_MODE_OFF);
player.addListener(this);

and then this is how we prepare for playback

DefaultDataSourceFactory dataSourceFactory = new DefaultDataSourceFactory(context, Util.getUserAgent(context, "App"), null);
        CacheDataSourceFactory cacheDataSourceFactory = new CacheDataSourceFactory(ProxyFactory.getProxy(context, 100 * 1024 * 1024), dataSourceFactory);
        ExtractorMediaSource.Factory factory = new ExtractorMediaSource.Factory(cacheDataSourceFactory);
        factory.setMinLoadableRetryCount(0);
        MediaSource audioSource = factory.createMediaSource(uri);
        player.prepare(audioSource);

We are not able to replicate the crash on our end so its a bit tricky. We only have the crash log (see below).

Version of ExoPlayer being used

2.8.0

Crash Log

Fatal Exception: java.lang.IndexOutOfBoundsException
       at com.google.android.exoplayer2.Timeline$1.getWindow(Unknown Source)
       at com.google.android.exoplayer2.Timeline.isEmpty(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.getCurrentWindowIndex(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.getCurrentPosition(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.getContentPosition(Unknown Source)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.onAudioDisabled(Unknown Source)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.onMetadata(Unknown Source)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.generatePlayingMediaPeriodEventTime(Unknown Source)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.onMetadata(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.addListener(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.addListener(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl.addListener(Unknown Source)
       at com.google.android.exoplayer2.ExoPlayerImpl$1.handleMessage(Unknown Source)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:154)
       at android.app.ActivityThread.main(ActivityThread.java:6351)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:896)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:786)

The text was updated successfully, but these errors were encountered:

tonihei · 2018-05-22T09:18:31Z

The stack trace of this crash looks really unusual. It doesn't really point a valid order of methods. Do you have any more details about what happened at the time this exception occurred?

One thing worth checking is that you only access the player from one single thread (the one you called newSimpleInstance on). But that's just a guess because we've seen problems in the past where this caused problems at unexpected locations. And it would also explain why it's difficult to reproduce.

kishido05 · 2018-05-23T03:16:31Z

@tonihei as of the moment we don't have any other info besides that as we can't replicate the crash on all of our test devices.

I'm going to take a look at the how we access the player. I'm quite sure we only do it on a single thread though.

And one more thing, I've said that the issue didn't occur when we were using SDK version 25 and Exoplayer version 2.7.3, but it seems I was mistaken, we just didn't notice it as it's not the top crash for the app during that time. (so I'm gonna rule out the possibility of problems with the SDK version)

Tolriq · 2018-05-28T15:55:24Z

@tonihei Just got the same crash after updating to 2.8.1 in prod (skipped 2.8.0) but previously no issue with 2.7.3, no target SDK change.

Same situation, can't reproduce very rare crash

Fatal Exception: java.lang.IndexOutOfBoundsException
       at com.google.android.exoplayer2.Timeline$1.getPeriod(Timeline.java:516)
       at com.google.android.exoplayer2.ExoPlayerImpl.com.google.android.exoplayer2.Timeline.getPeriod(ExoPlayerImpl.java:6749)
       at com.google.android.exoplayer2.ExoPlayerImpl.getCurrentPosition(ExoPlayerImpl.java:465)
       at com.google.android.exoplayer2.ExoPlayerImpl.getContentPosition(ExoPlayerImpl.java:520)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.generateEventTime(AnalyticsCollector.java:557)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.generateEventTime(AnalyticsCollector.java:584)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.generatePlayingMediaPeriodEventTime(AnalyticsCollector.java:594)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.onTimelineChanged$4fc0380a(AnalyticsCollector.java:408)
       at com.google.android.exoplayer2.ExoPlayerImpl.updatePlaybackInfo(ExoPlayerImpl.java:661)
       at com.google.android.exoplayer2.ExoPlayerImpl$1.com.google.android.exoplayer2.ExoPlayerImpl.handlePlaybackInfo(ExoPlayerImpl.java:1613)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:145)
       at android.app.ActivityThread.main(ActivityThread.java:6917)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1404)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1199)

tonihei · 2018-05-28T16:35:25Z

This stack trace is much better as it seems to point to a valid code position.

However, the call seems utterly impossible unless some form of parallel change is happening.
ExoPlayerImpl.getCurrentPosition() does something like this:

if (timeline.isEmpty()) {
 return localValue;
} else {
  timeline.getPeriod(...)
}

The stack trace indicates we end up in the else case and then call into an empty timeline, but that should have been caught by the if condition.
Can you please check again that your code isn't calling some method on another thread?

Side note: This problem is likely not new, but is only surfaced because we added an internal event listener (the AnalyticsCollector referenced in the stack trace) which also accesses the player state.

Tolriq · 2018-05-28T16:51:54Z

Well reading the stack trace it seems your listener is listening on main thread independently of what unique thread we may work :)

Furthermore is there anywhere in the documentation anything stating that exoplayer creation + all calls needs to happens on the same thread ?

tonihei · 2018-05-29T08:00:41Z

The documentation is here: http://google.github.io/ExoPlayer/doc/reference/com/google/android/exoplayer2/ExoPlayer.html
First bullet point under "Threading Model". I agree that it's not that obvious and we should probably mention it in the Getting Started guide too.

As you pointed out, the stack trace above seems to call the listeners on the main thread. That means the player was created on the main thread and all other calls to the player should happen on the main thread too.

Tolriq · 2018-05-29T08:37:47Z

I confirm that part of the DOC is well hidden and recommended quite different than mandatory :)
Specially since it worked very well before 2.8.0 for my use case. I do manage the access properly in my code and there was no risks of sync issues.

Creating the player on mainthread + using threadhandler is quite common, specially since support library mediasession should be created on main thread :(

If it's now mandatory and you can't allow such basic use case it should be in red here too http://google.github.io/ExoPlayer/guide.html

And changelog for 2.8.0 should be updated too as I now need to completely rewrite my code due to that little change and it would have been cool to know before pushing to prod :(

tonihei · 2018-05-29T10:06:09Z

As I said, this is not new and we always recommended to use one thread only. The problem only showed up because of our additional internal listener.

I see that it's potentially unwanted to use the main thread, but I think this can be solved by moving the ExoPlayerFactory call to your custom HandlerThread. Alternatively, we may provide an option for the ExoPlayerFactory to specify the Looper for all player operations after construction. Would that solve your issue?

Tolriq · 2018-05-29T10:36:37Z

It was recommended but with proper handling was working now it can't due to a change that is not clearly visible in the changelog. (Not saying that I'm right about how I did implement just that the change that render non recommended ways that could work to will not work at all for sure is not visible enough). I'm not native English speaker so don't take my words too hard :) And this could also explains why recommended for me is very far from forbidden.

HandlerThread was ok before migrating to exoplayer and as it did work with exoplayer and starting guide did not says it should not I keep it that way.

The main question is about the handling of ConcatenatingMediaSource that I use as dynamic....,

Does all the calls to the source also need to be done on same thread? As it's the consuming problematic part for main thread.

Providing the looper to get control over your AnalyticsCollector is a nice addition for this use case. I still have to repost from Player.EventListener to main thread but at least I do not have to change everything.

But this would propose an issue for those in my case.

tonihei · 2018-05-29T10:51:34Z

Yes, "recommended" may be misleading in this case. The guide says "strongly recommended" though ;)

For the rest of the question:

ConcatenatingMediaSource is indeed thread-safe and its operations can be used from any thread without issues. This was part of the design from the beginning unlike the main player interface which was designed to be used from one thread only. That means, you shouldn't need to change anything for the the calls which update or access the playlist.
I'm not quite sure what you mean by the "looper getting control over the AnalyticsCollector"? All player event listener calls are made on the thread the player was created on. This is true for the ones you add to the player with .addListener as well as the AnalyticsCollector which is added in the same way.
The Looper option I mentioned above would change "all player methods must be called on the thread the player was created on" to "all player methods must be called on the thread you provided to the factory". The same thread is used for listener notifications in both cases.

Tolriq · 2018-05-29T11:03:49Z

I meant that since I created the player on MainThread the listener was called on mainthread.

But I did not do anything in it that could be non thread safe. Just that I had the events on the thread I wanted and it was nice.

Since your code needs to access the player the same way as my non thread safe call the looper will migrate your AnalyticsCollector code to that thread and fix the issue, but it will also migrate my listener.
So I'll need to switch back to main thread for some of the actions on my listener. Not a very big deal but still changes needed that could not have been guessed from reading 2.8.0 changelog ;)

When I read http://google.github.io/ExoPlayer/guide.html I do not see anything talking about threading is there something else you call guide?

For the interface documentation that most won't read it's also said:
"Accessing an instance from multiple threads is discouraged as it may cause synchronization problems." This may lead to think that if you handle sync (that i did since except creation all access are on the same thread) you won't have issues ;)

Anyway now I know, but you may want to do something so that everybody know before migrating to 2.8.

tonihei · 2018-05-29T14:25:29Z

We'll reword some parts of the guide to make that more explicit. And we are adding support for setting a custom Looper to run all player commands and callbacks on. Marking this as an enhancement until this is done.

Tolriq · 2018-05-30T08:02:37Z

For the record I also got one

Fatal Exception: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
       at java.util.ArrayList.set(ArrayList.java:453)
       at com.google.android.exoplayer2.analytics.AnalyticsCollector.com.google.android.exoplayer2.analytics.AnalyticsCollector$MediaPeriodQueueTracker.onTimelineChanged(AnalyticsCollector.java:3705)
       at com.google.android.exoplayer2.ExoPlayerImpl.updatePlaybackInfo(ExoPlayerImpl.java:661)
       at com.google.android.exoplayer2.ExoPlayerImpl.stop(ExoPlayerImpl.java:350)
       at com.google.android.exoplayer2.SimpleExoPlayer.stop(SimpleExoPlayer.java:777)
       at org.leetzone.android.yatsewidget.renderers.local.MusicPlayer.stop(MusicPlayer.java:430)

And this one is called on the right thread and since I'm calling stop if the gathering is made after the stop there's no way that there's a timeline change after that.
So maybe there's something else with the collector.

tonihei · 2018-05-30T08:41:15Z

It's the same problem. The stack trace you show can only possibly happen when two concurrent modifications are happening.

Tolriq · 2018-05-30T09:01:51Z

Yes but all modifications are from the same thread this stop call occurs in my thread handler and your collector is also called in that handler thread and not in the main thread like the other stack trace, only thing that could make change on another thread would be something inside ExoPlayer.

And the app for sure does nothing after a stop in a short delay except clearing the playlist too.
So that would mean that the ConcatenatingMediaSource maybe thread safe but that even after the looper param I'll still need to do all playlist changes there?

I really want to be sure that the looper param will fully fix that of if I should already start to completely rewrite that part to have everything in a single thread.

tonihei · 2018-05-30T09:09:42Z

There are two sources of events:

Those which are called because of something like player.stop() (see above's stack trace). In this case the listeners are notified on the same thread.
Those which are called because something in the player changed for other reasons. These callbacks are made on the thread the player was created on (because we assume that the player is only used from this thread).

So for the example above, the callback for player.stop() happens in your handler thread. And clearing the playlist (or something else), sends a simultaneous message to the main thread, because the player was created on the main thread. That's why you're seeing the concurrent access.

When we add the support for the Looper param, the internal calls will happen on this specified Looper. So, if you can ensure that all player methods are called on the same Looper, no concurrent access should happen. And for the ConcatenatingMediaSource, it still means that you can call its methods on any thread because the resulting player callbacks will be directed to the specified Looper.

Tolriq · 2018-05-30T09:20:31Z

Ok thanks, then the looper should be enough and the refactor not that big.

Will it be a 2.8.x or a 2.9 thing? The crash are rare but just to know what to answer to users that encounter them.

tonihei · 2018-05-30T09:24:45Z

Difficult to say. Depends on how many other changes we have.

Not sure how difficult it is to do in your code, but you might move the player creation to your custom handler thread in the meantime as this is equivalent to specifying the Looper at the time of creation.

Tolriq · 2018-05-30T09:31:58Z

Well the initialisation part date from very long time with MediaPlayer and Android 2.x ;) Seeing that you recommend to use main thread for player access, if I have to update / rewrite that part I'll go full main thread road to be safer. (I currently only do music but should add video at some point so better be clean).

The exoplayer part of my app is currently only used by about 8% of the users and the race condition is rare so there's no urgency. Just need to know where I'm going as time planing as indie dev is the most complex part :(

kishido05 · 2018-05-31T06:15:21Z

Just a quick update on my part. There was indeed a line of code accessing the SimpleExoPlayer that is running on a separate thread. I have updated that part and made sure every other access is indeed in the main thread. So far, the issue hasn't resurfaced 2 days in to our new version.

This makes the requirement that all calls are made on one thread more explicit and also mentions this in the Getting Started guide. Issue:#4278 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198694579

Currently, the looper of the thread the player is created on is used (or the main looper if this thread doesn't have a looper). To allow more control over the threading, this change lets users specificy the looper which must be used to call player methods and which is used for event callbacks. Issue:#4278 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=201331564

tonihei self-assigned this May 22, 2018

tonihei added the need more info label May 22, 2018

tonihei added enhancement and removed need more info labels May 29, 2018

tonihei mentioned this issue May 31, 2018

EventListener isn't called in correct thread's looper #4332

Closed

ojw28 changed the title ~~Getting indexoutofbounds Timeline.getWindow~~ Allow specifying a Looper when instantiating ExoPlayer Jun 5, 2018

ojw28 closed this as completed Jun 20, 2018

tonihei mentioned this issue Jun 22, 2018

Crash reported pointing internal to player. #4408

Closed

tonihei mentioned this issue Jul 3, 2018

Invitation to comment: Player threading model #4463

Closed

Tolriq mentioned this issue Aug 28, 2018

ANR on player release #4352

Closed

tonihei mentioned this issue Sep 28, 2018

IndexOutOfBoundsException in SinglePeriodTimeline.getWindow #4877

Closed

google locked and limited conversation to collaborators Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying a Looper when instantiating ExoPlayer #4278

Allow specifying a Looper when instantiating ExoPlayer #4278

kishido05 commented May 22, 2018 •

edited

Loading

tonihei commented May 22, 2018

kishido05 commented May 23, 2018

Tolriq commented May 28, 2018 •

edited

Loading

tonihei commented May 28, 2018

Tolriq commented May 28, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 30, 2018

tonihei commented May 30, 2018 •

edited

Loading

Tolriq commented May 30, 2018

tonihei commented May 30, 2018 •

edited

Loading

Tolriq commented May 30, 2018

tonihei commented May 30, 2018

Tolriq commented May 30, 2018

kishido05 commented May 31, 2018

Allow specifying a Looper when instantiating ExoPlayer #4278

Allow specifying a Looper when instantiating ExoPlayer #4278

Comments

kishido05 commented May 22, 2018 • edited Loading

Issue description

Version of ExoPlayer being used

Crash Log

tonihei commented May 22, 2018

kishido05 commented May 23, 2018

Tolriq commented May 28, 2018 • edited Loading

tonihei commented May 28, 2018

Tolriq commented May 28, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 29, 2018

tonihei commented May 29, 2018

Tolriq commented May 30, 2018

tonihei commented May 30, 2018 • edited Loading

Tolriq commented May 30, 2018

tonihei commented May 30, 2018 • edited Loading

Tolriq commented May 30, 2018

tonihei commented May 30, 2018

Tolriq commented May 30, 2018

kishido05 commented May 31, 2018

kishido05 commented May 22, 2018 •

edited

Loading

Tolriq commented May 28, 2018 •

edited

Loading

tonihei commented May 30, 2018 •

edited

Loading

tonihei commented May 30, 2018 •

edited

Loading