-
-
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
Close audio effect control session properly #6993
Conversation
@Stypox I decided not to set a CONTENT_TYPE like before. Could be debatable if we want to set MOVIE instead of MUSIC (default) |
Thank you for the fix! This seems like a basic problem of forgetting to free memory. Is there no tool to test for bugs such as this at compile time or run time? @Yowlen please test and confirm if this fixes the issue for you. |
Yep, I can confirm this works perfectly in opening and closing appropriate audio sessions now. External equalizer apps also work well. 👍 |
GitHub is being a pain on mobile since very few of their pages actually have mobile versions and I've never used that Checks tab before, so I have no idea how to navigate it even with the instructions given. I got as far as tapping CI before I got lost and the slow loading times are making me extremely more frustrated than I need to be. |
Thank you. The equalizer app I was using to test this doesn't want to work on my new phone so I had to power up my old one to test and it just added to the frustration having to deal with stuff. Anyway, I can confirm it does close out the stream when it's done now. Thank you for putting up with me there, and for the hard work all of you do at making such a great app. |
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.
I also tested a little, and everything seems fine. The mute button also still works as expected, among other things. Thank you for taking some time to implement this @Redirion :-)
What is it?
Description of the changes in your PR
NewPipe would announce the audioSessionId once it is created for the ExoPlayer instance or when Audio gets enabled again after being disabled. However it did never announce closure of an audioSessionId. This is now done properly.
Apparently the missing closure of the audio effect control session could lead to memory leaks and cause problems with DSP / AudioFX / Equalizer apps. See also discussion in #2373
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