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

Fix crash on Session start on many Android devices #2905

Merged
merged 2 commits into from
Oct 30, 2022

Conversation

danryu
Copy link
Contributor

@danryu danryu commented Oct 11, 2022

This is a simple one-line change that fixes a crash on session startup that occurs on many Android devices.

CHANGELOG: fixes a crash on session startup that occurs on many Android devices

Context: Fixes an issue?
This change fixes a crash that occurs on session startup on many Android devices.
This was discovered in collaboration with Oboe devs (many thanks to them) and is best covered in this Issue comment here:
google/oboe#1571 (comment)

In brief (thanks to @flamme):
When your app was using legacy path and getting fast mode by requesting low latency, given your callback size(128) is smaller than the burst size(144), aaudio would send your app the data buffer from native audio server directly to save one memory copy. The data buffer in this case is read only for the client. In that case, when you call memset ( audioData, 0 /* value */, numBytes );, the crash happened. When you used oboe callback adapter, the data from native audio server would be copied to an intermediate buffer in oboe. In that case, your app would not crash.

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.10.0 milestone Oct 11, 2022
@hoffie
Copy link
Member

hoffie commented Oct 11, 2022

Thanks for upstream'ing this!
I've read the linked oboe thread and the change sounds sane.

It's slightly late for 3.9.1 IMO, so I've assigned it to 3.10.0.

The clang-format style checker has something to complain

@danryu
Copy link
Contributor Author

danryu commented Oct 11, 2022

The clang-format style checker has something to complain

I think that was a whitespace issue, fix now pushed

@pljones
Copy link
Collaborator

pljones commented Oct 11, 2022

It's slightly late for 3.9.1 IMO, so I've assigned it to 3.10.0.

Yep, and I'd want to give it a little bit more time to bed in (given the change). I'm about to announce the rc1 and write the draft release note for 3.9.1.

@ann0see
Copy link
Member

ann0see commented Oct 28, 2022

@hoffie can you test this on an Android device? I can't really (just a VM or WSA)

@hoffie
Copy link
Member

hoffie commented Oct 30, 2022

CHANGELOG: Android: Fixed a crash on session startup that occurs on many devices

Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Artifact from this PR runs as... bad¹ as Jamulus from master on my dated Android device. Reasoning sounds sane, therefore approving.

¹ I've never been able to use Jamulus sanely on my device. Output stutters. I've not been affected ba a crash which this PR aims to fix though.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you're one of the few people who can actually test this. Approving anyway.

@ann0see ann0see merged commit b0c3e74 into jamulussoftware:master Oct 30, 2022
@ann0see
Copy link
Member

ann0see commented Oct 30, 2022

If you haven't been added as contributor yet, please do so in the util.h file.

Thanks for fixing this 😀

@danryu
Copy link
Contributor Author

danryu commented Oct 31, 2022

Thanks @ann0see, I created PR #2950 to add myself as contributor.

(PS the android CI PR #2909 was probably the shortest route to actually testing Jamulus on Android - until the client is on a testing track on the Play Store, it's very difficult to actually get it on potential testers' devices :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants