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

Use picture-in-picture (PIP) for the popup player on Android >= 8.0 #8750

Closed
wants to merge 9 commits into from

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Aug 8, 2022

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

  • Allow Android's picture-in-picture (PIP) mode to be used in the popup player functionality on Android 8.0 and later.
  • Add setting to configure the popup mode (PiP or NewPipe's implementation.)
  • TODO: Use PiP in the "Start playing in popup" option as well. Currently it uses the NewPipe popup.

Before/After Screenshots/Screen Record

  • Before:

Screenshot_20220808-204326_NewPipe

Screenshot_20220820-175724_NewPipe

  • After:
device-2022-08-08-204526.mp4

Screenshot_20220820-175928_NewPipe

Screenshot_20220820-173226_NewPipe

Screenshot_20220820-173112_NewPipe

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

@AudricV
Copy link
Member

AudricV commented Aug 8, 2022

The PiP mode should be configurable and not enforced, in my opinion, as it have less advantages than the popup player such as:

  • setting the quality played, subtitles and playback speed used;
  • being not resizable on Android 10 and lower.

Good luck with the remaining work to make the feature working properly, including player UI changes!

You may use a separate Player UI for the PiP usage, though @Stypox may have better input on the subject, as they refactored how player UIs are in the app.

Also, be sure to enable PiP only if the feature is available on the device (some Android Go devices may not have this feature).

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 8, 2022

The PiP mode should be configurable and not enforced, in my opinion

Yeah, that sounds good to me. Allowing the user to choose between the popup and PiP could be useful.

  • setting the quality played, subtitles and playback speed used;

I set those views to be hidden upon entering PiP for the moment. They could be shown and used instead of the standard controls.

  • being not resizable on Android 10 and lower.

Good luck with the remaining work to make the feature working properly, including player UI changes!

Thank you!

You may use a separate Player UI for the PiP usage, though @Stypox may have better input on the subject, as they refactored how player UIs are in the app.

Got it, thanks for the input.

@AudricV AudricV added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) device/software specific Issues that only happen on some devices or with some specific hardware/software labels Aug 8, 2022
@opusforlife2
Copy link
Collaborator

Whoa. Isira not afraid to take on mammoth feature work, I see. :O

@opusforlife2

This comment was marked as off-topic.

@triallax

This comment was marked as off-topic.

@opusforlife2

This comment was marked as off-topic.

@TacoTheDank
Copy link
Member

The method currently overridden is deprecated (after two API versions, in classic Android fashion). Is there a way to use the new one on the Android versions that support it?

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 9, 2022

The method currently overridden is deprecated (after two API versions, in classic Android fashion).

The overridden method is in the Fragment. That method isn't deprecated.

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 10, 2022

Added a setting to configure the popup mode.

Screenshot_20220810-084846_NewPipe PIP_mode.jpg

At the moment, the issue is that the controls (resolution, etc.) aren't selectable in PiP. Maybe additional contributors could add changes here as well?

@AudricV
Copy link
Member

AudricV commented Aug 10, 2022

At the moment, the issue is that the controls (resolution, etc.) aren't selectable in PiP.

That's one of the limitations of Android Picture in Picture's feature: you can only tap on the actions provided at the bottom on the PiP window and tapping on everywhere else in the window would increase the PiP size on API 29 and lower. See https://developer.android.com/guide/topics/ui/picture-in-picture#handling_ui for more details.

By the way, it seems you have a few unwanted changes on your pushed code.

You may also show a message, probably a toast, when the PiP option it is not available, saying to check that the permission to use PiP may be not enabled or not available on the device NewPipe is running. Note that I am not sure about this.

@B0pol
Copy link
Member

B0pol commented Aug 17, 2022

Added a setting to configure the popup mode.

I’d call that Android Picture-in-picture and NewPipe picture-in-picture and also put in under video section, not appearance

Also the video player is broken, fullscreen button won’t fullscreen until you play/pause, then if you exit fullscreen it will do wtf and touch video controls still apply (like swipe up for volume up) until you press back button
Screenshot_20220817-223923_NewPipe_PIP_mode
Screenshot_20220817-223934_NewPipe_PIP_mode

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Aug 18, 2022

Also the video player is broken, fullscreen button won’t fullscreen until you play/pause, then if you exit fullscreen it will do wtf and touch video controls still apply (like swipe up for volume up) until you press back button

Yeah, that's why this is a draft for the moment. @Stypox is doing some refactoring of the player code: #8731 #8678

@opusforlife2
Copy link
Collaborator

Holy scrolling, guys. Please truncate your quoted texts.

@TacoTheDank
Copy link
Member

TacoTheDank commented Sep 9, 2022

What's with the massive values-v24 key string files? I'm pretty sure those aren't necessary, as keys can be accessed regardless of the SDK version.

Also, instead of copying everything in video_audio_settings.xml to xml-v24, just add it to the regular one, and have the preference be hidden below API 24 via code like this:

Lines 54-60 in VideoAudioSettingsFragment.java:

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
    final ListPreference popupConfigPreference =
            findPreference(getString(R.string.popup_configuration_key));
    if (popupConfigPreference != null) {
        popupConfigPreference.setVisible(false);
    }
}

No dealing with duplicate XML :)

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Oct 5, 2022

Also the video player is broken, fullscreen button won’t fullscreen until you play/pause, then if you exit fullscreen it will do wtf and touch video controls still apply (like swipe up for volume up) until you press back button !

I'm still having this issue with full screen not working until I play/pause, and general glitchiness surrounding changing from the main player to PIP. I can elaborate further if needed; I thought these issues would be resolved with the play refactoring (which is done already as far as I know?)

Hmm, I was under the impression that one of the PRs I mentioned earlier was for this issue, my bad. As for switching to PiP, I'll make changes for that.

Anyway, thank you for your hard work on this!

👍

@jawz101
Copy link

jawz101 commented Oct 26, 2022

This debug build with Android's newer PiP feature does not crash on my Pixel running Android 13 while the older Newpipe pop-up crashes every time.

I get the same full screen play/pause issue as the previous person but I'd rather deal with that than crashes

Send it pls!

@opusforlife2
Copy link
Collaborator

Send it pls!

@jawz101 Send it where?

@jawz101
Copy link

jawz101 commented Oct 27, 2022

Send it pls!

@jawz101 Send it where?

Into the app

@opusforlife2
Copy link
Collaborator

Bugs:

  • Start a video in background. Switch to popup. The PIP shows the progress bar below the thumbnail, and nothing else. So it's like a PIP-audio, which is quite useless. This means that PIP seems to capture the video details page this way. It should only capture the player view.
  • Portrait-only: going back from PIP may cause the embedded (portrait) player view to change size. It can go from slightly smaller to very small, with increasingly wide vertical black bars, but no horizontal ones.
  • If you try to start a video in popup even once, the "Hold to enqueue" tooltip will permanently show up and never go away, until the app is cleared from Recents.
  • Starting a video in embedded portrait mode and then tapping the full screen button causes it to go landscape, but not full screen.
  • Going full screen seems to add an extra 'layer' to the backstack. If you tap full screen again to go back, you can tap Back, but you will stay on the same screen, and the video will pause. This 'layer' lets you use the brightness and volume gestures in the embedded portrait player, and disables the player swiping down gesture instead.
  • While in full screen, switching to popup causes the PIP to show up, but with a black screen, and only audio playing.

@Isira-Seneviratne
Copy link
Member Author

Bugs:

@opusforlife2 Thanks for the feedback. I'm away from home at the moment, I'll look into those when I get back.

@Isira-Seneviratne
Copy link
Member Author

Been a bit busy these days, I'll try and look into the issues this evening.

@tsiflimagas
Copy link
Contributor

After the rebase, PiP player retains the navigation bar and a part of the top is black (about half of the height for small size, and a thinner strip when player is stretced).
IMG_20221105_173754

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Nov 10, 2022

After the rebase, PiP player retains the navigation bar and a part of the top is black (about half of the height for small size, and a thinner strip when player is stretced).

@AudricV Do you know if there have been any recent changes to the video player UI? Up until I rebased onto dev, the UI hiding functionality was working fine.

@sonarcloud
Copy link

sonarcloud bot commented Nov 13, 2022

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

@AudricV AudricV changed the title Use picture-in-picture (PIP) for the popup player on Android >= 7.0. Use picture-in-picture (PIP) for the popup player on Android >= 8.0 Apr 13, 2023
@Stypox
Copy link
Member

Stypox commented Mar 28, 2024

This will be properly considered when the player will be rewritten from scratch. Thank you for the contribution and for the insights anyway, they are sure going to be helpful!

@Stypox Stypox closed this Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
device/software specific Issues that only happen on some devices or with some specific hardware/software feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android 8.0+] Add Android Picture in Picture support