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

AudioStream.Close() binding not calling the derived class implementation #45

Closed
RLMorley opened this issue Jul 3, 2022 · 2 comments
Closed

Comments

@RLMorley
Copy link

RLMorley commented Jul 3, 2022

Hi there!

I have been noticing an issue where the Oboe API starts returning the "InternalError" code after my app has been running for a while.

To debug this I created a simple test that simply opens an output stream, and then closes it. This showed that after 40 iterations exactly, the API would start failing.

I then implemented the same in C++ and did not encounter this problem. I also noticed that I was getting more Logcat debug from AAudio showing that close() was being called that I was not seeing when running the equivalent Rust code. I then omitted the close call from the C++ test and observed the same bad behaviour (40 open max).

I then modified the oboe-sys dependency on my machine to add extra debug and discovered that only the base class implementation of close() was being called by the Rust binding, and not the function override in AudioStreamAAudio.cpp

This leaks resources and eventually leads to the API returning internal errors.

Finally, adding a close() call to a helper function AudioStreamWrapper.cpp (e.g. AudioStream_delete) does call the correct derived class implementation.

Can someone else have a look and verify this?
I can provide a sample project if needed.

Thanks

Rodrigodd added a commit to Rodrigodd/oboe-rs that referenced this issue Aug 26, 2022
This mainly fix issue katyo#41, that causes crashes when a `AudioStream` was
drop. That happen because the `AudioStream` was not closed on Drop, but
was deleted, causing a use-after-free by the not closed `onDataCallback`
thread.

Also, the method `AudioStreamBuilder::open_stream` was using the
deprecated method `openStream(AudioStream*)`, that do not allow deleting
the `AudioStream` safely. Replaced it by
`openStream(shared_ptr<AudioStream>)`.  The deprecated function allowed
a use-after-free by the `onErrorCallback` thread.

Also, as noted by issue katyo#45, the bindings for `AudioStream::close()` was
wrongly bound to the concrete implementation of the base class, instead
of calling the virtual method.

Also note that currently there is no safe way to delete the
`onErrorCallback` of a `AudioStream` in oboe (see
google/oboe#1610), so instead the current
implementation leaks the callback on drop.

Also, remove some unsound `Drop` implementations and replace them by
explicit unsafe delete methods.
@erikas-taroza
Copy link

erikas-taroza commented Dec 2, 2022

@katyo Hello.

Are there any updates regarding this issue? I am using cpal (which uses this crate) to play audio on Android, and I overrode the oboe dependency using the code in this branch. Unfortunatley, this issue persists.

EDIT:
I just tested by changing the dependency in cpal and it works fine. I thought it was overriding but it wasn't. So for now, I will have to fork cpal and update the oboe dependency to use the fixed branch.

Thank you.

katyo pushed a commit that referenced this issue Jan 17, 2023
This mainly fix issue #41, that causes crashes when a `AudioStream` was
drop. That happen because the `AudioStream` was not closed on Drop, but
was deleted, causing a use-after-free by the not closed `onDataCallback`
thread.

Also, the method `AudioStreamBuilder::open_stream` was using the
deprecated method `openStream(AudioStream*)`, that do not allow deleting
the `AudioStream` safely. Replaced it by
`openStream(shared_ptr<AudioStream>)`.  The deprecated function allowed
a use-after-free by the `onErrorCallback` thread.

Also, as noted by issue #45, the bindings for `AudioStream::close()` was
wrongly bound to the concrete implementation of the base class, instead
of calling the virtual method.

Also note that currently there is no safe way to delete the
`onErrorCallback` of a `AudioStream` in oboe (see
google/oboe#1610), so instead the current
implementation leaks the callback on drop.

Also, remove some unsound `Drop` implementations and replace them by
explicit unsafe delete methods.
@erikas-taroza
Copy link

This issue is fixed as of v0.5.0 and should be closed.

@katyo katyo closed this as completed Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants