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

Update ExoPlayer to 2.14.2 #7005

Merged
merged 4 commits into from
Nov 12, 2021
Merged

Update ExoPlayer to 2.14.2 #7005

merged 4 commits into from
Nov 12, 2021

Conversation

Redirion
Copy link
Member

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

  • Updates ExoPlayer from 2.12.3 to 2.14.2. Changelog
  • Makes use of the new MediaParser feature that has been added in ExoPlayer 2.14.0 for Android11+
  • fixes most deprecations that came with the ExoPlayer update

Fixes the following issue(s)

Since ExoPlayer 2.13 codec capabilities are checked by ExoPlayer which lead to issue #6207. This is a topic affecting some LinageOS users with older SOCs. However I hope that when using LineageOS 18.1 the delegation through MediaParser API will fixthis issue. Feedback is appreciated regarding this!

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

@Redirion Redirion added the player Issues related to any player (main, popup and background) label Aug 28, 2021
@tsiflimagas
Copy link
Contributor

This seems to have introduced a couple of issues with audio. Switching to popup player can sometimes lead to no sound or distorted audio. Here's a log i captured log-newpipe.txt

@Redirion Redirion marked this pull request as draft September 4, 2021 09:38
@Redirion
Copy link
Member Author

Redirion commented Sep 4, 2021

thank you @tsiflimagas I was able to reproduce the issue. I will need more time to investigate this

@Redirion
Copy link
Member Author

turns out I was exactly once able to reproduce it. Now tried with logcat running and couldn't reproduce it any more.

@litetex
Copy link
Member

litetex commented Nov 7, 2021

@Redirion Redirion marked this pull request as ready for review November 7, 2021 16:57
@litetex
Copy link
Member

litetex commented Nov 12, 2021

Smoke tested it on the following devices:

  • Pixel 3a (emulated) API 31 - Android 11
  • Pixel 3a (emulated) API 30 - Android 10
  • Huawei P20 lite API 28 - Android 9
  • Samsung Galaxy S3 API 19 - Android 4.4

The following works as expected:

  • Playing a normal video
    • Different speeds work
    • Different qualities work
  • Background player
  • Popup player

@litetex
Copy link
Member

litetex commented Nov 12, 2021

@Redirion

I don't understand the changes inside PlaybackResolver completely.
You applied some changes to some factories.

However I would change it like his:

diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/PlaybackResolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/PlaybackResolver.java
index 48ee305ee..4dc7e7002 100644
--- a/app/src/main/java/org/schabi/newpipe/player/resolver/PlaybackResolver.java
+++ b/app/src/main/java/org/schabi/newpipe/player/resolver/PlaybackResolver.java
@@ -9,6 +9,7 @@ import androidx.annotation.Nullable;
 import com.google.android.exoplayer2.C;
 import com.google.android.exoplayer2.MediaItem;
 import com.google.android.exoplayer2.source.MediaSource;
+import com.google.android.exoplayer2.source.MediaSourceFactory;
 import com.google.android.exoplayer2.util.Util;
 
 import org.schabi.newpipe.extractor.stream.StreamInfo;
@@ -41,25 +42,29 @@ public interface PlaybackResolver extends Resolver<StreamInfo, MediaSource> {
                                              @NonNull final String sourceUrl,
                                              @C.ContentType final int type,
                                              @NonNull final MediaSourceTag metadata) {
-        final Uri uri = Uri.parse(sourceUrl);
+        final MediaSourceFactory factory;
         switch (type) {
             case C.TYPE_SS:
-                return dataSource.getLiveSsMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(MediaItem.fromUri(uri));
+                factory = dataSource.getLiveSsMediaSourceFactory();
+                break;
             case C.TYPE_DASH:
-                return dataSource.getLiveDashMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(
-                                new MediaItem.Builder()
-                                        .setUri(uri)
-                                        .setLiveTargetOffsetMs(
-                                                PlayerDataSource.LIVE_STREAM_EDGE_GAP_MILLIS)
-                                        .build());
+                factory = dataSource.getLiveDashMediaSourceFactory();
+                break;
             case C.TYPE_HLS:
-                return dataSource.getLiveHlsMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(MediaItem.fromUri(uri));
+                factory = dataSource.getLiveHlsMediaSourceFactory();
+                break;
             default:
                 throw new IllegalStateException("Unsupported type: " + type);
         }
+
+        return factory.createMediaSource(
+                new MediaItem.Builder()
+                        .setTag(metadata)
+                        .setUri(Uri.parse(sourceUrl))
+                        .setLiveTargetOffsetMs(PlayerDataSource.LIVE_STREAM_EDGE_GAP_MILLIS)
+                        .build()
+        );
     }
 
     @NonNull
@@ -72,25 +77,30 @@ public interface PlaybackResolver extends Resolver<StreamInfo, MediaSource> {
         @C.ContentType final int type = TextUtils.isEmpty(overrideExtension)
                 ? Util.inferContentType(uri) : Util.inferContentType("." + overrideExtension);
 
+        final MediaSourceFactory factory;
         switch (type) {
             case C.TYPE_SS:
-                return dataSource.getLiveSsMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(MediaItem.fromUri(uri));
+                factory = dataSource.getLiveSsMediaSourceFactory();
+                break;
             case C.TYPE_DASH:
-                return dataSource.getDashMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(MediaItem.fromUri(uri));
+                factory = dataSource.getDashMediaSourceFactory();
+                break;
             case C.TYPE_HLS:
-                return dataSource.getHlsMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(MediaItem.fromUri(uri));
+                factory = dataSource.getHlsMediaSourceFactory();
+                break;
             case C.TYPE_OTHER:
-                return dataSource.getExtractorMediaSourceFactory().setTag(metadata)
-                        .createMediaSource(
-                                new MediaItem.Builder()
-                                        .setUri(uri)
-                                        .setCustomCacheKey(cacheKey)
-                                        .build());
+                factory = dataSource.getExtractorMediaSourceFactory();
+                break;
             default:
                 throw new IllegalStateException("Unsupported type: " + type);
         }
+
+        return factory.createMediaSource(
+                new MediaItem.Builder()
+                    .setTag(metadata)
+                    .setUri(uri)
+                    .setCustomCacheKey(cacheKey)
+                    .build()
+        );
     }
 }

This has a few advantages:

  1. It fixes the deprecation of setTag
  2. It makes the code more consistent
  3. It de-duplicates a lot of code

Update from IRC:
I can add the above changes if it works 😄

@litetex
Copy link
Member

litetex commented Nov 12, 2021

** Updated test results (with above changes) **

Smoke tested it on the following devices:

  • Pixel 3a (emulated) API 31 - Android 11
  • Pixel 3a (emulated) API 30 - Android 10
  • Huawei P20 lite API 28 - Android 9
  • Samsung Galaxy S3 API 19 - Android 4.4

The following works as expected:

  • Playing a normal video
    • Different speeds/pitch work
    • Different qualities work
  • Background player
  • Popup player
  • Watching a live-stream

* fixes the deprecation of ``setTag``
* makes the code more consistent
* de-duplicates some code
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.

LGTM

@Redirion
Copy link
Member Author

Thank you litetex

@Redirion Redirion merged commit dfba10f into TeamNewPipe:dev Nov 12, 2021
@Redirion Redirion deleted the exo14 branch November 12, 2021 19:19
@opusforlife2
Copy link
Collaborator

Woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants