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

Add AudioStreamErrorCallback for better control over disconnect handling #917

Closed
philburk opened this issue Jun 30, 2020 · 4 comments · Fixed by #1075
Closed

Add AudioStreamErrorCallback for better control over disconnect handling #917

philburk opened this issue Jun 30, 2020 · 4 comments · Fixed by #1075
Assignees
Labels
enhancement P1 high priority
Milestone

Comments

@philburk
Copy link
Collaborator

The current design of the AudioStreamCallback with onErrorBeforeCLose() and onErrorAfterClose() is a problem.
The stop and close is done inside Oboe in a thread spawned by the AAudio callback function.
The stop and close cannot be locked by the developer and may collide with similar actions taken by the app.
For example, when unplugging headphones, the error callback may be called causing a stop and close and a reopening of the stream.
At the same time the device menu may change causing the same activity. This was happening in Hello-Oboe and was causing a crash.

Perhaps we should add a separate error callback that has a single method. Oboe will not spawn a thread for this. Then the developer can lock around the stop/close and completely control the error response with locks, etc. They could also signal their own audio management thread.

class AudioStreamErrorCallback {
virtual void onError(AudioStream* /* oboeStream /, Result / error */);
};

Then in the builder we could add:

AudioStreamBuilder *setErrorCallback(AudioStreamErrorCallback *streamErrorCallback);

In Oboe, if the new error callback is set then Oboe will call it and let the app handle the error completely.
If they do not set it then we do the old behavior. and stop/close the stream for them automatically in a separate thread.
This can be backwards compatible.

@rpattabi
Copy link
Contributor

rpattabi commented Jul 1, 2020

Interesting find. Looking forward to the fix. Small suggestion/query.

I understand that currently oboe doesn't "know" bluetooth/headphone connectivity (BHC) and it just informs us of "error". App has to handle it assuming this could be BHC that caused this error (or additional code is needed to get this confirmation (java) and communicate it to c++ code dealing with oboe).

I wonder if it is oboe's responsibility to identify this and give us a meaningful event instead of just informing as an error? This way app code can easily differentiate BHC and other errors. I don't know if there is a way for oboe to do this though.

@philburk philburk added this to the v1.5 milestone Sep 15, 2020
@philburk philburk added the P1 high priority label Oct 14, 2020
@dturner dturner modified the milestones: v1.5, future Oct 14, 2020
@philburk
Copy link
Collaborator Author

@rpattabi - The Bluetooth connectivity information is not available at the native level.

Generally speaking, the only time this will be called is if there is a disconnect or the audio server dies.
In both cases the error callback should stop and close the stream.
It will probably also want to reopen the stream.

@rpattabi
Copy link
Contributor

Thanks for the clarification. We're restarting the stream when the error is ErrorDisconnected as suggested in disconnect.md. For audio-server-died case, do we get a different error?

@philburk
Copy link
Collaborator Author

philburk commented Nov 4, 2020

We're restarting the stream when the error is ErrorDisconnected

You may get different errors. I recommend ignoring the error code and just trying to restart the stream in all cases.

philburk added a commit that referenced this issue Nov 12, 2020
This allows an app to use different callbacks for data processing
and error processing. The AudioStreamCallback inherits
from both interfaces.

There is also a new method: bool onError()
that allows an app to completely override the default
error handling.

This change is backwards compatible with older versions.

Fixes #917
philburk added a commit that referenced this issue Nov 18, 2020
This allows an app to use different callbacks for data processing
and error processing. The AudioStreamCallback inherits
from both interfaces.

There is also a new method: bool onError()
that allows an app to completely override the default
error handling.

This change is backwards compatible with older versions.

Fixes #917
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement P1 high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants