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

PoC for limited PIP mode support on MainPlayer. #8045

Closed
wants to merge 2 commits into from

Conversation

karyogamy
Copy link
Contributor

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

I know it might not a good idea to do this now before the full player refactor, but did it anyways since I keep on seeing reports of unusable Android 12 popups. To avoid making too many changes in the Player, I've restricted the PIP mode to be accessible through minimize action only. There is a user-defined option with clear warning this is a feature under test, since I just want this PR to unblock users so they can play with a minimally viable PIP while devs have more time refactoring the player. This also means the original popups will co-exist with PIP mode.

@Team, please let me know if there are major issues with this implementation or its later maintenance so I can retract the PR and don't go crazy because "this works on my machine". On the Android side, PIP mode is able to take the view at the top of the current activity and expose that through PIP. You can see this when expanding the PIP view, it will also show the VideoDetailFragment title, which may or may not be a desirable effect. But since this change aims to touch the internal player code as little as possible, the current video player fragment layout pretty much worked to our advantage.

Unfortunately, because of this, when other player ui elements are showing, the layout will clutter the PIP view and would need manual intervention by expand the activity (VideoDetailFragment fragment) back to fix it. This is especially apparent when a queue finishes playing or when the player throws an exception. There is also an issue with aspect ratio when restarting a queue after playback has ended. Though these problems can't really be fixed without a player ui update/rewrite. Similarly, because of how the app's activity and fragments are structured, while in PIP mode, you cannot access other parts of the app, such as changing settings or searching for videos. To do this, you will have to wait for a full player refactor IF the team decides to make the player an activity.

If you are comfortable with these caveats, please give the PR build a test, especially if you have an Android 12 device, and let me know if there is any non-ui related issue or if there is major issue with the video view.

Before/After Screenshots/Screen Record

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.
Once installed, go to Settings -> Video and audio -> Minimize on app switch -> Minimize to PIP ... -> Start up a video and context switch when the video is playing

Due diligence

added: minimize action setting to enable PIP behavior.
@triallax triallax added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels Mar 16, 2022
@sonarqubecloud
Copy link

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

@Stypox
Copy link
Member

Stypox commented Mar 16, 2022

This seems like a really hacky solution 😅. I'd merge this only if you plan on properly implementing it in the near future. One of the most important things to be done asap is in my opinion not using a minimize on app switch setting but rather having a switch setting like "Use PiP instead of NewPipe's custom popup". But I know this would require much more work, so it's ok if you don't do it here but it would have to be done asap.

@karyogamy
Copy link
Contributor Author

karyogamy commented Mar 16, 2022

This seems like a really hacky solution

@Stypox I absolutely agree. The dilemma is that we don't know how long the player rewrite/refactor will take. It might be a year or longer before the player gets to a good state for a proper PIP implementation, so it will very likely not be done or even considered in the near future. This PR is written in a way to make the PIP hack easily decoupled, hence the warning message, controller interface and lack of integration with popup buttons, in hopes it will take some heat off the lack of PIP support. The current solution should work so long as the fragment holding the player has the view positioned at the top of the activity.

Use PiP instead of NewPipe's custom popup

I tried this too, but the changes are way too intrusive since now all callers of enqueue to popup will need access to fragment manager on the activity instead of just Context. I want to touch the existing code as little as possible with this change.

@team It's perfectly understandable to not want this hack merged. Maybe we can go for an alternative such as putting it on the SB fork instead. Please let me know your thoughts.

@litetex
Copy link
Member

litetex commented Apr 15, 2022

As this is a PoC, I changed the PR to "Draft".

Someone should create a proper implementation in a new PR ;)

@litetex litetex marked this pull request as draft April 15, 2022 16:12
@AudricV
Copy link
Member

AudricV commented Apr 22, 2022

Closing in favor of #8279.

@AudricV AudricV closed this Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Can't interact with app/screen below popup player on Android 12
5 participants