Conversation
src/hostapi/oboe/pa_oboe.cpp
Outdated
| // FIXME this is leading to a segfault - is the stream still active shortly after closing? | ||
| // Try to wait for state change in vain | ||
| // for (int i = 0; i < paOboe_numberOfBuffers; ++i) { | ||
| // if (paOboeStream->hasOutput) | ||
| // PaUtil_FreeMemory(paOboeStream->outputBuffers[i]); | ||
| // if (paOboeStream->hasInput) | ||
| // PaUtil_FreeMemory(paOboeStream->inputBuffers[i]); | ||
| // } | ||
|
|
||
| // if (paOboeStream->hasOutput) | ||
| // PaUtil_FreeMemory(paOboeStream->outputBuffers); | ||
| // if (paOboeStream->hasInput) | ||
| // PaUtil_FreeMemory(paOboeStream->inputBuffers); | ||
|
|
||
| // PaUtil_FreeMemory(paOboeStream); |
There was a problem hiding this comment.
This remains a blocking issue - Perhaps @philburk you would have an idea of why this approach doesn't currently work and leads to a segfault?
There was a problem hiding this comment.
@acolombier wrote:
is the stream still active shortly after closing?
Ideally no. But these were some system bugs that could cause this.
These have been fixed in the newer versions of Android. But if you are running on older versions of Android then
you may hit these problems. Luckily you can avoid the problems with some careful coding.
This Oboe page has lots of information on this issue:
https://github.com/google/oboe/wiki/TechNote_HowToAvoidCrashes
There is also this related issue:
https://github.com/google/oboe/wiki/CrashUAF_ObtainReleaseBuffer
|
@hopefulGiupplo and @acolombier - thank you both for working on this. We are getting close to finishing 19.8. So I can start looking at this PR in more detail. |
|
@acolombier - Were any changes made to the code between the end of #840 and the squashing into the 1st commit of this PR? |
|
Hi @philburk , yes, here is the summary
|
philburk
left a comment
philburk
left a comment
There was a problem hiding this comment.
Some general thoughts...
Oboe will support almost any configuration that you request. So you may not need to probe for them. Just advertise, for example, mono, stereo, {8000, 16000, 32000, 44100, 48000}, {float, int16, int24}
You can decide whether to use PerformanceMode=LowLatency if the suggested latency from PortAudio is < 80(?) msec.
To avoid crashes, do not delete anything right after close.
Just hang onto the memory in a unique_ptr and only free it if you allocate a new one later.
If the app specifies a sample rate, use the Oboe sample rate converter.
You will get much better latency.
builder->setSampleRateConversionQuality(oboe::SampleRateConversionQuality::Medium)
|
|
||
| 4) Build PaOboe (you can use "build_all_PaOboe.sh"). | ||
| 5) Don't forget to add liboboe.so and libportaudio.so in your jniLibs folder. | ||
|
|
There was a problem hiding this comment.
It would be nice to have instructions for adding this to an Android Studio project.
I think most Android developers are using that.
It might help to add some build.gradle files, which might get imported into Android Studio automatically.
There was a problem hiding this comment.
Unfortunately, I don't use Android Studio and don't have any experience with it - I use PortAudio+oboe in Qt, using a CMake setup.
WOuld you be able to share some basic build.gradle?
| * @param direction The device direction | ||
| * @param channelCounts The device channel count | ||
| * @param sampleRate The device default sampleRate | ||
| */ |
There was a problem hiding this comment.
It seems like we would also need a deviceId so that we can select the device using builder.setDeviceId(id). Maybe I am missing something.
Did you convert spaces to tabs OR tabs to spaces?! I hope the latter.
That is up to you. I don't know how hard it would be. And we would lose this partial review if we started a new PR. I am fine either way. I can just focus on the current state of the code and move forward. |
acolombier
force-pushed
the
feat/add-oboe-host-api
branch
from
3002310 to
2e3aad8
Compare
|
Thanks for all this input and the initial review. I have pushed a new head as #, with 3 commits:
Let me know if this is better to review, I can revert otherwise.
I just reverted to what was used, in order to limit the diff - initial version add 310 line changes, including many that were unrelated to oboe and seem to be random style updates (capitalisation number of spaces/tabs, ..). Regarding the used style, I try to imitate as much what I could see in other places of PortAudio, but feel free to point inconsistency and I will update. I will start addressing your feedback asap! |
|
I have pushed the first batch of changes, including some DRY cleanup and better concurrency/allocation handling. I still need to test that! |
acolombier
force-pushed
the
feat/add-oboe-host-api
branch
from
3671f42 to
8508b45
Compare
|
Tested and could not reproduce any crash! |
3 participants
This superseded #840
Note that this patch was previously on top of b0cc303 (ref) but will need to be re-tested on top of on master, once we have finish upgraded our vcpkg to the latest versions.