Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExoPlayer crash with IMAloader onTimelineChanged Assertions.checkArgument #5831

Closed
amjadmasri opened this issue May 5, 2019 · 9 comments
Closed
Assignees
Labels

Comments

@amjadmasri
Copy link

amjadmasri commented May 5, 2019

[REQUIRED] Issue description

Our app just recently released and we found in the Firebase Crashalytics an increasing number of crashes which I included below
we are using an HlsMediaSource for the content then adding the AdsMediaSourceand for the ads and
here is the code of how we are initializing the player:

ImaAdsLoader.Builder builder = new ImaAdsLoader.Builder(this);
 
imaAdsLoader=builder.setImaSdkSettings(ImaSdkFactory.getInstance().createImaSdkSettings()).buildForAdTag(Uri.parse(adVMAP));

DefaultDataSourceFactory defaultDataSourceFactory = new DefaultDataSourceFactory(this, Util.getUserAgent(this, getResources().getString(R.string.app_name)));

 MediaSource mediaSource = new 
 HlsMediaSource.Factory(defaultDataSourceFactory).createMediaSource(Uri.parse(episodeLink));

     AdsMediaSource adsMediaSource = new AdsMediaSource(mediaSource, defaultDataSourceFactory, imaAdsLoader, playerView.getOverlayFrameLayout());

     player.addListener(this);

     boolean haveStartPosition = startPosition != C.INDEX_UNSET;
     if (haveStartPosition) {
         player.seekTo(startPosition);
     }

     player.prepare(adsMediaSource,!haveStartPosition,false);
     player.setPlayWhenReady(true);

[REQUIRED] Reproduction steps

we couldn't reproduce the issue , we only see the crashes on the crashlytics console.

[REQUIRED] Link to test content

[REQUIRED] A full bug report captured from the device

Fatal Exception: java.lang.IllegalArgumentException
       at com.google.android.exoplayer2.util.Assertions.checkArgument(Assertions.java:39)
       at com.google.android.exoplayer2.ext.ima.ImaAdsLoader.onTimelineChanged(ImaAdsLoader.java:912)
       at com.google.android.exoplayer2.ExoPlayerImpl$PlaybackInfoUpdate.notifyListeners(ExoPlayerImpl.java:780)
       at com.google.android.exoplayer2.ExoPlayerImpl.updatePlaybackInfo(ExoPlayerImpl.java:717)
       at com.google.android.exoplayer2.ExoPlayerImpl.handlePlaybackInfo(ExoPlayerImpl.java:648)
       at com.google.android.exoplayer2.ExoPlayerImpl.handleEvent(ExoPlayerImpl.java:593)
       at com.google.android.exoplayer2.ExoPlayerImpl$1.handleMessage(ExoPlayerImpl.java:125)
       at android.os.Handler.dispatchMessage(Handler.java:105)
       at android.os.Looper.loop(Looper.java:156)
       at android.app.ActivityThread.main(ActivityThread.java:6577)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:986)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:876)

[REQUIRED] Version of ExoPlayer being used

2.9.4

[REQUIRED] Device(s) and version(s) of Android being used

Samsung galaxy S6
HTC Desire 820s
HUAWEI MediaPad T3 7

android 6, 7 and 8

@andrewlewis
Copy link
Collaborator

The failing assertion is Assertions.checkArgument(timeline.getPeriodCount() == 1);.

It looks like HlsMediaSource should always produce a single-period timeline so I wouldn't expect to see this error. However, I wonder if there's a case where the timeline can empty and this throws (there is a check for the timeline becoming empty due to resetting the player before this assertion, but maybe there is another case).

Could you confirm that you are always just playing an HlsMediaSource (and never a concatenation)?

@amjadmasri
Copy link
Author

Thank you for replying
yes we are always playing HlsMediaSource

@ojw28 ojw28 assigned tonihei and unassigned andrewlewis May 15, 2019
@ojw28
Copy link
Contributor

ojw28 commented May 15, 2019

@tonihei - Can you think of any way that this could happen (in 2.9.4, if not 2.10.0)?

My suspicion would be that there's some path somewhere that ends up calling onTimelineChanged with an empty timeline and a reason other than TIMELINE_CHANGE_REASON_RESET. I couldn't find one though. If we're unsure, perhaps we could have ImaAdsLoader.onTimelineChanged treat the empty timeline the same as TIMELINE_CHANGE_REASON_RESET?

@tonihei
Copy link
Collaborator

tonihei commented May 16, 2019

The problem seems to be caused by a Timeline update coming from the internal player with an empty Timeline, while ExoPlayerImpl has a non-empty Timeline.

The internal player sets the timeline in two places only: for updates coming from the MediaSource, or when resetting the internal state. The former can't possibly be an empty Timeline when coming from a HlsMediaSource. The latter is only called after calling prepare(..,resetState=true) or stop(reset=true).
However, in both cases, ExoPlayerImpl must already have an empty Timeline (sent with TIMELINE_CHANGE_REASON_RESET) and does not accept any other timeline until the respective command is handled. So I can't see any conceivably way in which this could happen.

@amjadmasri Do your logs contain any information on the circumstances of the error? For example app or player events happening before or at this time which would help to narrow down the cause?
Also, do you re-use your AdsMediaSource and/or your HlsMediaSource for different Player instances or in the same Player instance? Do you use Player.stop(..) at all? Do you use ExoPlayer from a different thread at any point?

@ojw28 Agree that we can make the check broader and ignore all empty timelines as a precaution.

@amjadmasri
Copy link
Author

unfortunately we couldn't reproduce the bug, so we only have what Firebase crashalytics gives us.
we don't reuse the media sources.
I will try and give you as much information about our use case and code as possible.

we use the following code to pause and resume playback

     private void pausePlayer() {
        if(player!=null)
        player.setPlayWhenReady(false);
     }                                  
    private void startPlayer() {
        if(player!=null)
        player.setPlayWhenReady(true);
    }

pause is called on the onPause lifecycle
and start is called on the onResume lifecycle
we also show an interstitial ad so these methods gets called when the ad plays.

also the code snippet in the original question with the following code at the beginning is called to init the player

trackSelectionFactory = new AdaptiveTrackSelection.Factory();
        trackSelectorParameters = new DefaultTrackSelector.ParametersBuilder().build();

        trackSelector = new DefaultTrackSelector(trackSelectionFactory);
        trackSelector.setParameters(trackSelectorParameters);


        player = ExoPlayerFactory.newSimpleInstance(this, trackSelector);
        playerView.setPlayer(player);

on the onStart Lifecycle
and on onPlayerError if the exception type is ExoPlaybackException.TYPE_SOURCE to retry playing the content.

we don't call the Player.stop()
and we don't have any other threads.

also we have been getting reports of another bug on the Firebase Console that is related to the Timeline being null
we have forward and rewind buttons that seek the player forward 10 seconds or backwards 10 seconds and sometimes when they are clicked this crash happens

Fatal Exception: java.lang.NullPointerException: Attempt to invoke virtual method 'com.google.android.exoplayer2.Timeline$Period com.google.android.exoplayer2.Timeline.getPeriod(int, com.google.android.exoplayer2.Timeline$Period)' on a null object reference
       at com.google.android.exoplayer2.ext.ima.ImaAdsLoader.onPositionDiscontinuity(ImaAdsLoader.java:976)
       at com.google.android.exoplayer2.ExoPlayerImpl.seekTo(ExoPlayerImpl.java:337)
       at com.google.android.exoplayer2.SimpleExoPlayer.seekTo(SimpleExoPlayer.java:935)
       at com.google.android.exoplayer2.BasePlayer.seekTo(BasePlayer.java:42)
      

