Skip to content

fix(aaudio): Wait for AAudioStream status change#1125

Open
osoftware wants to merge 1 commit intoRustAudio:masterfrom
osoftware:master
Open

fix(aaudio): Wait for AAudioStream status change#1125
osoftware wants to merge 1 commit intoRustAudio:masterfrom
osoftware:master

Conversation

@osoftware
Copy link

This PR fixes two bugs at once.

After starting the stream and before the status changes to Started, dropping it causes the main thread to lock permanently. The underlying AAudioStream never closes while the drop method waits for it to close. By using wait_for_state_change method we ensure the play method won't return befor it's safe to drop the stream.

If wait_for_state_change is never called then AAudioStream_getTimestamp always returns InvalidState error leading to info.timestamp().capture and info.timestamp().playback in stream callbacks always returning zero.

Copy link
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. A big PR was just merged so if you would be so kind to rebase and look at the review comments below? And add a changelog entry?


stream.request_start().map_err(PlayStreamError::from)?;
stream
.wait_for_state_change(ndk::audio::AudioStreamState::Starting, 300_000_000)
Copy link
Member

Choose a reason for hiding this comment

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

Not an expert in AAudio but isn't Oboe's kDefaultTimeoutNanos equal to 2_000_000_000? Would that be worth using and documenting? If there is a reason why 300_000_000 would be more appropriate, that magic number would also be deserving of a comment.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of this const, I'll try it out.

}

impl StreamTrait for Stream {
fn play(&self) -> Result<(), PlayStreamError> {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't same also apply to pause()?

Copy link
Author

Choose a reason for hiding this comment

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

I might makes sense to use wait_for_state_change consistently in both but not using it on pause doesn't yield any serious issues and the method returns faster which might be preferable. IDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants