onAudioReady() is called simultaneously from two threads on OpenSL ES backend

[Devenec]

Android version(s): 10 and 11
Android device(s): Multiple, see below
Oboe version: 1.6.1
App name used for testing: -

Short description
When calling requestStart() for an OpenSL ES output stream, onAudioReady() is called from the thread that called requestStart(), and simultaneously from an AudioTrack thread.

Steps to reproduce
The issue may be hard to reproduce. It occurs in our game within 5 minutes of starting the game.

Call requestStart() for an OpenSL ES output stream, and observe that onAudioReady() is called from two threads simultaneously.

Expected behavior
onAudioReady() is not called from multiple threads simultaneously.

Actual behavior
onAudioReady() is called from two threads simultaneously.

Device

• Nokia 9 PureView (Android 10)
• Samsung Galaxy A12 (Android 11)

Any additional context
Stack trace of the thread that called requestStart():

> libNativeGame.so!OboeChannelCallback::onAudioReady(oboe::AudioStream*, void*, int)(OboeChannelCallback * this, oboe::AudioStream * stream, void * buffer, int32_t frame_count) Line 37	c++14
  liboboe.so!non-virtual thunk to oboe::AudioSourceCaller::onProcessFixedBlock(unsigned char*, int)()	unknown
  liboboe.so!FixedBlockReader::read(unsigned char*, int)()	unknown
  liboboe.so!oboe::SourceFloatCaller::onProcess(int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphPortFloatOutput::pullData(long, int)()	unknown
  liboboe.so!oboe::flowgraph::SampleRateConverter::onProcess(int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphPortFloatOutput::pullData(long, int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphSink::pullData(int)()	unknown
  liboboe.so!oboe::flowgraph::SinkFloat::read(void*, int)()	unknown
  liboboe.so!non-virtual thunk to oboe::FilterAudioStream::onAudioReady(oboe::AudioStream*, void*, int)()	unknown
  liboboe.so!oboe::AudioStream::fireDataCallback(void*, int)()	unknown
  liboboe.so!oboe::AudioStreamOpenSLES::processBufferCallback(SLAndroidSimpleBufferQueueItf_ const* const*)()	unknown
  liboboe.so!oboe::AudioOutputStreamOpenSLES::requestStart()()	unknown

Stack trace of AudioTrack thread:

> libNativeGame.so!OboeChannelCallback::onAudioReady(oboe::AudioStream*, void*, int)(OboeChannelCallback * this, oboe::AudioStream * stream, void * buffer, int32_t frame_count) Line 37	c++14
  liboboe.so!non-virtual thunk to oboe::AudioSourceCaller::onProcessFixedBlock(unsigned char*, int)()	unknown
  liboboe.so!FixedBlockReader::read(unsigned char*, int)()	unknown
  liboboe.so!oboe::SourceFloatCaller::onProcess(int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphPortFloatOutput::pullData(long, int)()	unknown
  liboboe.so!oboe::flowgraph::SampleRateConverter::onProcess(int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphPortFloatOutput::pullData(long, int)()	unknown
  liboboe.so!oboe::flowgraph::FlowGraphSink::pullData(int)()	unknown
  liboboe.so!oboe::flowgraph::SinkFloat::read(void*, int)()	unknown
  liboboe.so!non-virtual thunk to oboe::FilterAudioStream::onAudioReady(oboe::AudioStream*, void*, int)()	unknown
  liboboe.so!oboe::AudioStream::fireDataCallback(void*, int)()	unknown
  liboboe.so!oboe::AudioStreamOpenSLES::processBufferCallback(SLAndroidSimpleBufferQueueItf_ const* const*)()	unknown
  libwilhelm.so!audioTrack_callBack_pullFromBuffQueue(int, void*, void*)()	unknown
  libaudioclient.so!android::AudioTrack::processAudioBuffer()()	unknown
  libaudioclient.so!android::AudioTrack::AudioTrackThread::threadLoop()()	unknown
  libutils.so!android::Thread::_threadLoop(void*)()	unknown
  libandroid_runtime.so!android::AndroidRuntime::javaThreadShell(void*)()	unknown
  libutils.so!thread_data_t::trampoline(thread_data_t const*)()	unknown
  libc.so!__pthread_start(void*)()	unknown
  libc.so!__start_thread()	unknown

[Devenec]

It seems that calling processBufferCallback() in AudioOutputStreamOpenSLES::requestStart() before calling setPlayState_l(SL_PLAYSTATE_PLAYING) fixes the issue.

From:

    setState(StreamState::Starting);
    Result result = setPlayState_l(SL_PLAYSTATE_PLAYING);
    if (result == Result::OK) {
        setState(StreamState::Started);
        mLock.unlock();
        if (getBufferDepth(mSimpleBufferQueueInterface) == 0) {
            // Enqueue the first buffer if needed to start the streaming.
            // This might call requestStop() so try to avoid a recursive lock.
            processBufferCallback(mSimpleBufferQueueInterface);
        }
    } else {

to:

    setState(StreamState::Starting);
    mLock.unlock();

    if (getBufferDepth(mSimpleBufferQueueInterface) == 0) {
        // Enqueue the first buffer if needed to start the streaming.
        // This might call requestStop() so try to avoid a recursive lock.
        processBufferCallback(mSimpleBufferQueueInterface);
    }

    mLock.lock();
    Result result = setPlayState_l(SL_PLAYSTATE_PLAYING);
    if (result == Result::OK) {
        setState(StreamState::Started);
        mLock.unlock();
    } else {

Could this change cause any issues?

[robertwu1]

Thanks for finding this issue!

Moving processBufferCallback up seems like the right thing to do. However, I don't think we should release the lock. I opened #1570 to change how processBufferCallback instead.

Can you help verify whether my set of changes works for you?

[philburk]

Great! I think there must be a race condition if there is something in the queue when setting the PLAYSTATE. It is possible the callbacks start immediately and drain the queue and then the processBufferCallback gets called.

If we call getBufferDepth() BEFORE we call setPlayState_l(SL_PLAYSTATE_PLAYING)
then we can avoid the race.

[robertwu1]

Per Phil's suggestion, the most recent version of #1570 saves the earlier buffer depth instead. @Devenec can you verify whether this works for you?

    setState(StreamState::Starting);
    int bufferDepthBeforeSettingPlayState = getBufferDepth(mSimpleBufferQueueInterface);
    Result result = setPlayState_l(SL_PLAYSTATE_PLAYING);
robertwu1 marked this conversation as resolved.
Show resolved
    if (result == Result::OK) {
        setState(StreamState::Started);
        mLock.unlock();
        if (bufferDepthBeforeSettingPlayState == 0) {
            // Enqueue the first buffer if needed to start the streaming.
            // This might call requestStop() so try to avoid a recursive lock.
            processBufferCallback(mSimpleBufferQueueInterface);

[Devenec]

Per Phil's suggestion, the most recent version of #1570 saves the earlier buffer depth instead. @Devenec can you verify whether this works for you?

    setState(StreamState::Starting);
    int bufferDepthBeforeSettingPlayState = getBufferDepth(mSimpleBufferQueueInterface);
    Result result = setPlayState_l(SL_PLAYSTATE_PLAYING);
robertwu1 marked this conversation as resolved.
Show resolved
    if (result == Result::OK) {
        setState(StreamState::Started);
        mLock.unlock();
        if (bufferDepthBeforeSettingPlayState == 0) {
            // Enqueue the first buffer if needed to start the streaming.
            // This might call requestStop() so try to avoid a recursive lock.
            processBufferCallback(mSimpleBufferQueueInterface);

This doesn't seem to work.

However the changes in the first commit of #1570 work, after removing the lock from AudioOutputStreamOpenSLES::requestStop_l() and adding conditional requestStop() call to AudioStreamOpenSLES::bqCallbackGlue().

I'm leaving for summer holiday today and will be back in August. I'll go with the first commit for now.

[robertwu1]

Thanks for the feedback! I reverted the PR back to the first commit and made the necessary fixes to it.

Have a great vacation!