ojw28 pushed a commit that referenced this issue May 16, 2019
We previously only checked whether the reason for the timeline change is
RESET which indicates an empty timeline. Change this to an explicit check
for empty timelines to also ignore empty media or intermittent timeline
changes to an empty timeline which are not marked as RESET.

Issue:#5831
PiperOrigin-RevId: 248499118
@tonihei
Copy link
Collaborator

tonihei commented May 16, 2019

Thanks for all the information, I'll have a look again to see if I can find a potential source for the problem.

The other stack trace is definitely a bug, because a seek could always happen before we get the first timeline update.

@tonihei
Copy link
Collaborator

tonihei commented May 16, 2019

I still can't see a concrete reason why the original problem may have occurred, but I believe the problem may be fixed in 2.10 because we added more robustness to our listener notification system (e.g. always ensure correct order, never send updates after a listener has been removed).

The commit above also works around the problem should it still occur for some reason, so I think we can close this issue (as soon as the second problem is also fixed)

ojw28 pushed a commit that referenced this issue May 17, 2019
We previously only checked whether the reason for the timeline change is
RESET which indicates an empty timeline. Change this to an explicit check
for empty timelines to also ignore empty media or intermittent timeline
changes to an empty timeline which are not marked as RESET.

Issue:#5831
PiperOrigin-RevId: 248499118
@amjadmasri
Copy link
Author

@tonihei
hello and sorry it took us this long to get back to you
after upgrading to the Exoplayer version 2.10.0 and three days of the new release we stopped getting the assertion error
but the second bug is still affecting some users

com.google.android.exoplayer2.ext.ima.ImaAdsLoader.onPositionDiscontinuity (ImaAdsLoader.java:1019)
com.google.android.exoplayer2.ExoPlayerImpl.lambda$seekTo$3 (ExoPlayerImpl.java:331)
com.google.android.exoplayer2.-$$Lambda$ExoPlayerImpl$Or0VmpLdRqfIa3jPOGIz08ZWLAg.invokeListener (Unknown Source)
com.google.android.exoplayer2.BasePlayer$ListenerHolder.invoke (BasePlayer.java:165)
com.google.android.exoplayer2.ExoPlayerImpl.invokeAll (ExoPlayerImpl.java:823)
com.google.android.exoplayer2.ExoPlayerImpl.lambda$notifyListeners$6 (ExoPlayerImpl.java:716)
com.google.android.exoplayer2.-$$Lambda$ExoPlayerImpl$DrcaME6RvvSdC72wmoYPUB4uP5w.run (Unknown Source:4)
com.google.android.exoplayer2.ExoPlayerImpl.notifyListeners (ExoPlayerImpl.java:726)
com.google.android.exoplayer2.ExoPlayerImpl.notifyListeners (ExoPlayerImpl.java:716)
com.google.android.exoplayer2.ExoPlayerImpl.seekTo (ExoPlayerImpl.java:331)
com.google.android.exoplayer2.SimpleExoPlayer.seekTo (SimpleExoPlayer.java:964)
com.google.android.exoplayer2.BasePlayer.seekTo (BasePlayer.java:42)

thank you!

@tonihei
Copy link
Collaborator

tonihei commented Jun 3, 2019

we stopped getting the assertion error

Glad to hear that!

but the second bug is still affecting some users

Yes, the potential fix hasn't been released yet because we were still testing various scenarios to see that it actually works.

tonihei added a commit that referenced this issue Aug 23, 2019
Any seek before the first timeline becomes available will result in a NPE.
Change it to handle that case gracefully.

Issue:#5831
PiperOrigin-RevId: 264603061
@tonihei tonihei closed this as completed Aug 23, 2019
ojw28 pushed a commit that referenced this issue Sep 2, 2019
Any seek before the first timeline becomes available will result in a NPE.
Change it to handle that case gracefully.

Issue:#5831
PiperOrigin-RevId: 264603061
@google google locked and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants