google / ExoPlayer

This project is deprecated and stale. The latest ExoPlayer code is available in https://github.com/androidx/media
https://developer.android.com/media/media3/exoplayer
Apache License 2.0
21.74k stars 6.03k forks source link

Allow playback to continue with sideloaded subtitles when included subtitles fail to load (e.g. 404) #7635

Closed giladna closed 4 years ago

giladna commented 4 years ago

[REQUIRED] Issue description

I have updated the demo up forked from 2.7.11 added support in side loaded text tracks select any dash (updated sample only for dash) play select text track German then after 2 sec seek forward

the playback stuck.

expected: if error in text track url or content video playback should not be broken

https://github.com/giladna/ExoPlayer/tree/invalid_vtt_track

Note: If I set CustomTextLoadErrorHandlingPolicy on the SingleSampleMediaSource.Factory .setLoadErrorHandlingPolicy(new CustomTextLoadErrorHandlingPolicy()) https://github.com/giladna/ExoPlayer/blob/invalid_vtt_track/demos/main/src/main/java/com/google/android/exoplayer2/demo/CustomTextLoadErrorHandlingPolicy.java

Exception will not be fired but still playback stuck

2020-07-16 16:15:09.086 5686-5716/? W/ActivityManager: wait for provider publish: waiters=1 callerApp=ProcessRecord{7d20611d0 5686:system/1000} cpr=ContentProviderRecord{7e15c75 u0 com.samsung.android.lool/com.samsung.android.sm.database.SmProvider}
2020-07-16 16:15:18.165 5686-5710/? E/libprocessgroup: Error encountered killing process cgroup uid 99496 pid 27582: No such file or directory
2020-07-16 16:15:21.456 27908-28520/com.google.android.exoplayer2.demo E/ExoPlayerImplInternal: Source error
      com.google.android.exoplayer2.upstream.HttpDataSource$InvalidResponseCodeException: Response code: 404
        at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.open(DefaultHttpDataSource.java:300)
        at com.google.android.exoplayer2.upstream.DefaultDataSource.open(DefaultDataSource.java:177)
        at com.google.android.exoplayer2.upstream.StatsDataSource.open(StatsDataSource.java:83)
        at com.google.android.exoplayer2.source.SingleSampleMediaPeriod$SourceLoadable.load(SingleSampleMediaPeriod.java:404)
        at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:415)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:764)

[REQUIRED] Reproduction steps

as above

[REQUIRED] Link to test content

as above

[REQUIRED] A full bug report captured from the device

not needed reproduced any time

[REQUIRED] Version of ExoPlayer being used

2.7.11

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

Galaxy 10e android 9

icbaker commented 4 years ago

I think this is a dupe of https://github.com/google/ExoPlayer/issues/6640 (which has a suggested workaround: https://github.com/google/ExoPlayer/issues/6640#issuecomment-553904951)

If we manage to fetch the subtitle content, but fail to decode it, then playback should continue: https://github.com/google/ExoPlayer/issues/6885

But currently if the failure happens earlier (i.e. during reading) then we treat it the same way as being unable to load the video or audio track and fail playback.

giladna commented 4 years ago

@icbaker

I have added this in the sample it did not work, you can try it on my branch

https://github.com/giladna/ExoPlayer/blob/a594c1d79264761f7518d04dbb530c431d498478/demos/main/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java#L593

giladna commented 4 years ago

@icbaker

I did not call trackSelector.setParameters( trackSelector .buildUponParameters() .setRendererDisabled(textRendererIndex, true)))

This will disable all tracks right? even though one single url is broken

icbaker commented 4 years ago

[i wrote most of this before https://github.com/google/ExoPlayer/issues/7635#issuecomment-659443589]

It looks like your example is missing two things from the suggested workaround:

  1. You don't call super.getRetryDelayMsFor(...) until it returns C.TIME_UNSET (i.e. you don't allow any 'normal' retries)

  2. You don't have this bit:

     handler.post(
        () -> trackSelector.setParameters(
            trackSelector
                .buildUponParameters()
                .setRendererDisabled(textRendererIndex, true)));

Presumably that's because you don't want to disable the text renderer because you want the sideloaded subtitles to be displayed instead? If you do add the renderer disabling bit, does playback continue (without any text)?

Marking this as an enhancement.

giladna commented 4 years ago

@icbaker

doing the following code fixed the problem

https://github.com/giladna/ExoPlayer/blob/c5a8c05609a4da974e5a1646e01e5dc137b37065/demos/main/src/main/java/com/google/android/exoplayer2/demo/CustomTextLoadErrorHandlingPolicy.java#L34

as you said text track is not functional anymore it could be be helpful if on onTracksChanged even we can receive only the valid tracks.

giladna commented 4 years ago

@icbaker

my issue actually is with external the media in my case does not have internal

icbaker commented 4 years ago

doing the following code fixed the problem

Cool - glad that worked, but I agree it doesn't really solve your usecase.

my issue actually is with external the media in my case does not have internal

I'm not sure I understand this. I might have worded the new title confusingly - my intention was to distinguish between 'sideloaded' subtitles (i.e. provided via SingleSampleMediaSource and MergingMediaSource) and subtitles 'included' in the original media (e.g. WebVTT subtitles fetched via a DASH manifest) - but not necessarily embedded inside the media container (e.g. inside an mp4 file)

It seems the problematic subtitles in your example are an 'included' .vtt file, while you want to play the 'sideloaded' .srt file?

What do you mean by 'external' and 'internal'?

giladna commented 4 years ago

@icbaker thanks for your attention and will to help!

my media does not have internal (included) text tracks.

I have created 2 external text tracks (sideloaded) and one of them has url which response with 404 the thing is that switching to this corrupted text track will not end in playback error only if I seek for some point in the future.

switching off text renderer is bad user experience we agree.

I think that the best solution in this case is to eliminate the invalid tracks. and expose only the valid text tracks we have using the onTracksChanged then we will not expose to the app the invalid text tracks at all. (I know we can face a case of corrupter vtt although it is there). again the question here whose responsibility to check this and of course I understand the impact on loading time.

in the other hand once it is exposed any behaviour that is taken will be wired to the user. or in the worst case playback will fail.

giving an advantage to the in band tracks can be good solution but not a complete one.

AquilesCanta commented 4 years ago

In case you haven't seen this, if you are using SingleSampleMediaSource, this looks like a job for treatLoadErrorsAsEndOfStream. The purpose of the configuration option is pretty straightforward.

AquilesCanta commented 4 years ago

Also, can we mark this issue as a duplicate of #3140?

icbaker commented 4 years ago

Thanks for clarifying @giladna, I had indeed misunderstood.

I also hadn't seen #3140 - and I agree with @AquilesCanta this seems like a duplicate of that.

I'm going to mark it as such - but we can re-open if that doesn't seem correct.

giladna commented 4 years ago

@AquilesCanta works!!!

return new SingleSampleMediaSource.Factory(new DefaultDataSourceFactory(this, ((DemoApplication) getApplication()).buildHttpDataSourceFactory()))
    //.setLoadErrorHandlingPolicy(new CustomTextLoadErrorHandlingPolicy(trackSelector))
    .setTreatLoadErrorsAsEndOfStream(true)
    .createMediaSource(Uri.parse(pkExternalSubtitle.getUrl()), subtitleFormat, C.TIME_UNSET);

while setLoadErrorHandlingPolicy will totally disables the text track this will keep the working ones functional.

Gilad

AquilesCanta commented 4 years ago

Thanks for confirming @giladna! Useful for future readers.