-
Notifications
You must be signed in to change notification settings - Fork 461
fix(aaudio): Correctly detect minimum buffer size #1039
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
Conversation
…UT_FRAMES_PER_BUFFER on the AudioManager not using AudioTrack
roderickvd
left a comment
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.
Thanks for the contribution!
Thanks for the review - will address all feedback in the morning. The Q around input is an interesting one, I couldn't see anything different for input in their docs but also I agree the property is named after output so... |
|
This works well now - longer term I think we should look at a few feature requests:
|
This is a squashed version of RustAudio#1039, 2b19dd8
This is a squashed version of RustAudio#1039, 2b19dd8
roderickvd
left a comment
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.
Thanks for addressing the earlier comments. Here's a couple of what should be the last nits.
|
|
||
| frames_per_buffer_string | ||
| .parse::<i32>() | ||
| .map_err(|e| jni::errors::Error::JniCall(jni::errors::JniError::Unknown)) |
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.
Would it make sense to retain the error message and return something like jni::errors::Error::ParseError(format!("Failed to parse frames per buffer: {}", e))?
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've fixed this, the master version of jni is nicer, to do this with the current supported version I have had to add combine as a dependency which I don't love. Let me know what you think? I can't see a great solution here for passing the error back through.
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.
Agreed, if we can prevent pulling in another dependency then let's put in that effort. How about JniCall(JniError::Other(String))?
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.
JniError::Other(jint) is the signature so it won't accept a string.
We only really have ParseFailed with the additional dependency or FieldAlreadySet(String)
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.
@roderickvd The crate jni will get released at some point which will allow us to use ParseFailed(String) without an external dependency. In the meantime I think we have three choices:
- Pull in combine dependency and use ParseFailed
- Use FieldAlreadySet(String)
- Add a TODO to update to ParseFailed(String) on jni release and just return the error without the string.
I'm leaning towards 3 or 2 (also with a TODO).
What's your preference? I can then implement this - I'd love to get this across the line with the next release so we don't have to run a patch for cpal.
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've updated this and gone with 3 - this feels the best in my mind, let me know if you want me to change.
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.
Makes sense. Let's not spend too much time over it. If you'd want to track it and deal with it later?
Yes. #964 also added querying latency - we could go from there.
That's interesting for mobile platforms. I believe iOS does too?
You know why Oboe was removed in the first place? (I don't.) |
Agreed. Documeted as an issue / feature request - #1042
This could work as functions on the Stream trait? Although I'm not sure if this is universally supported functionality across platforms.
Pros/cons where detailed here - #961 |
|
@roderickvd addressed the nits - let me know your feedback on the last one. |
We'd have to map out the API and judge how it'd impact the various hosts. Preferably, it'd keep things simple. The ALSA host would also be a candidate, because we can play the same game: requesting a large buffer with X periods but only using the first Y periods of it, and dynamically tuning Y. But, to the point of keeping things simple, we'd have to play around to see how easy it is to do that in the poll cycle.
Ah, thanks, could've looked for that myself 😊 That makes it seem like there's almost no cons and all pros, yet you refer to something that Oboe does by itself and NDK doesn't? If we can implement dynamic buffers without the weight of Oboe, that'd also be nice, no?
👍 I'll take a look. |
| const CHANNEL_CONFIGS: [(i32, u16); 2] = [(CHANNEL_OUT_MONO, 1), (CHANNEL_OUT_STEREO, 2)]; | ||
|
|
||
| const SAMPLE_RATES: [i32; 13] = [ | ||
| 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000, 176_400, 192_000, |
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 know this isn't yours, but now I read it I think that it should also support 12 and 24 kHz.
|
Thanks for your PR and efforts. I'm gonna merge this now, and add the 12/24 kHz after manually. Let me know if I'm wrong. |
AAudio buffer size minimum should be detected using the PROPERTY_OUTPUT_FRAMES_PER_BUFFER on the AudioManager not using AudioTrack.
Android documentation reference: https://developer.android.com/ndk/guides/audio/audio-latency#buffer-size
This allows for a sensible buffer size minimum to be used in comparison to the 1 second+ buffer sizes reported by AudioTrack.
Tested on a Samsung Galaxy Tab A9:
SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(5512), max_sample_rate: SampleRate(5512), buffer_size: Range { min: 256, max: 2147483647 }, sample_format: I16 }Closes: #890