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

Fixed crashes with AudioFocusRequestManager on Android 7.1 #4341

Closed

Conversation

rapterjet2004
Copy link
Contributor

@rapterjet2004 rapterjet2004 commented Oct 15, 2024

#4338 (comment)

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

@rapterjet2004 rapterjet2004 added the 3. to review Waiting for reviews label Oct 15, 2024
@rapterjet2004 rapterjet2004 self-assigned this Oct 15, 2024
@MmAaXx500
Copy link

Unfortunately, this does not fix the issue, and it seems to me that on versions before android O, the audio focus request got removed.

On my fork, I created a commit that fixes this issue, but didn't have the time to submit a pr. https://github.com/MmAaXx500/talk-android/tree/api25-audiofocus
If you want, I can open a pr, or you can take the code from there.

Signed-off-by: rapterjet2004 <[email protected]>
Copy link
Contributor

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/4341-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

Copy link
Contributor

Codacy

Lint

TypemasterPR
Warnings9494
Errors132132

SpotBugs

CategoryBaseNew
Bad practice66
Correctness1111
Dodgy code7979
Internationalization33
Malicious code vulnerability33
Performance66
Security11
Total109109

@MmAaXx500
Copy link

MmAaXx500 commented Oct 17, 2024

Now there is no crashing, but waits indefinitely on opening a conversation.

If you have no specific reason to remove the requestAudioFocus and abandonAudioFocus then you can drop your first commit (9cacb54) and use my suggested changes only.
Edit: linked the wrong commit

Edit2: now it's working, there was an error on my side. My question about removal of requestAudioFocus and abandonAudioFocus still stands, however.

@sowjanyakch
Copy link
Contributor

sowjanyakch commented Oct 23, 2024

@MmAaXx500 - thank you for testing and mentioning that there is no crash. I could not test this PR because I don't have a device that runs on android 7.1 or below. I referred android documentation, https://developer.android.com/media/optimize/audio-focus#audio-focus-7-1, I see that it's okay to use requestFocus() and abandonFocus(). What is the error that you are getting?

One more suggestion always use safe call with let '?.let' whenever possible instead of '!!' to avoid unnecessary crashes.

Instead of

audioManager.requestAudioFocus(focusRequest!!)

we can use

focusRequest?.let { request ->
audioManager.requestAudioFocus(request)
}

@mahibi
Copy link
Collaborator

mahibi commented Oct 23, 2024

Sorry all, I should have looked directly when the issue came up.
I just calculated the percentage usage of android 7.0 and 7.1 for our user base.

For 7.0 it is 0,3%
For 7.1 it is 0,19%

We have the rule to ditch android version lower than 0,5%.

So instead to introduce more logic here, we should create a PR that removes support for these versions and bump the minSdkVersion to 26 (Android 8.0) which currently has an install base of 0,7%.

Sorry if this is disappointing if you're a user with android version lower than 8, but we are not able to support endless scenarios with the cost of complexity.

@mahibi mahibi self-requested a review October 23, 2024 13:03
@mahibi mahibi closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when opening any conversation on Android 7.1
4 participants