-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add seamless transition between background and video players when putting the app in background (for video-only streams and audio-only streams only) #7349
Conversation
@alexlyzhov @Coderdude112 @Logarithmus @Darkmaster006 @IHateTheProgrammingClass @peepopoggers @avently @Stypox @opusforlife2 (pinging all participants of the corresponding issue) Can you please test this and see if it works for the cases where my changes should work (I described them on the PR body)? Thank you in advance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick code review (no testing, just on GH's web-interface):
app/src/main/java/org/schabi/newpipe/player/resolver/Resolver.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java
Outdated
Show resolved
Hide resolved
100% of videos I found are working without reload. Even ten years old. Is it real? Did google separate audio with video for all videos or at least for a big part of all videos? It's amazing. When I tested this feature a couple of years ago it weren't so many videos with separated audio source. Anyway, works cool at least for initial testing without hard scenarios (just background/foreground, screen on/off). Good job! |
On my end it also seems to be working correctly. Tried clicking everywhere, and it doesn't ever seem to interrupt, great work, thank you so much! |
I'm on a Pixel 3 running Android 12, and now I'm having the same issue as #6770 but also when the video is set to background. Other than that, the video is seamless when in popup and turning on / off the screen. Other than that I noticed the pause when clicking on the background / popup buttons is astronomically smaller, though it does reset the playback position. |
@TiA4f8R You are a Super Saiyan. |
This comment has been minimized.
This comment has been minimized.
It's intended, this video has OTF streams which are not supported by NewPipe right now (the support of these streams will be added in #6537). So only the highest quality (1080p) and 360p are video-only streams which can be played by NewPipe. So the 720p stream available is a video stream with audio (itag 22). So to prevent spending more data than needed, the previous process still happen (reload the player to play an audio stream).
Does this happen when you remove the permission to display over other apps or when using the workaround with adb described in the issue? Anyway, I will be not able to fix both of the issues (the first needs to implement Android PIP support as described in the issue, the second is #4497). |
If I revoke the permission to draw over apps and then set the playback to background it works as expected, if I set the playback to popup I get an error from NewPipe saying permission to draw over other apps is required. If I am understanding correctly this PR solves the issue of the pause occurring when the playback is set to popup and the screen turns off or on. Is this correct? Also, I will try the adb fix later today when I get home |
works great |
This comment has been minimized.
This comment has been minimized.
I have tested this on a |
066d000
to
5f8d49d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@TiA4f8R what's the reason for the last commit (progress loop releated)? It will be a reason for some bugs in the future. Right now when hide the app to background and stopped a played video or paused it, when you return to the app, you'll see a wrong timestamp under the video. Also as I far as I remember the progress loop saves currently played timestamp to db which then will be used before opening the same video. So an optimization you tried to make doesn't cost upcoming bugs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another few comments from my side (the old ones above are still valid)
I also don't think 5f8d49d is going to work. Because even when the video is disabled (videoEnabled
) we need to update the PlayerNotfication.
I would also suggest doing stuff like optimizing the progress loop in another PR, as this seems to be unrelated to the current one.
5f8d49d
to
1677f2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggested changes.
Did a quick test and it seems to work as expected.
This comment has been minimized.
This comment has been minimized.
@9Orlaf2 This will be not fixed with this PR, as the bug is not related to my changes (I can reproduce it too on video 5lMmnfVylEE for example). However, the stream can be rewinded a bit (30 seconds), so you can rewind or pause a bit the stream to prevent the issue (segments have more time to be fetched and the buffering should not happen or happen less frequently). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good. Thank you for such a great PR :-D
I tested normal player and popup player on my Android 10 phone and it worked as expected
app/src/main/java/org/schabi/newpipe/player/resolver/VideoPlaybackResolver.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Unable to compile! * Cleaned up ``getMostCompactAudioIndex`` and ``getHighestQualityAudioIndex`` into a new method ``getAudioIndexByHighestRank`` * Removed unreadable code and use Java Streams API * Tests work as expected
* Replaced by ``wasLastResolvedVideoAndAudioSeparated`` * Uses an ``Optional`` instead (we can't determine if the video and audio streams are separated when we did not fetch it)
Removed fixed problems.
* Fixed expected and actual results. They were reversed... * Added new method ``getSortedStreamVideosListWithPreferVideoOnlyStreamsTest``
Reload the play queue manager and set the recovery in this case, like on the current behavior (without this PR).
This commit also allows a seamless transition for livestreams.
…streams in background ExoPlayer right now fetches HLS video tracks even if you disable them (with setRendererDisabled or setSelectionOverride). See issue 9282 of ExoPlayer's issue tracker for more information.
8b32937
to
c5fc371
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will merge after 0.22.1 :-)
@Stypox, why not to merge it in 0.22.1? |
0.22.1 is a small bugfix release mostly fixing regressions, and we want to publish it fast without dedicating too much testing to it |
0.22.1 has it's own branch 😉 |
What is it?
Description of the changes in your PR
This PR makes the app do a seamless transition between main or popup player and background player when putting the app in background (for e.g. when turning off your device or switching to recent apps).
This will only happen when you are playing video-only streams or audio-only streams (when there are audio streams available and only video streams with audio, the pause when backgrounding the app will still happen, in order to reduce data usage; in the case when there are only video streams with audio and no audio streams, no pause will happen).
This doesn't affect the pause which occur when pressing the
Background
or thePopup
button (the current player type is destroyed and a new player type is created).A black video is shown for a few seconds after you come back to the player. It is intended, because the video is not fetched when you are in background, to prevent you to spend more data than needed to play the audio. This is the behavior of the YouTube app on Android (for Premium clients only of course).
Note that livestreams will still fetch the video stream (because streams we are getting right now are not demuxed (this will probably change soon) and there is an ExoPlayer bug which still fetches video even if it has been disabled with the track selector for HLS contents (the delivery method we can only use for livestreams right now) (a low priority has been given by the ExoPlayer team to this bug)).
This PR makes also the player use video-only streams first (instead of video streams with audio), when possible.
Fixes the following issue(s)
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