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

Refactor how the player is started or changed #9225

Closed
wants to merge 2 commits into from

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Oct 27, 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

At the moment the code is in a highly experimental state. I don't know if the current approach is the right one: with this PR changes to the running player can be made through a static object, and while this seems to work well, I suspect there might be some concurrency/syncing/static problems, which could be causing unexpected crashes.

TODO

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.

I expect crashes or wrong behaviors to happen in multiple scenarios, since I changed everything, and I mean everything, related to how the player is started or changed. Therefore please test the APK and try to spot as many bugs as possible, possibly providing STRs! @opusforlife2 @SameenAhnaf

Due diligence

@opusforlife2
Copy link
Collaborator

Oh wow here we go.

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Oct 27, 2022
@killerrook
Copy link

killerrook commented Oct 28, 2022

There are some problems with background play

  1. Sometimes the player crashes on long press> start playing in background (may be something to do with speed try <50kbps)(non consistent)
  2. Sometimes no minimized player appears on the bottom of screen when the player doesn't crash after following 1 (consistent)
  3. Sometimes the title in notification and player changes when start playing in background is used but doesn't start playing. No playing in background toast is shown. Although if you click on play either in the notification or player the video works fine (consistent)

Video for issue 2

Record_2022-10-28-09-41-39_f07feaf9423ebf52102ad95eb98f2c1e.mp4

Video for issue 3

Record_2022-10-28-09-46-35_f07feaf9423ebf52102ad95eb98f2c1e.mp4

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Oct 29, 2022

Here we go indeed:

  • Switching away from a video and coming back causes the status bar to butt in to the screen in its full red glory. If there is a software nav bar, that shows up too.
  • Playing a video in portrait and then tapping full screen causes the app to go landscape without actually going full screen.
  • Tapping full screen while a video is playing in full screen causes it to rotate to portrait and then rotate back instantly.
  • Tapping full screen while the video is paused causes it to rotate normally. But tapping full screen again causes it to go landscape without the full screen.
  • When you load a video page after opening the app once, and then open another one (either with or without closing the first one completely), the 1st video page remains stuck on the screen until it is immediately replaced by the 2nd video page. This remains the case until the app is closed and removed from RAM. The buffering indicator and loading UI is hidden from the user.
  • If one video page is open and a video is playing in the main player, tapping a 2nd video will cause its details page to load... but the 1st video won't stop playing.
  • If audio is played instead of video like the previous point, tapping a 2nd video causes the 1st video's page to open again instead. Sometimes a stutter is noticed.
  • Tapping on "start background playback" from the long-press menu of a list item doesn't create a notification until the stream is ready to play. Basically, it doesn't appear for the duration the loading icon would have been shown.

@opusforlife2
Copy link
Collaborator

Currently, if a video is being loaded in the main player but hasn't started playing yet (meaning that a video and audio stream is being fetched), does tapping Background cause the whole thing to reload again, or does it only discard the fetching of the video stream and continue loading the audio stream?

@bazipn
Copy link

bazipn commented Dec 4, 2022

Full screen doesn't work as intended on phone, do you want it tested on a TV for other bugs?

@opusforlife2
Copy link
Collaborator

@bazipn This PR is very WIP right now. If you want to help test on TV, you're welcome to, but I would wait until the large list I've posted above gets solved, since some of those would be common to all form factors.

If you don't mind testing anyway, please go ahead!

@Stypox
Copy link
Member Author

Stypox commented Apr 5, 2023

Closing as it doesn't work well and it may cause even more strange bugs

@Stypox Stypox closed this Apr 5, 2023
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.

Refactor how the player is started or changed
5 participants