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

Increase playlist stuck target duration coefficient and catch BehindLiveWindowExceptions properly #7661

Merged
merged 5 commits into from
Feb 1, 2022

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Jan 15, 2022

What is it?

  • Bugfix (user facing)

Description of the changes in your PR

Set a bigger delay before throwing a PlaylistStuckException (15 * TARGET_DURATION_OF_A_SEGMENT) than ExoPlayer's delay (3.5 * TARGET_DURATION_OF_A_SEGMENT), to improve streaming experience on livestreams.

Indeed, I noticed that sometimes, YouTube HLS manifests of livestreams are being stuck sometimes randomly, for a few seconds, and sometimes very frequently (I don't know if that's a YouTube throttling or not), where livestreams on the website/biggest official clients are playing mostly fine. For low or ultralow latency livestreams, this stucking results right now in a exception which is thrown very quickly (sometimes after 1 or 2 second), which makes the livestream unplayable.

This PR also fixes a bug when ExoPlayer encounter a BehindLiveWindowException: the play queue manager was reloaded to try to reload the stream and then an error was thrown. It makes no sense to do this, so this PR makes the player seek instead to the default position, preparing again ExoPlayer (like recommended by ExoPlayer's website) and don't throw an error.

Fixes the following issue(s)

No issue I found was opened for these bugs.

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@AudricV AudricV added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Jan 15, 2022
@AudricV AudricV requested a review from Redirion January 15, 2022 18:14
@Redirion
Copy link
Member

generally this looks good to me but we need to keep track of those "copy" classes somewhere. In the event that ExoPlayer gets updated we would also have to check if have to update / replace / remove these copy classes as well.

@AudricV
Copy link
Member Author

AudricV commented Jan 16, 2022

Yes, unfortunately, that's the only disadvantage of this, but I don't think we have a choice if we want to improve ExoPlayer's default behavior.

@litetex
Copy link
Member

litetex commented Jan 16, 2022

I also currently don't like the PR as it introduces a bunch of copy-paste code... jada jada see above comments

Wouldn't it be better if we implement the change into ExoPlayer directly / open a PR by them?
The PR could either

  • make all methods / fields protected so that they can be overridden (by us) or
  • add an option for the MAXIMUM_PLAYLIST_STUCK_DURATION_MS or something similar

We could also fork Exoplayer and build a custom version with the fix. However that might be to complex (e.g. the building part).

Fun Fact: // TODO: Allow customization of stuck playlists handling. is already inside the ExoPlayer code 😆

@opusforlife2
Copy link
Collaborator

Wouldn't it be better if we implement the change into ExoPlayer directly / open a PR by them?

Let's please do this. It will be ironic and hilarious.

@Redirion
Copy link
Member

what I just noticed: it should already be customizable. See this commit:
google/ExoPlayer@28434cf

@AudricV
Copy link
Member Author

AudricV commented Jan 17, 2022

Yes, but we need to pass an HlsMediaSource.Factory. The only problem is that the factory uses the constructor with the default playlist stuck target duration coefficient value. So what I should do?

@litetex
Copy link
Member

litetex commented Jan 17, 2022

Yes, but we need to pass an HlsMediaSource.Factory. The only problem is that the factory uses the constructor with the default playlist stuck target duration coefficient value. So what I should do?

Uhm I don't see a problem here:

--- a/app/src/main/java/org/schabi/newpipe/player/helper/PlayerDataSource.java
+++ b/app/src/main/java/org/schabi/newpipe/player/helper/PlayerDataSource.java
@@ -8,12 +8,17 @@ import com.google.android.exoplayer2.source.ProgressiveMediaSource;
 import com.google.android.exoplayer2.source.SingleSampleMediaSource;
 import com.google.android.exoplayer2.source.dash.DashMediaSource;
 import com.google.android.exoplayer2.source.dash.DefaultDashChunkSource;
+import com.google.android.exoplayer2.source.hls.HlsDataSourceFactory;
 import com.google.android.exoplayer2.source.hls.HlsMediaSource;
+import com.google.android.exoplayer2.source.hls.playlist.DefaultHlsPlaylistTracker;
+import com.google.android.exoplayer2.source.hls.playlist.HlsPlaylistParserFactory;
+import com.google.android.exoplayer2.source.hls.playlist.HlsPlaylistTracker;
 import com.google.android.exoplayer2.source.smoothstreaming.DefaultSsChunkSource;
 import com.google.android.exoplayer2.source.smoothstreaming.SsMediaSource;
 import com.google.android.exoplayer2.upstream.DataSource;
 import com.google.android.exoplayer2.upstream.DefaultDataSourceFactory;
 import com.google.android.exoplayer2.upstream.DefaultLoadErrorHandlingPolicy;
+import com.google.android.exoplayer2.upstream.LoadErrorHandlingPolicy;
 import com.google.android.exoplayer2.upstream.TransferListener;

 public class PlayerDataSource {
@@ -45,7 +50,16 @@ public class PlayerDataSource {
         return new HlsMediaSource.Factory(cachelessDataSourceFactory)
                 .setAllowChunklessPreparation(true)
                 .setLoadErrorHandlingPolicy(
-                        new DefaultLoadErrorHandlingPolicy(MANIFEST_MINIMUM_RETRY));
+                        new DefaultLoadErrorHandlingPolicy(MANIFEST_MINIMUM_RETRY))
+                .setPlaylistTrackerFactory(
+                        (dataSourceFactory, loadErrorHandlingPolicy, playlistParserFactory) ->
+                            new DefaultHlsPlaylistTracker(
+                                    dataSourceFactory,
+                                    loadErrorHandlingPolicy,
+                                    playlistParserFactory,
+                                    /* playlistStuckTargetDurationCoefficient */ 10.0
+                            )
+                        );
     }

     public DashMediaSource.Factory getLiveDashMediaSourceFactory() {

@AudricV AudricV force-pushed the livestreams-improvements branch from 134578b to 81e6bea Compare January 20, 2022 16:20
@AudricV AudricV requested a review from litetex January 20, 2022 16:21
@AudricV AudricV changed the title Use a custom HlsPlaylistTracker to increase playlists stuck delay and catch BehindLiveWindowExceptions properly Increase playlist stuck target duration coefficient and catch BehindLiveWindowExceptions properly Jan 20, 2022
@AudricV AudricV force-pushed the livestreams-improvements branch from 81e6bea to 7c2af00 Compare January 20, 2022 17:16
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost perfect

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Jan 22, 2022

Importing database into this APK is causing it to close at the splash screen without crashing. This isn't happening on other PR APKs.

Edit: It's happening on all the 3 APKs I downloaded just now: seamless switching, livestream quality selector, and this. Maybe a bug slipped into the dev branch?

@AudricV
Copy link
Member Author

AudricV commented Jan 22, 2022

See #7674. I just need to rebase all my PRs.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have anything to add, code looks good, the comment by litetex just needs to be solved :-)

Instead of trying to reload the play queue manager and then throwing an error, BehindLiveWindowExceptions now make the app seek to the default playback position, like recommended by ExoPlayer.
The buffering state is shown in this case.

Error handling of other exceptions is not changed.
…o allow more stucking on HLS livestreams

ExoPlayer's default behavior is to use a multiplication of target segment by a coefficient (3,5).
This coefficient (and this behavior) cannot be customized without using a custom HlsPlaylistTracker right now.

New behavior is to wait 15 seconds before throwing a PlaylistStuckException.
This should improve a lot HLS live streaming on (very) low-latency livestreams with buffering issues, especially on YouTube with their HLS manifests.
…Tracker

They seem to be wrong, by looking at the class work and at the return of CustomHlsPlaylistTracker's methods.
@AudricV AudricV force-pushed the livestreams-improvements branch from 7c2af00 to 52cc4a0 Compare January 30, 2022 19:42
@AudricV AudricV requested review from litetex and removed request for Redirion January 30, 2022 19:43
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

Watched some livestreams for a few minutes and everything worked fine 😄

@Redirion Redirion merged commit e865c43 into TeamNewPipe:dev Feb 1, 2022
@AudricV AudricV deleted the livestreams-improvements branch February 2, 2022 20:13
This was referenced Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants