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

Clarification on thread safety #32

Closed
svenoaks opened this issue Jan 1, 2018 · 13 comments
Closed

Clarification on thread safety #32

svenoaks opened this issue Jan 1, 2018 · 13 comments
Assignees
Labels
Milestone

Comments

@svenoaks
Copy link

svenoaks commented Jan 1, 2018

It seems a little confusing to me from the guide what is permissible as far as thread safety.

Note: When a stream uses a callback function, it's safe to read/write from the callback thread while also closing the stream from the thread in which it is running

Does this mean I can be writing data to the audioData void* in the callback while simultaneously calling requestPause(), requestStop(), close() in some other function on another thread?
`

@philburk
Copy link
Collaborator

philburk commented Jan 3, 2018

Thanks for asking. The Thread Safety section is pretty confusing. Some of it is inaccurate. Oboe does use some mutexes for non-time-critical functions like start()/stop().

There are some things you CAN do.

  • You can call AudioStream::stop(), requestStop() , pause() or requestPause() from an application thread while a callback is running in a system thread. That will stop the callback thread.

  • You can call AudioStream::close() from an application thread while a callback is running in a system thread. It will stop the callback before closing or deleting any stream resources.

There are some things you CANNOT do.

  • Never call AudioStream::stop(), requestStop() , pause(), requestPause() or close() from a callback method. Return oboe::DataCallbackResult::Stop if you want the callback to stop.

  • Never call AudioStream::read() or AudioStream::write() on a stream from that stream's own callback method. You should use the audioData array instead. You can read() or write() another stream from a callback as long as that stream does not have an active callback. We recommend that if you have data flowing between multiple streams then only one of them should use a callback.

In general you should only modify the stream from one application thread. From the callback methods try to only use get*() methods.

Please ask more questions. When we have an explanation that makes sense then we will update the docs.

@svenoaks
Copy link
Author

svenoaks commented Jan 5, 2018

Thanks, what about getBufferSizeInFrames() calling which could be concurrent with the LatencyTuner->tune() in the callback?

@philburk
Copy link
Collaborator

philburk commented Jan 5, 2018

You can safely call getBufferSizeInFrames() from as many threads as you want. You may be sampling a changing value. So only use the information for non-critical purposes, such as a fullness display.

Only call setBufferSizeInFrames() from one thread. So if latency tuning is active in the callback, do not call it from the application.

@svenoaks
Copy link
Author

svenoaks commented Jan 5, 2018

Thanks. As far as I understand, getBufferSizeInFrames() is an ever increasing value, then should become stable when it's no longer possible to glitch the audio. I'm saving this value in SharedPreferences for use in next session so it won't have to retune, exposing user to audio glitches again. Is there a recommended setting for bufferSizeInFrames to prevent glitching to start with, I'm finding it often shoots up to 4 or more times framesPerBurst.

@svenoaks
Copy link
Author

svenoaks commented Jan 5, 2018

Thinking aloud and off-topic - wouldn't it be good if LatencyTuner had a mode where it started from the maximum buffer size and worked down for some initial period of time like a minute, stopping lowering when it encounters drop-outs, then worked like it does currently, raising back up. That way user wouldn't be presented with lots of static when first opening an app, which can happen if there is heavy CPU load. I suppose this could be implemented in application too, but more suitable within LatencyTuner.

@philburk
Copy link
Collaborator

philburk commented Jan 5, 2018

As far as I understand, getBufferSizeInFrames() is an ever increasing value, then should
become stable when it's no longer possible to glitch the audio.

Not necessarily. It depends on the latency tuning algorithm. Suppose a glitch was caused by some transient activity. After a while it may be safe to lower the latency again. Some apps might be willing to risk a glitch in order to minimize latency. Some apps will not want to take that risk. This is why we left the tuning out of the OS. Apps can decide.

An app can calculate output buffer fullness as framesWritten minus framesRead.
If the buffer stays relatively full for a long time then an app might decide it is safe to lower the buffer size back down.

I'm saving this value in SharedPreferences for use in next session so it won't have to retune

Excellent. We hoped that apps would do that.

Is there a recommended setting for bufferSizeInFrames to prevent glitching to start
with, I'm finding it often shoots up to 4 or more times framesPerBurst.

The behavior can vary a lot by device.

wouldn't it be good if LatencyTuner had a mode where it started from the maximum buffer size

That could be a good strategy for some apps. But some apps, synthesizers for example, are often silent for a few seconds before the user starts playing. That is a good time for the LatencyTuner to glitch because the glitches are inaudible.

We encourage app developers to customize the LatencyTuner for their own purposes. And please share your experience.

Also please let use know if you post any apps in the Play Store. We can then use it for testing the OS.

@svenoaks
Copy link
Author

svenoaks commented Jan 5, 2018

I was planning on rolling out update https://play.google.com/store/apps/details?id=com.smp.musicspeed with Oboe as the default sound output this weekend. I was a little hesitant because it is developer preview, but it seems to be behaving perfectly so far. My use is simple, only an output stream. I wanted to start using AAudio as soon as possible. Do you think it is relatively safe to use for production? I was thinking the preview aspects of it would be obvious things like unimplemented functions.

@philburk
Copy link
Collaborator

philburk commented Jan 9, 2018

Do you think it is relatively safe to use for production?

Until you filed #40, I would have said yes. We are now investigating that as a high priority.

Let's leave this open until we clarify the thread safety issues in the docs.

@philburk philburk self-assigned this Jan 9, 2018
@dturner
Copy link
Collaborator

dturner commented Jun 15, 2018

Any update here @philburk?

@philburk
Copy link
Collaborator

#40 has been addressed by not using AAudio on 8.0.

We need to add the restrictions and caveats from this discussion to the autodocs for AudioStreamCallback. Then reference those restrictions in AudioStreamBuilder::setCallback() docs. Then we can close this.

@dturner dturner added this to the v1.0 milestone Jun 20, 2018
@dturner
Copy link
Collaborator

dturner commented Jun 20, 2018

AI: Clarify this in the docs

@dturner
Copy link
Collaborator

dturner commented Oct 2, 2018

Phil to update docs. Don to regenerate API reference.

philburk pushed a commit that referenced this issue Oct 5, 2018
For example, do not stop() or write() a stream from the callback.

Issue #32
@philburk philburk modified the milestones: v1.0, future Nov 21, 2018
@philburk philburk modified the milestones: future, v1.3 May 14, 2019
@dturner
Copy link
Collaborator

dturner commented Jun 21, 2019

Docs have been updated, specifically here: file:///Users/donturner/Code/workspace-android/oboe/docs/reference/classoboe_1_1_audio_stream_callback.html

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

No branches or pull requests

4 participants