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

Crash when reading a non-existing Adaptation Set from DASH manifest #2795

Closed
dbaelz opened this issue May 9, 2017 · 6 comments
Closed

Crash when reading a non-existing Adaptation Set from DASH manifest #2795

dbaelz opened this issue May 9, 2017 · 6 comments

Comments

@dbaelz
Copy link

dbaelz commented May 9, 2017

Issue description

When reading the AdaptationSet from the Dash manifest (MPD) the ExoPlayer didn’t check the provided indices of the Period and AdaptationSet elements. This could result in an IndexOutOfBoundsException when the referenced period or adaption set didn’t exist.

Reproduction steps

The reason for the faulty reference is probably an error in the manifest file, but the ExoPlayer should handle it more robustly and without a crash. Currently I can’t provide a manifest to reproduce the crash. However, the crash has occurred several times on different devices. I'm not sure if a simple check of the indices is sufficient. Maybe the correct index of the period and the adaption set must be checked and handled earlier.

Version of ExoPlayer being used

ExoPlayer r2.3.1

Device(s) and version(s) of Android being used

  • Samsung SM-T550 (Android 6.0.1)
  • Sony F8331 (Android 7.1.1)
  • NVIDIA SHIELD Tablet K1 (7.0)

Stacktrace

java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
	at java.util.ArrayList.get(ArrayList.java:411)
	at java.util.Collections$UnmodifiableList.get(Collections.java:1295)
	at com.google.android.exoplayer2.source.dash.DefaultDashChunkSource.getAdaptationSet(DefaultDashChunkSource.java:303)
	at com.google.android.exoplayer2.source.dash.DefaultDashChunkSource.updateManifest(DefaultDashChunkSource.java:145)
	at com.google.android.exoplayer2.source.dash.DashMediaPeriod.updateManifest(DashMediaPeriod.java:94)
	at com.google.android.exoplayer2.source.dash.DashMediaSource.processManifest(DashMediaSource.java:455)
	at com.google.android.exoplayer2.source.dash.DashMediaSource.onManifestLoadCompleted(DashMediaSource.java:365)
	at com.google.android.exoplayer2.source.dash.DashMediaSource$ManifestCallback.onLoadCompleted(DashMediaSource.java:730)
	at com.google.android.exoplayer2.source.dash.DashMediaSource$ManifestCallback.onLoadError(DashMediaSource.java:724)
	at com.google.android.exoplayer2.source.dash.DashMediaSource$ManifestCallback.onLoadCanceled(DashMediaSource.java:724)
	at com.google.android.exoplayer2.source.dash.DashMediaSource$ManifestCallback.onLoadCompleted(DashMediaSource.java:724)
	at com.google.android.exoplayer2.upstream.Loader$LoadTask.handleMessage(Loader.java:355)
	at android.os.Handler.dispatchMessage(Handler.java:102)
	at android.os.Looper.loop(Looper.java:154)
	at android.os.HandlerThread.run(HandlerThread.java:61)
	at com.google.android.exoplayer2.util.PriorityHandlerThread.run(PriorityHandlerThread.java:40)
@ojw28 ojw28 added the bug label May 9, 2017
@ba-a
Copy link

ba-a commented May 11, 2017

I can be a little bit more specific about the crash now. It happens due to an error in the playout of the stream which results in an incorrect dash manifest.
In the attachment I provided what one of these broken manifests looks like and for comparison a correct one.

Due to the playout error the manifest contains only the video AdaptionSet but none for the audio. It seems that the player currently does no check there but just assumes that at least two sets are included. We will fix the bug on the server side but I think the ExoPlayer should be able to handle the situation as well.

Broken Manifest mpd.txt
Correct Manifest mpd.txt

If you need more information just let us know.

@ojw28
Copy link
Contributor

ojw28 commented May 11, 2017

Yup, understood. It's pretty difficult (and a lot of extra code) to validate all parts of all media for correctness, so we have a pretty broad try { ... } catch ( ... ) { propagate as playback failure } to handle unexpected cases without killing the process. There are however a few specific callbacks that aren't covered by this logic, one of which is the ManifestCallback.onLoad* callbacks.

We'll use this issue to track getting them covered correctly, after which the IndexOutOfBoundsException will be propagated out the front of the player as a playback failure, but will not cause process death.

@ba-a
Copy link

ba-a commented May 12, 2017

Sounds good, thanks for your effort!

@ggabdol
Copy link

ggabdol commented Jun 12, 2017

Thank you very much for the effort on this. We're looking forward to seeing this issue fixed. Could you please tell me if there is an ETA for the fix?

@ojw28
Copy link
Contributor

ojw28 commented Jun 12, 2017

We do not have an ETA for this. It's preferable to fix this on the client side as a defensive measure. However if you're aware of your manifest server generating invalid manifests, you should really fix the problem on the server side at which point this becomes a non-issue. We have multiple large third party providers using ExoPlayer, who simply don't encounter this issue because their servers do the right thing.

@jpardogo
Copy link

👍

ojw28 added a commit that referenced this issue Nov 7, 2017
Issue: #2795

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174836960
@ojw28 ojw28 closed this as completed Nov 7, 2017
ojw28 added a commit that referenced this issue Nov 13, 2017
Issue: #2795

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174836960
@google google locked and limited conversation to collaborators Mar 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants