oboe: add blocking read/write #55

philburk · 2018-02-09T01:03:29Z

Use FIFO for OpenSL ES.
This will allow blocking read/write calls on a stream without using a callback.

Use FIFO for OpenSL ES.

mnaganov · 2018-02-09T01:22:39Z

src/opensles/AudioStreamBuffered.cpp

@@ -25,23 +25,18 @@ namespace oboe {
 * AudioStream with a FifoBuffer
 */
 AudioStreamBuffered::AudioStreamBuffered(const AudioStreamBuilder &builder)
-        : AudioStream(builder)
-        , mFifoBuffer(nullptr)


Why not to change the field type to a smart pointer and forget about manual initialization / deletion:

std::unique_ptr<FifoBuffer> mFifoBuffer;

mnaganov · 2018-02-09T01:24:19Z

src/opensles/AudioStreamBuffered.cpp

-        LOGD("AudioStreamBuffered(): new FifoBuffer");
-        // TODO: Fix memory leak here
+        LOGD("AudioStreamBuffered(): new FifoBuffer(bytesPerFrame=%d", getBytesPerFrame());
+        // FIFO is configured with the same format and channels as the stream.
        mFifoBuffer = new FifoBuffer(getBytesPerFrame(), 1024); // TODO size?
        // Create a callback that reads from the FIFO
        mInternalCallback = std::unique_ptr<AudioStreamBufferedCallback>(new AudioStreamBufferedCallback(this));


You can use std::unique_ptr::reset method http://en.cppreference.com/w/cpp/memory/unique_ptr/reset:

mInternalCallback.reset(new AudioStreamBufferedCallback(this))

Thanks. I cleaned up this section quite a bit.

Implment timestamps. Improve buffer capacity calculation. Refactored and simplified buffer callback.

Also move some curly brackets.

philburk · 2018-02-09T22:41:53Z

src/aaudio/AudioStreamAAudio.cpp

+int32_t AudioStreamAAudio::read(void *buffer,
+                                 int32_t numFrames,
+                                 int64_t timeoutNanoseconds)
+{


move to "{" previous line.

Use FIFO for OpenSL ES.

Implment timestamps. Improve buffer capacity calculation. Refactored and simplified buffer callback.

Also move some curly brackets.

…rite Had to rebase on github.

mnaganov · 2018-02-09T23:49:02Z

src/opensles/AudioStreamBuffered.cpp

+                                  int32_t numFrames,
+                                  int64_t timeoutNanoseconds) {
+    if (getDirection() == Direction::Output) {
+        return (int32_t) Result::ErrorUnavailable; // TODO review, better error code?


ErrorInvalidState?

or maybe ErrorUnsupported

I would categorize writing to an input stream or reading from an output stream as an invalid operation, so ErrorInvalidOperation?

I like InvalidOperation. Currently all oboe errors are defined as AAUDIO_ERROR_*. We could add INVALID_OPERATION to aaudio and try keep the numbers in sync.

dturner · 2018-02-12T16:08:07Z

src/fifo/FifoBuffer.cpp

        , mFramesReadCount(0)
        , mFramesUnderrunCount(0)
        , mUnderrunCount(0)
 {
+    assert(bytesPerFrame > 0);


Is there a more elegant way we could handle these two assertions?

Maybe. I added them because this is a constructor and the FIFO cannot work if the values are incorrect. If they are zero then the FIFO seems to work but doesn't. This took a while to debug. I generally don't like asserts but they seem justified here.

Yes agreed, just checking they weren't left in for debugging purposes.

dturner · 2018-02-12T16:09:21Z

src/opensles/AudioStreamBuffered.cpp

+        // FIFO is configured with the same format and channels as the stream.
+        int32_t capacity = getBufferCapacityInFrames();
+        if (capacity == oboe::kUnspecified) {
+            capacity = getFramesPerBurst() * 16; // arbitrary


Change 16 to konstant

dturner · 2018-02-12T16:09:45Z

src/opensles/AudioStreamBuffered.cpp

+    if (getDirection() == oboe::Direction::Output) {
+        // Read from the FIFO and write to audioData
+        framesTransferred = mFifoBuffer->readNow(audioData, numFrames);
+        LOGV("%s() read %d / %d frames from FIFO", __func__, framesTransferred, numFrames);


Logging in callback, remove

dturner · 2018-02-12T16:09:58Z

src/opensles/AudioStreamBuffered.cpp

+    } else {
+        // Read from audioData and write to the FIFO
+        framesTransferred = mFifoBuffer->write(audioData, numFrames); // FIXME writeNow????
+        LOGV("%s() wrote %d / %d frames to FIFO", __func__, framesTransferred, numFrames);


Logging in callback, remove

dturner · 2018-02-12T16:15:31Z

src/opensles/AudioStreamBuffered.cpp

+    return mBackgroundRanAtNanoseconds + nanosPerBuffer + margin;
+}
+
+// TODO: This method should return a tuple of Result,int32_t where the


There's a few instances where returning a tuple would certainly be easier to understand, rather than having:

positive int32_t = some interesting value for the caller
negative int32_t = error which must be cast back to a Result type

Any thoughts?

Right, the current way is more appropriate for C. E.g. for sizes, there is a standard type is GNU C library called 'ssize_t' which either contains a positive size or negative error code.

In C++, I would prefer a dedicated type that can be checked as a boolean in a logical expression, and then either the value or an error code can be extracted from it. Here is a good example: https://www.kdab.com/tuple-pair-cpp-apis/ Look for "class value_or_error" in the article. Note that they use std::variant in the implementation, which is from c++17, but we don't actually need it, can just have two fields instead.

Hmmm. Maybe. The way that value_or_error template override * seems odd. You use *result to get the value and result.error() to get the error.

I implemented this. But I also notice that many oboe functions return a positive value or a negative error. Changing all of them would be disruptive.

dturner · 2018-02-12T16:34:02Z

src/opensles/AudioStreamOpenSLES.cpp

        LOGE("Oboe callback returned %d", result);
-        return SL_RESULT_INTERNAL_ERROR;
+        return SL_RESULT_INTERNAL_ERROR; // TODO How should we stop OpenSL ES.


We could use the SL player interface to set the play state to stopped. E.g.

https://github.com/googlesamples/android-ndk/blob/master/audio-echo/app/src/main/cpp/audio_player.cpp#L235

Additionally, we could set the stream state to stopped.

Can that be called safely from inside an OpenSL callback?

90% sure they can. We should probably add a test for this.

This is a separate issue from the read/write patch.
I filed: #61

Let's finish up this patch.

dturner

Few comments/questions:

Is it worth adding any unit tests for this?
Should we move sample code inside the oboe repo so we can launch simultaneously with sample code? It'd make testing new features (like this) much easier too.
Main thing to fix is the logging in the callback
I ran a few tests using a port of the aaudio-echo sample, results:

Pixel API 25 (OpenSL ES) performance mode NONE: worked great
Pixel 2 API 27 (AAudio), performance mode NONE: quite glitchy
Pixel 2 API 27 (AAudio), performance mode LOW_LATENCY: worked great

Check for negative setBufferSize() Add more asserts.

oboe: add blocking read/write

373e646

Use FIFO for OpenSL ES.

mnaganov reviewed Feb 9, 2018

View reviewed changes

oboe: more blocking read/write

5fa5b88

Implment timestamps. Improve buffer capacity calculation. Refactored and simplified buffer callback.

philburk requested a review from dturner February 9, 2018 22:41

oboe: remove getNextReadTime

362abd9

Also move some curly brackets.

philburk commented Feb 9, 2018

View reviewed changes

Phil Burk and others added 5 commits February 9, 2018 14:55

oboe: add blocking read/write

34217a7

Use FIFO for OpenSL ES.

oboe: more blocking read/write

a5d6637

Implment timestamps. Improve buffer capacity calculation. Refactored and simplified buffer callback.

oboe: remove getNextReadTime

f9fc01d

Also move some curly brackets.

Merge branch 'master' into readwrite

bc0643f

Merge branch 'readwrite' of https://github.com/google/oboe into readw…

ee53861

…rite Had to rebase on github.

mnaganov approved these changes Feb 9, 2018

View reviewed changes

dturner reviewed Feb 12, 2018

View reviewed changes

dturner requested changes Feb 12, 2018

View reviewed changes

Oboe: remove some logs, misc cleanup

477e01c

Check for negative setBufferSize() Add more asserts.

dturner approved these changes Feb 14, 2018

View reviewed changes

philburk merged commit 44c6b6e into master Feb 14, 2018

philburk deleted the readwrite branch February 14, 2018 17:40

philburk mentioned this pull request Mar 24, 2018

Implement AAudioStream read and write methods #36

Closed

Hyyzt mentioned this pull request Aug 18, 2022

the drumthumper have a crash #1608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oboe: add blocking read/write #55

oboe: add blocking read/write #55

philburk commented Feb 9, 2018

mnaganov Feb 9, 2018 •

edited

Loading

philburk Feb 9, 2018

mnaganov Feb 9, 2018 •

edited

Loading

philburk Feb 9, 2018

philburk Feb 9, 2018

mnaganov Feb 9, 2018

philburk Feb 10, 2018

dturner Feb 12, 2018 •

edited

Loading

philburk Feb 12, 2018

dturner Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 13, 2018

dturner Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 12, 2018

mnaganov Feb 12, 2018

philburk Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 12, 2018

philburk Feb 12, 2018

dturner Feb 13, 2018

philburk Feb 14, 2018

dturner left a comment

oboe: add blocking read/write #55

oboe: add blocking read/write #55

Conversation

philburk commented Feb 9, 2018

mnaganov Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaganov Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturner Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturner left a comment

Choose a reason for hiding this comment

mnaganov Feb 9, 2018 •

edited

Loading

mnaganov Feb 9, 2018 •

edited

Loading

dturner Feb 12, 2018 •

edited

Loading