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

Oboe may need handle the case that openStream failed too many times #1541

Closed
xzhan96 opened this issue May 20, 2022 · 16 comments · Fixed by #1550
Closed

Oboe may need handle the case that openStream failed too many times #1541

xzhan96 opened this issue May 20, 2022 · 16 comments · Fixed by #1550
Assignees
Labels
bug P1 high priority

Comments

@xzhan96
Copy link

xzhan96 commented May 20, 2022

Android version(s):
Android device(s):
Oboe version: latest
App name used for testing:
(Please try to reproduce the issue using the OboeTester or an Oboe sample.)

Short description

Steps to reproduce

  1. disallow cellphone's microphone permission for oboe app
  2. try openStream many times
  3. then give permission for oboe app
  4. openStream maybe failed

By investigate the oboe source code, I think here may have issue, could you help double check?

image

@xzhan96 xzhan96 added the bug label May 20, 2022
@robertwu1
Copy link
Collaborator

Can you try to reproduce the bug with OboeTester?

OboeTester adds logic to handle this case. If OboeTester works, can you copy the logic over to your code?

@robertwu1
Copy link
Collaborator

I don't think calling close() where you suggested is a good idea. If the stream fails to open, we should aim to not have any resources open for the stream.

Perhaps we don't clean up correctly if open fails

@philburk
Copy link
Collaborator

I tried to reproduce this using OboeTester. I prevented TEST INPUT from asking for mic permission.
I could not repro the bug.

Perhaps we don't clean up correctly if open fails

That is quite possible.

What puzzles me is that it is not usually the open() that fails when permissions are not given.
It is the requestStart() that fails. That is when AudioFlinger rejects the attempt.

@xzhan96 - What device and Android version are you using? (You did not fill out the template so I have to ask.)

What error code did the open() return?

@xzhan96
Copy link
Author

xzhan96 commented May 23, 2022

@philburk @robertwu1 thanks for your answer, and sorry for the unclear information. Let me clarify more details:

Device:
Xiaomi Mi 9 with Android 10, and Samsung s21 with Android 11. But I think I can reproduce this issue on any device with our app.

Background:
Our SDK is for realtime communication, like WebRTC. On android we use Oboe as audio pipeline, and we will try restart when open oboe stream failed(as SDK, we can't assume or ensure customer can handle permission logic correctly, so we can't deal with that like OboeTester here).

Every time restart, we will call "oboe::AudioStreamBuilder::openStream", but if open failed, such as no permission, we can't get a valid AudioStream, so we can do nothing.

How to reproduce on our app and sdk:

  • Give app Mic permission
  • running and enter a RTC room do audio capture
  • Disallow the mic permission of app
  • wait for about 30 second, we can see many below errors during this period:

image

  • Then give the mic permission to app again.
  • you will still see above logs in the next time, may continue several minutes, even more.

My analysis:

  • I tried OboeTester, and can't reproduce even delete the logic,I think it because OboeTester will not create too many objects
  • It seems other body also met such issue before, and the solution is to destroy object when create failed.
  • see the code here:

image

If createAudioRecorder fails, external user( I mean SDK developer like me) can not handle the error, so I also can't call below code to clean up the resources?

image

- So I still think Oboe need handle such case?

Thanks a lot for you kindly help.

@xzhan96 xzhan96 removed their assignment May 23, 2022
@xzhan96 xzhan96 changed the title openStream will be failed when no microphone permission Oboe may need handle the case that openStream failed too many times May 23, 2022
@xzhan96
Copy link
Author

xzhan96 commented May 23, 2022

Addition info:
We can solve the issue by calling close here:
image

@xzhan96
Copy link
Author

xzhan96 commented May 24, 2022

@philburk @robertwu1 please check if the information is enough, thanks a lot.

@robertwu1
Copy link
Collaborator

Can you help us collect a bug report? We want to investigate why the open() opens halfway.

@philburk note that they seem to be using OpenSL ES for some reason

@robertwu1
Copy link
Collaborator

Is your app on the play store?

@xzhan96
Copy link
Author

xzhan96 commented May 25, 2022

Is your app on the play store?

https://intl.cloud.tencent.com/document/product/647/34615
image

you can download sdk and try the android project in zip, you need register an account firstly, but this step should be quickly. please see this document:
image

@robertwu1
Copy link
Collaborator

robertwu1 commented May 25, 2022

EngineOpenSLES::open() seems to call close already if open fails. However, AudioOutputStreamOpenSLES::open() doesn't seem to have the same logic.

On the bottom of AudioOutputStreamOpenSLES::open(), can you try adding an "AudioStreamOpenSLES::close()"? Same for "AudioInputStreamOpenSLES::open()"

Result AudioOutputStreamOpenSLES::open() {
    ...
error:
    AudioStreamOpenSLES::close();
    return Result::ErrorInternal; // TODO convert error from SLES to OBOE
}

Thanks!

@philburk philburk self-assigned this May 26, 2022
@philburk
Copy link
Collaborator

I was able to reproduce this by Modifying OboeTester.
I turned off the code in OboeTester that checks for RECORD permission for TEST INPUT.
I modified OboeTester so that if the open fails then it keeps trying 40 times to open the input stream.

Result result = Result::ErrorClosed;
int tryCount = 0;
while (result != Result::OK && tryCount++ < 40) {
    result = builder.openStream(oboeStream);
    LOGD("%s() - open result = %d, errCount = %d", __func__,
         result, tryCount );
}

I long-pressed OboeTester and used "App Info" to turn off the RECORD permission.
I launched OboeTester and ran TEST INPUT.
I selected AudioApi:OpenSL ES
I pressed Open and looked in the logs. The first 31 open attempts looked like:

I OboeAudio: AudioStreamOpenSLES::open() chans=0, rate=0
E AudioPolicyIntefaceImpl: getInputForAttr permission denied: recording not allowed ....
E OboeAudio: Realize recorder object result:SL_RESULT_CONTENT_UNSUPPORTED
D OboeAudio: open() - open result = -896, errCount = 31

Starting with #32 they failures looked like:

I OboeAudio: AudioStreamOpenSLES::open() chans=0, rate=0
E libOpenSLES: Too many objects
W libOpenSLES: Leaving Engine::CreateAudioRecorder (SL_RESULT_MEMORY_FAILURE)
 E OboeAudio: createAudioRecorder() result:SL_RESULT_MEMORY_FAILURE
D OboeAudio: open() - open result = -896, errCount = 32

The "Too many objects" error seems related to:
#define MAX_INSTANCE 32 // maximum active objects per engine, see mInstanceMask
in frameworks/wilhelm/src/itfstruct.h

The open is failing at:
result = (*mObjectInterface)->Realize(mObjectInterface, SL_BOOLEAN_FALSE);

By adding close() to the error section of AudioInputStreamOpenSLES::open() I was able to prevent the "Too many objects" error. The close() method calls:

(*mObjectInterface)->Destroy(mObjectInterface);
EngineOpenSLES::getInstance().close();

I am preparing a PR that will fix this.

Note that there is also a problem in the app. It should check for RECORD permission before trying to open the stream.

@philburk
Copy link
Collaborator

I also noticed that before I made the fix, once I got the "Too many objects" error, that I could never open the stream even if I gave the app RECORD permissions.

After the fix I could open the stream when RECORD permission was restored.

philburk added a commit that referenced this issue May 26, 2022
There were resource leaks if the open failed.
So now it calls close() internally if open fails.

Fixes #1541
@philburk philburk added the P1 high priority label May 26, 2022
@xzhan96
Copy link
Author

xzhan96 commented May 26, 2022

@philburk Does aaudio also has this issue?

@philburk
Copy link
Collaborator

@xzhan96 wrote:

Does aaudio also has this issue?

I don't think so.

  1. In AAudio, the open() does not fail from permission. The start() does.
  2. If the open does fail then there are calls to close and cleanup.

philburk added a commit that referenced this issue May 26, 2022
There were resource leaks if the open failed.
So now it calls close() internally if open fails.

Fixes #1541
@philburk
Copy link
Collaborator

@xzhan96 - we merged a possible fix in #1550

Please give it a try and let us know if it helps.

@philburk philburk assigned xzhan96 and unassigned philburk May 26, 2022
@xzhan96
Copy link
Author

xzhan96 commented Aug 31, 2022

@xzhan96 - we merged a possible fix in #1550

Please give it a try and let us know if it helps.

😸 Late feedback, it fixed my issue, thanks a lot

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

Successfully merging a pull request may close this issue.

3 participants