ExoPlayer 2.17.1 update and MediaSource management rework #8020

karyogamy · 2022-03-12T19:50:50Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Hi team,
I took a shot at upgrading ExoPlayer to 2.17.0 and realized that quite a few API has been made obsolete. For example, getTag has been dropped and will need even more hacks to get it to work. Since a rework is needed anyways, I took the liberty to toss out the old custom MediaSource management in favour of a more vanilla ExoPlayer model, following this comment. This is an attempt to help with refactoring the player so we can shift to expose ExoPlayer interface for player management instead of managing our own PlayQueue for playlist modifications.

This means ExoPlayer will now be responsible for managing the internal playlist, and PlayQueue will no longer play a central role to playback management. It will become a player proxy for user issued commands, and acts as a view for the player playlist, since ExoPlayer doesn't provide an API for the entire playlist. As an added bonus, shuffle now works properly. You may also notice buffer will start immediately when seeking to an unprepared stream, as opposed to the old model, where playback resumes until StreamInfo is fetched before buffering starts. This is because StreamInfo are now loaded on-demand inside the MediaSource in a custom CompositeMediaSource that dispatches a call to resolve/build the actual MediaSource using the PlaybackResolvers.

Also, each MediaSource is now associated with a MediaItem that holds metadata. Tag objects are now contained inside this MediaItem instead. As a result, the old MediaSourceTag are now revamped into a generic interface that bridges between PlayQueueItem and StreamInfo since they both hold similar metadata. This can be expanded later so this interface object is exposed to external adapters and properly display if a stream is properly loaded or has errors (e.g. content error).

Going back to MediaSource management, a challenge arising from the new model is its lifecycle, where MediaSource in ExoPlayer aren't really allowed to self update. This means when a stream expires, we cannot update the Playlist MediaSource with a new one, nor should we since ExoPlayer should take care of it instead. This brings in DataSpec resolution, which is an interface ExoPlayer exposes that lets us examine an URI and replace it with another. So the solution here is to update our PlaybackResolver to encapsulate the Extractor Stream metadata (e.g. AudioStream, VideoStream, SubtitleStream, etc.) in an URI, and deliver that to the new DataSpecResolver to convert our custom URI to a real stream URI. This means we can rely on our own Extractor cache to handle expiration and resolve stream metadata into the real URL and ExoPlayer will handle the rest.

Concerns:

Because ExoPlayer now takes care of managing its own playlist, the MediaSourceManager (now PlayQueueReactor) no longer correct desynchronizations between the PlayQueue and ExoPlayer's own playlist. Since PlayQueue and PlayQueueReactor communicate with a message bus, non-atomic calls to PlayQueue can cause desyncs. For example, Enqueue next was broken because it works by appending to the queue and moving the last item to the current playing. When the append event message gets to the reactor, the item has already moved on the play queue. This is temporarily fixed by introducing a new event Insert to avoid mixing logic with auto-queuing on Append events. But a proper solution would be to refactor PlayQueue into an interface and create a new wrapper ActivePlayQueue on the base PlayQueue to handle all the player controlled play queue logic. The ultimate end-state for PlayQueue in this new model is to mainly act as a quick-access view for the adapters, and all actions to the queue are exposed via ExoPlayer interface instead.
Since ExoPlayer controls the shuffling now, we also lose some control (for the time being) in shuffled mode. For example, currently PlayQueue shuffle reorders the queue and is presented to the user (I actually like this better) instead of ExoPlayer's model of jumping to a random index. But this now also breaks the Enqueue next feature when used in shuffle mode. We can potentially create our own shuffle order as an individual component, though it means more code and kind of defeats the purpose of lifting the feature to ExoPlayer.
As mentioned before, StreamInfo preparation will now also be part of the buffering. This means, when selecting a stream that is not yet loadedm at all, buffering will feel a bit slower than before. Whereas before, the StreamInfo would load in the background while the stream continues to play before the buffering starts. I tried to mitigate this by pre-emptively loading the StreamInfo of the next MediaItem, but this issue will never fully go away since we can't load every item on the queue.

As you can see, there are quite a few more things that can be worked on to get this to a perfect state. But since the PR size is getting larger, I wanted to get this PR out now to get some opinion on if we want to make this shift in player modelling as part of the greater refactor work (or if this path isn't really worth proceeding). And if we want to make the shift, have the changes tested by more people and the code reviewed since it might break existing features.

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.

Fixes the following issue(s)

relate to Rewrite the player (and all its components) #7939

Due diligence

I read the contribution guidelines.

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/helper/CacheFactory.java

app/src/main/java/org/schabi/newpipe/player/helper/MediaSessionManager.java

app/src/main/java/org/schabi/newpipe/player/helper/PlayerDataSource.java

app/src/main/java/org/schabi/newpipe/player/mediasession/PlayQueueNavigator.java

litetex

Thank you for the PR!

A ExoPlayer update was long overdue in my opinion, good that you picked it up.

Very good PR in my opinion, I like the coding style and the usage of new Java 8 features such as the Streams API 👍

Didn't test it yet but I found some things in the review that should be processed:

Update: Also please have a look at the Sonarcloud warnings

litetex · 2022-03-14T16:27:26Z

app/src/main/java/org/schabi/newpipe/player/Player.java

@@ -798,7 +816,6 @@ public void handleIntent(@NonNull final Intent intent) {
        if (oldPlayerType != playerType && playQueue != null) {
            // If playerType changes from one to another we should reload the player
            // (to disable/enable video stream or to set quality)
-            setRecovery();


Why was this removed?

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/mediaitem/MediaItemTag.java

app/src/main/java/org/schabi/newpipe/player/resolver/ExtractorDataSourceResolver.java

karyogamy · 2022-03-15T01:02:32Z

Many thanks for the thorough review @litetex.

Given the serialization usage coincides with the DataSpec issue that might conflict with @TiA4f8R's delivery-methods-v2, I'd recommend not going forward with this PR as-is, and instead continue with the existing model where we have control MediaSource. The player changes though will largely remain the same, so the review is still effective. I've acked most of the change requests, and will make the change and add docs when time permits. You can check out the diff here.

Eventually when we decide to move to the ExoPlayer model, we will have the same issue using DataSpecs and need to choose to go with serialization or cache.

added: MediaItemTag for ManagedMediaSources. added: silent track for FailedMediaSource. added: keyframe fast forward at initial playback buffer. added: error notification on silently skipped streams.

fixed: checkStyle failures.

…ediaSource. fixed: onPlaybackSynchronize to rewind when not playing, which was incorrectly removed in previous commit. fixed: sonar and checkstyle issues.

litetex

Some minor things I noticed:

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/mediaitem/MediaItemTag.java

app/src/main/java/org/schabi/newpipe/player/mediasource/FailedMediaSource.java

app/src/main/java/org/schabi/newpipe/player/Player.java

…fications are created. updated: Throwable usage to Exceptions. updated: minor styles and documentations.

litetex

Nearly perfect, only minor things:

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/mediasource/FailedMediaSource.java

app/src/main/java/org/schabi/newpipe/player/playback/MediaSourceManager.java

…onds before allowing retry. updated: minor style fixes.

sonarqubecloud · 2022-03-27T17:35:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

0.0% Coverage
0.0% Duplication

litetex

LGTM

Found one problem while testing (however this is likely unrelated): When enqueuing a video it gets enqueued 2x.

Otherwise it looks like it' working fine 👍

karyogamy · 2022-04-02T18:18:45Z

Other issues that should be fixed in tandem:

Video progress resets during player switch: Video sometimes restarts from beginning when backgrounding #7916
Video Surface Released: Livestream -> Background: unrecoverable player error #6989

AudricV · 2022-04-03T12:55:13Z

It seems I found a regression with this PR: use the Play all button on the Creative Commons channel, open the video with Stacy Allison-Cassin, enable autogenerated subtitles, then switch to the next video. Wait for lyrics and you will notice that English subtitles are enabled, even if subtitles are marked as disabled by the player.

You need to switch to another subtitle language and then go back to the English one to make the player show that English subtitles are enabled.

karyogamy · 2022-04-03T15:55:09Z

Confirmed - can reproduce by queuing any 2 videos, first with a manual subtitle (e.g. english) and the second with only auto-generated subtitle of the previous language (e.g. english auto-generated) and the rest is the same as @TiA4f8R describes.

I suspect this is because of how ExoPlayer compare language names, we can probably fix this by updating the selected ExoPlayer subtitle track according to the selected caption dropdown on metadata update. Will take a look when next available.

app/src/main/java/org/schabi/newpipe/player/Player.java

Stypox · 2022-04-16T08:54:32Z

app/src/main/java/org/schabi/newpipe/player/Player.java

+            if (surface.isValid()) {
+                // initially set the surface manually otherwise
+                // onRenderedFirstFrame() will not be called
+                simpleExoPlayer.setVideoSurface(surface);


Could this be replaced by simpleExoPlayer.setVideoSurfaceHolder(binding.surfaceView.getHolder());, or would there be issues? After some testing in #8170 I couldn't notice any difference.

Absolutely, that is the better solution. setVideoSurfaceHolder is doing the same null checks here: https://github.com/google/ExoPlayer/blob/release-v2/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java#L1262

Stypox · 2022-04-16T08:58:23Z

app/src/main/java/org/schabi/newpipe/player/Player.java

-        onMetadataChanged(metadata);
+        maybeAutoQueueNextStream(streamInfo);
+        onMetadataChanged(streamInfo);
+        NotificationUtil.getInstance().createNotificationIfNeededAndUpdate(this, true);


Also, why does this now have forceRecreate = true? Also, it seems like you are calling createNotificationIfNeededAndUpdate twice when updateMetadataWith is called, since there is a createNotificationIfNeededAndUpdate(this, false); also inside onMetadataChanged. And as you can see that one has forceRecreate = false.

_{Note for myself: check NotificationPlayerUi.onMetadataChanged after getting a response here}

Oversight on my part. createNotificationIfNeededAndUpdate in onMetadataChanged should be removed.

app/src/main/java/org/schabi/newpipe/player/resolver/ExtractorDataSourceResolver.java

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/mediasource/PlaybackDispatch.java

app/src/main/java/org/schabi/newpipe/player/Player.java

karyogamy · 2022-04-16T19:44:29Z

app/src/main/java/org/schabi/newpipe/player/Player.java

-        onMetadataChanged(metadata);
+        maybeAutoQueueNextStream(streamInfo);
+        onMetadataChanged(streamInfo);
+        NotificationUtil.getInstance().createNotificationIfNeededAndUpdate(this, true);


Oversight on my part. createNotificationIfNeededAndUpdate in onMetadataChanged should be removed.

karyogamy · 2022-04-16T19:45:42Z

app/src/main/java/org/schabi/newpipe/player/Player.java

+            if (surface.isValid()) {
+                // initially set the surface manually otherwise
+                // onRenderedFirstFrame() will not be called
+                simpleExoPlayer.setVideoSurface(surface);


Absolutely, that is the better solution. setVideoSurfaceHolder is doing the same null checks here: https://github.com/google/ExoPlayer/blob/release-v2/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java#L1262

karyogamy · 2022-04-16T19:56:41Z

Sorry about that folks, got too used to Stash PR workflow and didn't realize on Github all my review replies stayed in Pending and never got out =\

DefaultControlDispatcher was removed in ExoPlayer 2.16.0, so the class extending it that handled play/pause was removed in TeamNewPipe#8020. The new solution is to use an instance of ForwardingPlayer. Call sessionConnector.setPlayer with an instance of ForwardingPlayer that overrides play() and pause() and calls the callback methods.

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/Player.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/Player.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/Player.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/Player.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/helper/CacheFactory.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/helper/MediaSessionManager.java Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/helper/PlayerDataSource.java Outdated Show resolved Hide resolved

karyogamy commented Mar 12, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/mediasession/PlayQueueNavigator.java Show resolved Hide resolved

AudricV added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels Mar 12, 2022

litetex requested changes Mar 14, 2022

View reviewed changes

karyogamy force-pushed the exo-update-v17 branch 2 times, most recently from 05dad02 to 9fb95e3 Compare March 17, 2022 00:55

litetex marked this pull request as draft March 17, 2022 16:56

karyogamy force-pushed the exo-update-v17 branch 4 times, most recently from ed0e8bb to 6554599 Compare March 19, 2022 19:45

karyogamy added 2 commits March 19, 2022 15:56

updated: ExoPlayer to 2.17.1.

4e459b3

added: MediaItemTag for ManagedMediaSources. added: silent track for FailedMediaSource. added: keyframe fast forward at initial playback buffer. added: error notification on silently skipped streams.

added: documentations to MediaItemTags and Player.

69646e5

fixed: checkStyle failures.

karyogamy force-pushed the exo-update-v17 branch from 6554599 to fd6cb44 Compare March 19, 2022 20:02

karyogamy requested a review from litetex March 19, 2022 20:22

karyogamy marked this pull request as ready for review March 19, 2022 20:23

karyogamy force-pushed the exo-update-v17 branch from fd6cb44 to 77ed300 Compare March 20, 2022 02:27

added: documentations on lifecycles for FailedMediaSource and LoadedM…

b81eb35

…ediaSource. fixed: onPlaybackSynchronize to rewind when not playing, which was incorrectly removed in previous commit. fixed: sonar and checkstyle issues.

karyogamy force-pushed the exo-update-v17 branch from 77ed300 to b81eb35 Compare March 20, 2022 02:40

litetex requested changes Mar 22, 2022

View reviewed changes

updated: onPlayerError to not catch unspecified source errors so noti…

d289dc8

…fications are created. updated: Throwable usage to Exceptions. updated: minor styles and documentations.

litetex requested changes Mar 27, 2022

View reviewed changes

updated: source loading error for FailedMediaSource to wait for 3 sec…

a00bc95

…onds before allowing retry. updated: minor style fixes.

litetex approved these changes Mar 30, 2022

View reviewed changes

litetex mentioned this pull request Mar 30, 2022

Fix SparseItemUtil so we don't enqueue twice #8127

Merged

2 tasks

litetex merged commit 860d28e into TeamNewPipe:dev Apr 2, 2022

tsiflimagas mentioned this pull request Apr 2, 2022

Video progress resets when switching player - Workaround #7668 doesn't work for playlists #7990

Closed

5 tasks

This was linked to issues Apr 2, 2022

Video progress resets when switching player - Workaround #7668 doesn't work for playlists #7990

Closed

Video sometimes restarts from beginning when backgrounding #7916

Closed

Livestream -> Background: unrecoverable player error #6989

Closed

triallax mentioned this pull request Apr 2, 2022

Livestream -> Background: unrecoverable player error #6989

Closed

4 tasks

AudricV mentioned this pull request Apr 4, 2022

Phone restarts when opening a video #5961

Closed

4 tasks

karyogamy mentioned this pull request Apr 5, 2022

Fix caption auto-selection not reflected in player GUI #8150

Merged

5 tasks

AudricV mentioned this pull request Apr 14, 2022

Fullscreen sometimes show a quarter of a screen [Android 12] #7262

Closed

4 tasks

Stypox reviewed Apr 16, 2022

View reviewed changes

This was referenced Apr 16, 2022

Refactor player and extract UI components #8170

Merged

Release 0.23.0 (please TEST!) #8230

Closed

karyogamy commented Apr 16, 2022

View reviewed changes

litetex mentioned this pull request Apr 16, 2022

[YT] Throttling of (60fps) streams #8238

Closed

5 tasks

ktprograms mentioned this pull request Apr 30, 2022

Fix hiding player controls when playing from media button #8315

Merged

5 tasks

Stypox mentioned this pull request Aug 28, 2022

Fix wrong thumbnail in notification on Android 13 #8899

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExoPlayer 2.17.1 update and MediaSource management rework #8020

ExoPlayer 2.17.1 update and MediaSource management rework #8020

karyogamy commented Mar 12, 2022 •

edited

Loading

litetex left a comment •

edited

Loading

litetex Mar 14, 2022

karyogamy commented Mar 15, 2022 •

edited

Loading

litetex left a comment

litetex left a comment

sonarqubecloud bot commented Mar 27, 2022

litetex left a comment

karyogamy commented Apr 2, 2022

AudricV commented Apr 3, 2022 •

edited

Loading

karyogamy commented Apr 3, 2022

Stypox Apr 16, 2022

karyogamy Apr 16, 2022

Stypox Apr 16, 2022 •

edited

Loading

karyogamy Apr 16, 2022

karyogamy Apr 16, 2022

karyogamy Apr 16, 2022

karyogamy commented Apr 16, 2022

ExoPlayer 2.17.1 update and MediaSource management rework #8020

ExoPlayer 2.17.1 update and MediaSource management rework #8020

Conversation

karyogamy commented Mar 12, 2022 • edited Loading

What is it?

Description of the changes in your PR

APK testing

Fixes the following issue(s)

Due diligence

litetex left a comment • edited Loading

Choose a reason for hiding this comment

litetex Mar 14, 2022

Choose a reason for hiding this comment

karyogamy commented Mar 15, 2022 • edited Loading

litetex left a comment

Choose a reason for hiding this comment

litetex left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 27, 2022

litetex left a comment

Choose a reason for hiding this comment

karyogamy commented Apr 2, 2022

AudricV commented Apr 3, 2022 • edited Loading

karyogamy commented Apr 3, 2022

Stypox Apr 16, 2022

Choose a reason for hiding this comment

karyogamy Apr 16, 2022

Choose a reason for hiding this comment

Stypox Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

karyogamy Apr 16, 2022

Choose a reason for hiding this comment

karyogamy Apr 16, 2022

Choose a reason for hiding this comment

karyogamy Apr 16, 2022

Choose a reason for hiding this comment

karyogamy commented Apr 16, 2022

karyogamy commented Mar 12, 2022 •

edited

Loading

litetex left a comment •

edited

Loading

karyogamy commented Mar 15, 2022 •

edited

Loading

AudricV commented Apr 3, 2022 •

edited

Loading

Stypox Apr 16, 2022 •

edited

Loading