From 76eb751438b244ac27822372c602e86e55c7f064 Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Fri, 5 Jan 2024 19:36:09 +0100 Subject: [PATCH 1/5] Simplify selection of video stream I was trying to understand the logic here, and noticed the indirection via a QualityResolver interfaces is pretty unnecessary. Just branching directly makes the logic a lot easier to follow. The `-999` sentinel value is a bit dumb, but java does not recognize that videoIndex is always initialized. Nice side-effect, the `Resolver` interface was completely unused and can be dropped. --- .../org/schabi/newpipe/player/Player.java | 29 ++------ .../resolver/AudioPlaybackResolver.java | 1 - .../player/resolver/PlaybackResolver.java | 5 +- .../newpipe/player/resolver/Resolver.java | 9 --- .../resolver/VideoPlaybackResolver.java | 66 +++++++++++++------ 5 files changed, 53 insertions(+), 57 deletions(-) delete mode 100644 app/src/main/java/org/schabi/newpipe/player/resolver/Resolver.java diff --git a/app/src/main/java/org/schabi/newpipe/player/Player.java b/app/src/main/java/org/schabi/newpipe/player/Player.java index 49e72328e..8d5978c5f 100644 --- a/app/src/main/java/org/schabi/newpipe/player/Player.java +++ b/app/src/main/java/org/schabi/newpipe/player/Player.java @@ -42,8 +42,6 @@ import static org.schabi.newpipe.player.notification.NotificationConstants.ACTIO import static org.schabi.newpipe.player.notification.NotificationConstants.ACTION_RECREATE_NOTIFICATION; import static org.schabi.newpipe.player.notification.NotificationConstants.ACTION_REPEAT; import static org.schabi.newpipe.player.notification.NotificationConstants.ACTION_SHUFFLE; -import static org.schabi.newpipe.util.ListHelper.getPopupResolutionIndex; -import static org.schabi.newpipe.util.ListHelper.getResolutionIndex; import static org.schabi.newpipe.util.Localization.assureCorrectAppLanguage; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -116,7 +114,6 @@ import org.schabi.newpipe.player.ui.PlayerUiList; import org.schabi.newpipe.player.ui.PopupPlayerUi; import org.schabi.newpipe.player.ui.VideoPlayerUi; import org.schabi.newpipe.util.DependentPreferenceHelper; -import org.schabi.newpipe.util.ListHelper; import org.schabi.newpipe.util.NavigationHelper; import org.schabi.newpipe.util.image.PicassoHelper; import org.schabi.newpipe.util.SerializedCache; @@ -292,7 +289,7 @@ public final class Player implements PlaybackListener, Listener { context.getString( R.string.use_exoplayer_decoder_fallback_key), false)); - videoResolver = new VideoPlaybackResolver(context, dataSource, getQualityResolver()); + videoResolver = new VideoPlaybackResolver(context, dataSource); audioResolver = new AudioPlaybackResolver(context, dataSource); currentThumbnailTarget = getCurrentThumbnailTarget(); @@ -306,25 +303,6 @@ public final class Player implements PlaybackListener, Listener { new NotificationPlayerUi(this) ); } - - private VideoPlaybackResolver.QualityResolver getQualityResolver() { - return new VideoPlaybackResolver.QualityResolver() { - @Override - public int getDefaultResolutionIndex(final List sortedVideos) { - return videoPlayerSelected() - ? ListHelper.getDefaultResolutionIndex(context, sortedVideos) - : ListHelper.getPopupDefaultResolutionIndex(context, sortedVideos); - } - - @Override - public int getOverrideResolutionIndex(final List sortedVideos, - final String playbackQuality) { - return videoPlayerSelected() - ? getResolutionIndex(context, sortedVideos, playbackQuality) - : getPopupResolutionIndex(context, sortedVideos, playbackQuality); - } - }; - } //endregion @@ -1908,7 +1886,10 @@ public final class Player implements PlaybackListener, Listener { // Note that the video is not fetched when the app is in background because the video // renderer is fully disabled (see useVideoSource method), except for HLS streams // (see https://github.com/google/ExoPlayer/issues/9282). - return videoResolver.resolve(info); + return videoResolver.resolve(info, videoPlayerSelected() + ? VideoPlaybackResolver.SelectedPlayer.MAIN + : VideoPlaybackResolver.SelectedPlayer.POPUP + ); } public void disablePreloadingOfCurrentTrack() { diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/AudioPlaybackResolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/AudioPlaybackResolver.java index 2d4404b2a..9c0dc0edb 100644 --- a/app/src/main/java/org/schabi/newpipe/player/resolver/AudioPlaybackResolver.java +++ b/app/src/main/java/org/schabi/newpipe/player/resolver/AudioPlaybackResolver.java @@ -45,7 +45,6 @@ public class AudioPlaybackResolver implements PlaybackResolver { * @param info of the stream * @return the audio source to use or null if none could be found */ - @Override @Nullable public MediaSource resolve(@NonNull final StreamInfo info) { final MediaSource liveSource = PlaybackResolver.maybeBuildLiveMediaSource(dataSource, info); 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 e204b8372..de0d5be73 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 @@ -46,11 +46,10 @@ import java.nio.charset.StandardCharsets; import java.util.Objects; /** - * This interface is just a shorthand for {@link Resolver} with {@link StreamInfo} as source and - * {@link MediaSource} as product. It contains many static methods that can be used by classes + * This interface contains many static methods that can be used by classes * implementing this interface, and nothing else. */ -public interface PlaybackResolver extends Resolver { +public interface PlaybackResolver { String TAG = PlaybackResolver.class.getSimpleName(); diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/Resolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/Resolver.java deleted file mode 100644 index a3e1db5b4..000000000 --- a/app/src/main/java/org/schabi/newpipe/player/resolver/Resolver.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.schabi.newpipe.player.resolver; - -import androidx.annotation.NonNull; -import androidx.annotation.Nullable; - -public interface Resolver { - @Nullable - Product resolve(@NonNull Source source); -} diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java index 670c13934..496506454 100644 --- a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java +++ b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java @@ -39,8 +39,6 @@ public class VideoPlaybackResolver implements PlaybackResolver { private final Context context; @NonNull private final PlayerDataSource dataSource; - @NonNull - private final QualityResolver qualityResolver; private SourceType streamSourceType; @Nullable @@ -54,17 +52,23 @@ public class VideoPlaybackResolver implements PlaybackResolver { VIDEO_WITH_AUDIO_OR_AUDIO_ONLY } - public VideoPlaybackResolver(@NonNull final Context context, - @NonNull final PlayerDataSource dataSource, - @NonNull final QualityResolver qualityResolver) { - this.context = context; - this.dataSource = dataSource; - this.qualityResolver = qualityResolver; + /** + * Depending on the player we select different video streams. + */ + public enum SelectedPlayer { + MAIN, + POPUP + } + + public VideoPlaybackResolver(@NonNull final Context context, + @NonNull final PlayerDataSource dataSource) { + this.context = context; + this.dataSource = dataSource; } - @Override @Nullable - public MediaSource resolve(@NonNull final StreamInfo info) { + public MediaSource resolve(@NonNull final StreamInfo info, + @NonNull final SelectedPlayer selectedPlayer) { final MediaSource liveSource = PlaybackResolver.maybeBuildLiveMediaSource(dataSource, info); if (liveSource != null) { streamSourceType = SourceType.LIVE_STREAM; @@ -80,14 +84,42 @@ public class VideoPlaybackResolver implements PlaybackResolver { final List audioStreamsList = getFilteredAudioStreams(context, info.getAudioStreams()); - final int videoIndex; + int videoIndex = -999; if (videoStreamsList.isEmpty()) { videoIndex = -1; } else if (playbackQuality == null) { - videoIndex = qualityResolver.getDefaultResolutionIndex(videoStreamsList); + switch (selectedPlayer) { + case MAIN -> { + videoIndex = ListHelper.getDefaultResolutionIndex( + context, + videoStreamsList + ); + } + case POPUP -> { + videoIndex = ListHelper.getPopupDefaultResolutionIndex( + context, + videoStreamsList + ); + } + } + } else { - videoIndex = qualityResolver.getOverrideResolutionIndex(videoStreamsList, - getPlaybackQuality()); + switch (selectedPlayer) { + case MAIN -> { + videoIndex = ListHelper.getResolutionIndex( + context, + videoStreamsList, + getPlaybackQuality() + ); + } + case POPUP -> { + videoIndex = ListHelper.getPopupResolutionIndex( + context, + videoStreamsList, + getPlaybackQuality() + ); + } + } } final int audioIndex = @@ -195,10 +227,4 @@ public class VideoPlaybackResolver implements PlaybackResolver { public void setAudioTrack(@Nullable final String audioLanguage) { this.audioTrack = audioLanguage; } - - public interface QualityResolver { - int getDefaultResolutionIndex(List sortedVideos); - - int getOverrideResolutionIndex(List sortedVideos, String playbackQuality); - } } From 0148d65cab7f8eb0580cae36da0aae2f719c68c3 Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Sat, 6 Jan 2024 13:47:30 +0100 Subject: [PATCH 2/5] Inline getDefaultResolutionWithDefaultFormat --- .../resolver/VideoPlaybackResolver.java | 12 ++--- .../org/schabi/newpipe/util/ListHelper.java | 44 +++++++------------ 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java index 496506454..4febccf18 100644 --- a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java +++ b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java @@ -106,17 +106,17 @@ public class VideoPlaybackResolver implements PlaybackResolver { } else { switch (selectedPlayer) { case MAIN -> { - videoIndex = ListHelper.getResolutionIndex( + videoIndex = ListHelper.getDefaultResolutionWithDefaultFormat( context, - videoStreamsList, - getPlaybackQuality() + getPlaybackQuality(), + videoStreamsList ); } case POPUP -> { - videoIndex = ListHelper.getPopupResolutionIndex( + videoIndex = ListHelper.getDefaultResolutionWithDefaultFormat( context, - videoStreamsList, - getPlaybackQuality() + getPlaybackQuality(), + videoStreamsList ); } } diff --git a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java index 71071d997..3017b153b 100644 --- a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java @@ -83,18 +83,6 @@ public final class ListHelper { return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); } - /** - * @param context Android app context - * @param videoStreams list of the video streams to check - * @param defaultResolution the default resolution to look for - * @return index of the video stream with the default index - * @see #getDefaultResolutionIndex(String, String, MediaFormat, List) - */ - public static int getResolutionIndex(final Context context, - final List videoStreams, - final String defaultResolution) { - return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); - } /** * @param context Android app context @@ -109,19 +97,6 @@ public final class ListHelper { return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); } - /** - * @param context Android app context - * @param videoStreams list of the video streams to check - * @param defaultResolution the default resolution to look for - * @return index of the video stream with the default index - * @see #getDefaultResolutionIndex(String, String, MediaFormat, List) - */ - public static int getPopupResolutionIndex(final Context context, - final List videoStreams, - final String defaultResolution) { - return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); - } - public static int getDefaultAudioFormat(final Context context, final List audioStreams) { return getAudioIndexByHighestRank(audioStreams, @@ -634,7 +609,7 @@ public final class ListHelper { * @param videoStreams the list of video streams to check * @return the index of the preferred video stream */ - private static int getDefaultResolutionWithDefaultFormat(@NonNull final Context context, + public static int getDefaultResolutionWithDefaultFormat(@NonNull final Context context, final String defaultResolution, final List videoStreams) { final MediaFormat defaultFormat = getDefaultFormat(context, @@ -680,6 +655,14 @@ public final class ListHelper { return format; } + /** #Comparator for two resolution strings. + * + * See {@link #sortStreamList} for ordering. + * + * @param r1 first + * @param r2 second + * @return comparison int + */ private static int compareVideoStreamResolution(@NonNull final String r1, @NonNull final String r2) { try { @@ -696,12 +679,17 @@ public final class ListHelper { } } + /** Does the application have a maximum resolution set? + * + * @param context App context + * @return whether a max resolution is set + */ static boolean isLimitingDataUsage(@NonNull final Context context) { return getResolutionLimit(context) != null; } /** - * The maximum resolution allowed. + * The maximum resolution allowed by application settings. * * @param context App context * @return maximum resolution allowed or null if there is no maximum @@ -720,7 +708,7 @@ public final class ListHelper { } /** - * The current network is metered (like mobile data)? + * Is the current network metered (like mobile data)? * * @param context App context * @return {@code true} if connected to a metered network From be4e0cb3dc555512a7bcbc075baf7a469f8bc94f Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Sat, 6 Jan 2024 13:50:20 +0100 Subject: [PATCH 3/5] Remove now unnecessary switch --- .../resolver/VideoPlaybackResolver.java | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java index 4febccf18..14c1f9f1e 100644 --- a/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java +++ b/app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java @@ -102,24 +102,12 @@ public class VideoPlaybackResolver implements PlaybackResolver { ); } } - } else { - switch (selectedPlayer) { - case MAIN -> { - videoIndex = ListHelper.getDefaultResolutionWithDefaultFormat( - context, - getPlaybackQuality(), - videoStreamsList - ); - } - case POPUP -> { - videoIndex = ListHelper.getDefaultResolutionWithDefaultFormat( - context, - getPlaybackQuality(), - videoStreamsList - ); - } - } + videoIndex = ListHelper.getDefaultResolutionWithDefaultFormat( + context, + getPlaybackQuality(), + videoStreamsList + ); } final int audioIndex = From e5ac6824e8117fb4c2b9185da9891b160e7b8e3b Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Sat, 6 Jan 2024 14:20:07 +0100 Subject: [PATCH 4/5] Remove null check on getDefaultSharedPreferences The function never returns `null`. --- app/src/main/java/org/schabi/newpipe/util/ListHelper.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java index 3017b153b..0acd476fe 100644 --- a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java @@ -371,9 +371,8 @@ public final class ListHelper { PreferenceManager.getDefaultSharedPreferences(context); // Load the preferred resolution otherwise the best available - String resolution = preferences != null - ? preferences.getString(context.getString(key), context.getString(value)) - : context.getString(R.string.best_resolution_key); + String resolution = + preferences.getString(context.getString(key), context.getString(value)); final String maxResolution = getResolutionLimit(context); if (maxResolution != null From 71d88d0bc0c6023edf5ca5e2ed11bb755f667a21 Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Sat, 6 Jan 2024 15:12:43 +0100 Subject: [PATCH 5/5] ListHelper: rename functions & variables and add documentation This should make the purpose of these functions more clear. --- .../org/schabi/newpipe/util/ListHelper.java | 65 ++++++++++++------- .../newpipe/util/SecondaryStreamHelper.java | 4 +- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java index 0acd476fe..978048a84 100644 --- a/app/src/main/java/org/schabi/newpipe/util/ListHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/ListHelper.java @@ -78,7 +78,7 @@ public final class ListHelper { */ public static int getDefaultResolutionIndex(final Context context, final List videoStreams) { - final String defaultResolution = computeDefaultResolution(context, + final String defaultResolution = getPreferredResolutionOrCurrentLimit(context, R.string.default_resolution_key, R.string.default_resolution_value); return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); } @@ -92,7 +92,7 @@ public final class ListHelper { */ public static int getPopupDefaultResolutionIndex(final Context context, final List videoStreams) { - final String defaultResolution = computeDefaultResolution(context, + final String defaultResolution = getPreferredResolutionOrCurrentLimit(context, R.string.default_popup_resolution_key, R.string.default_popup_resolution_value); return getDefaultResolutionWithDefaultFormat(context, defaultResolution, videoStreams); } @@ -365,22 +365,42 @@ public final class ListHelper { .collect(Collectors.toList()); } - private static String computeDefaultResolution(@NonNull final Context context, final int key, - final int value) { + /** Lookup the preferred resolution and the current resolution limit. + * + * @param context App context + * @param defaultResolutionKey The defaultResolution preference key + * @param defaultResolutionDefaultValue Default resolution if key does not exist + * @return The smaller resolution of either the preference or the current limit. + */ + private static String getPreferredResolutionOrCurrentLimit( + @NonNull final Context context, + final int defaultResolutionKey, + final int defaultResolutionDefaultValue + ) { final SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(context); // Load the preferred resolution otherwise the best available - String resolution = - preferences.getString(context.getString(key), context.getString(value)); + final String preferredResolution = preferences.getString( + context.getString(defaultResolutionKey), + context.getString(defaultResolutionDefaultValue) + ); - final String maxResolution = getResolutionLimit(context); - if (maxResolution != null - && (resolution.equals(context.getString(R.string.best_resolution_key)) - || compareVideoStreamResolution(maxResolution, resolution) < 1)) { - resolution = maxResolution; + // clamp to the currently maximum allowed resolution + final String result; + final String resolutionLimit = getCurrentResolutionLimit(context); + if (resolutionLimit != null + && ( + // if the preference is best_resolution + preferredResolution.equals(context.getString(R.string.best_resolution_key)) + // or the preference is higher than the current max allowed resolution + || compareVideoStreamResolution(resolutionLimit, preferredResolution) < 1 + )) { + result = resolutionLimit; + } else { + result = preferredResolution; } - return resolution; + return result; } /** @@ -601,7 +621,7 @@ public final class ListHelper { /** * Fetches the desired resolution or returns the default if it is not found. - * The resolution will be reduced if video chocking is active. + * The resolution will be reduced if video choking is active. * * @param context Android app context * @param defaultResolution the default resolution @@ -683,27 +703,28 @@ public final class ListHelper { * @param context App context * @return whether a max resolution is set */ - static boolean isLimitingDataUsage(@NonNull final Context context) { - return getResolutionLimit(context) != null; + static boolean isCurrentlyLimitingDataUsage(@NonNull final Context context) { + return getCurrentResolutionLimit(context) != null; } /** - * The maximum resolution allowed by application settings. + * The maximum current resolution allowed by application settings. + * Takes into account whether we are on a metered network. * * @param context App context - * @return maximum resolution allowed or null if there is no maximum + * @return current maximum resolution allowed or null if there is no maximum */ - private static String getResolutionLimit(@NonNull final Context context) { - String resolutionLimit = null; + private static String getCurrentResolutionLimit(@NonNull final Context context) { + String currentResolutionLimit = null; if (isMeteredNetwork(context)) { final SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(context); final String defValue = context.getString(R.string.limit_data_usage_none_key); final String value = preferences.getString( context.getString(R.string.limit_mobile_data_usage_key), defValue); - resolutionLimit = defValue.equals(value) ? null : value; + currentResolutionLimit = defValue.equals(value) ? null : value; } - return resolutionLimit; + return currentResolutionLimit; } /** @@ -734,7 +755,7 @@ public final class ListHelper { final @NonNull Context context) { final MediaFormat defaultFormat = getDefaultFormat(context, R.string.default_audio_format_key, R.string.default_audio_format_value); - return getAudioFormatComparator(defaultFormat, isLimitingDataUsage(context)); + return getAudioFormatComparator(defaultFormat, isCurrentlyLimitingDataUsage(context)); } /** diff --git a/app/src/main/java/org/schabi/newpipe/util/SecondaryStreamHelper.java b/app/src/main/java/org/schabi/newpipe/util/SecondaryStreamHelper.java index 75d9a3892..c3677055e 100644 --- a/app/src/main/java/org/schabi/newpipe/util/SecondaryStreamHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/SecondaryStreamHelper.java @@ -31,7 +31,7 @@ public class SecondaryStreamHelper { * Finds an audio stream compatible with the provided video-only stream, so that the two streams * can be combined in a single file by the downloader. If there are multiple available audio * streams, chooses either the highest or the lowest quality one based on - * {@link ListHelper#isLimitingDataUsage(Context)}. + * {@link ListHelper#isCurrentlyLimitingDataUsage(Context)}. * * @param context Android context * @param audioStreams list of audio streams @@ -56,7 +56,7 @@ public class SecondaryStreamHelper { } final boolean m4v = mediaFormat == MediaFormat.MPEG_4; - final boolean isLimitingDataUsage = ListHelper.isLimitingDataUsage(context); + final boolean isLimitingDataUsage = ListHelper.isCurrentlyLimitingDataUsage(context); Comparator comparator = ListHelper.getAudioFormatComparator( m4v ? MediaFormat.M4A : MediaFormat.WEBMA, isLimitingDataUsage);