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

Close audio effect control session properly #6993

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

Redirion
Copy link
Member

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

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

@Redirion
Copy link
Member Author

@Stypox I decided not to set a CONTENT_TYPE like before. Could be debatable if we want to set MOVIE instead of MUSIC (default)

@TobiGr TobiGr added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Aug 27, 2021
@opusforlife2
Copy link
Collaborator

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.

@rawlife56
Copy link

Yep, I can confirm this works perfectly in opening and closing appropriate audio sessions now. External equalizer apps also work well. 👍

@Yowlen
Copy link

Yowlen commented Aug 27, 2021

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.

@Redirion
Copy link
Member Author

@Yowlen
Copy link

Yowlen commented Aug 27, 2021

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.

@Redirion Redirion requested a review from Stypox August 29, 2021 08:34
Copy link
Member

@Stypox Stypox left a 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 :-)

@Redirion Redirion merged commit f629a4d into TeamNewPipe:dev Aug 31, 2021
This was referenced Sep 5, 2021
@Redirion Redirion deleted the closeaudioeffectsession branch September 24, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NewPipe doesn't properly shut down audio streams when playback is finished/interrupted
6 participants