New namespace, constexprs and scoped enums #18

dturner · 2017-11-27T21:40:06Z

!!!BREAKING CHANGE!!!

Summary

New oboe:: namespace
Macro constants replaced with constexpr and scoped enums
Oboe_convert*Text methods replaced with single convertToText method

Detailed changelog

1) `oboe` namespace used for all classes and functions

Why?
Having a namespace provides better encapsulation, avoids polluting the global namespace and also avoids having to prefix all Oboe classes and methods with the word "Oboe".

What code changes do I need to make?
If you're using any classes which start with "Oboe" you'll need to change them to use oboe::. For example:

OboeStreamBuilder builder;

becomes

oboe::AudioStreamBuilder builder;

or you can use the oboe namespace without qualification:

using namespace oboe;
AudioStreamBuilder builder;

2) Constants defined by macros are replaced by `constexpr` and scoped enums

Why?
C++ Core Guidelines ES.31: Don’t use macros for constants or “functions” http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-macros2.
Effective Modern C++ (Scott Meyer) Item 15 - Use constexpr wherever possible.
Scoped enums allows better type checking at compile time and eliminate the need for long #defines in the global namespace.

What code changes do I need to make?
Anywhere you were using a value starting with OBOE_ you'll need to change it either to the corresponding value starting with oboe::k. For example OBOE_NANOS_PER_MICROSECOND becomes oboe::kNanosPerMicrosecond. Or you'll need to change it to the corresponding scoped enum:

OBOE_AUDIO_FORMAT_PCM_FLOAT becomes oboe::AudioFormat::Float

3) `Oboe_convert*Text` methods replaced with single `convertToText` method. **

Why?
A single method is easier to remember and to use.

What code changes do I need to make?
If you are using a Oboe_convert*Text method then you'll need to change it to use convertToText. For example:

Oboe_convertResultToText(result)

becomes

oboe::convertToText(result)

dturner · 2017-11-27T22:05:14Z

GettingStarted.md

@@ -48,24 +48,24 @@ Include the Oboe header:

    #include <oboe/Oboe.h>

-Streams are built using an `OboeStreamBuilder`. Create one like this:
+Streams are built using an `StreamBuilder`. Create one like this:


Deferring doing the an/a changes until we have made a decision on Stream vs AudioStream naming

dturner · 2017-11-27T22:05:31Z

GettingStarted.md


-Define an `OboeStreamCallback` class to receive callbacks whenever the stream requires new data.
+Define an `StreamCallback` class to receive callbacks whenever the stream requires new data.


dturner · 2017-11-27T22:06:00Z

GettingStarted.md

-    OboeStream *stream;
-    oboe_result_t result = builder.openStream(&stream);
+    oboe::Stream *stream;
+    oboe::Result result = builder.openStream(&stream);

 Check the result to make sure the stream was opened successfully. Oboe has many convenience methods for converting its types into human-readable strings, they all start with `Oboe_convert`:


all start with oboe::convert

dturner · 2017-11-27T22:10:11Z

include/oboe/Stream.h

@@ -177,16 +175,16 @@ class OboeStream : public OboeStreamBase {
     * @param timeoutNanoseconds Maximum number of nanoseconds to wait for completion.
     * @return The number of frames actually written or a negative error.
     */
-    virtual oboe_result_t write(const void *buffer,
+    virtual int32_t write(const void *buffer,


IMO this method would be better off returning a std::tuple of Result and int32_t. This avoids the return type representing both the frames written (if positive) or an error (if negative).

dturner · 2017-11-27T22:11:59Z

include/oboe/Stream.h


 /**
 * Base class for Oboe C++ audio stream.
 */
-class OboeStream : public OboeStreamBase {
+class Stream : public StreamBase {


We might want to consider calling this AudioStream since Stream is quite a generic term. At some point in the future we might also have a MidiStream

dturner · 2017-11-27T22:13:47Z

src/common/LatencyTuner.cpp

            if (--mIdleCountDown <= 0) {
-                mState = STATE_ACTIVE;
+                mState = State::Active;
            }
            mPreviousXRuns = mStream.getXRunCount();


getXRunCount might also be better returning a std::tuple

philburk

Nice!

philburk · 2017-11-27T22:05:12Z

GettingStarted.md

@@ -48,24 +48,24 @@ Include the Oboe header:

    #include <oboe/Oboe.h>

-Streams are built using an `OboeStreamBuilder`. Create one like this:
+Streams are built using an `StreamBuilder`. Create one like this:


Are the class names Stream and StreamBuilder too generic? Is there a danger of colliding if developers are using multiple namespaces?

There would only be a danger if they're using non-qualified namespaces e.g.

using namespace oboe; Stream stream;

But yes, I do think Stream is too generic. AudioStream, AudioStreamBuilder etc would be more readable. Also if we ever support MIDI we could have a MidiStream. If you agree I'll rename.

philburk · 2017-11-27T22:07:22Z

GettingStarted.md

-    OboeStream *stream;
-    oboe_result_t result = builder.openStream(&stream);
+    oboe::Stream *stream;
+    oboe::Result result = builder.openStream(&stream);

 Check the result to make sure the stream was opened successfully. Oboe has many convenience methods for converting its types into human-readable strings, they all start with `Oboe_convert`:


they now start with "oboe::convert"

philburk · 2017-11-27T22:15:04Z

include/oboe/StreamBuilder.h

@@ -171,7 +174,7 @@ class OboeStreamBuilder : public OboeStreamBase {
     * @param performanceMode for example, OBOE_PERFORMANCE_MODE_LOW_LATENCY


philburk · 2017-11-27T22:15:14Z

include/oboe/StreamBuilder.h

@@ -185,7 +188,7 @@ class OboeStreamBuilder : public OboeStreamBase {
     * @param deviceId device identifier or OBOE_DEVICE_UNSPECIFIED


philburk · 2017-11-27T22:24:02Z

src/common/Utilities.cpp

+    return size;
+}
+
+const char *convertResultToText(Result returnCode) {


If running with a future version AAudio, there may be new results or new formats. So we should add a default "Unrecognized" return value.

Done for all convert methods

mnaganov · 2017-11-27T23:38:39Z

include/oboe/Definitions.h

+
+    constexpr int32_t kUnspecified = 0;
+
+    constexpr int64_t kNanosPerMicrosecond =    1000;


Please add a TODO to consider using std::chrono, as it provides a lot of time unit conversion utilities.

mnaganov · 2017-11-27T23:39:57Z

include/oboe/Definitions.h

+        Stop = AAUDIO_CALLBACK_RESULT_STOP,
+    };
+
+    enum class Result : aaudio_result_t {


Consider renaming this class to "Error", which would allow dropping the "Error" prefix from the enum items.

Is it normal to have an Error::OK value? We're going to see a lot of:

if (error != Error::OK){

Yes, that's perhaps not the best expression. But seeing a lot of "Error" prefixes doesn't make me happy either. Although, I've just checked Vulkan's 'Result' type, and they also have "error" prefixes everywhere. OK, let's not consider changing anything here for now.

mnaganov · 2017-11-27T23:52:28Z

include/oboe/Definitions.h

+
+    enum class Result : aaudio_result_t {
+        OK,
+        ErrorBase = AAUDIO_ERROR_BASE,


ErrorBase shouldn't be used as a source of an assignment anywhere. To prevent this, I would suggest introducing a helper function 'isError' which compares the value against AAUDIO_ERROR_BASE.

Why don't I just remove ErrorBase from the list of possible values in the Result enum?

Sure. What I've meant is that if anyone will ever need to check against AAUDIO_ERROR_BASE, you can provide a convenience method.

ErrorBase removed

mnaganov · 2017-11-27T23:59:54Z

include/oboe/StreamBase.h

-    oboe_audio_format_t     mFormat = OBOE_AUDIO_FORMAT_UNSPECIFIED;
-    oboe_direction_t        mDirection = OBOE_DIRECTION_OUTPUT;
-    oboe_performance_mode_t mPerformanceMode = OBOE_PERFORMANCE_MODE_NONE;
+    StreamCallback         *mStreamCallback = NULL;


mnaganov · 2017-11-28T00:00:43Z

src/common/Stream.cpp

+        );
+        mPreviousScheduler = scheduler;
+    }
+    if (mStreamCallback == NULL) {


mnaganov · 2017-11-28T00:02:33Z

include/oboe/StreamBuilder.h

@@ -199,7 +202,7 @@ class OboeStreamBuilder : public OboeStreamBase {
     * @param streamCallback
     * @return
     */
-    OboeStreamBuilder *setCallback(OboeStreamCallback *streamCallback) {
+    StreamBuilder *setCallback(StreamCallback *streamCallback) {


Please consider specifying StreamCallback ownership via use of std::unique_ptr or std::shared_ptr. With a raw pointer, it's easy to run into a trouble of the client code deletes the callback instance.

Good spot. I think it makes sense for the client to retain ownership of the callback so that means using a std::shared_ptr<StreamCallback>. I've updated the code to do this. Would you mind taking a look.

mnaganov · 2017-11-28T00:07:48Z

src/common/Utilities.cpp

+
+void convertPcm16ToFloat(const int16_t *source, float *destination, int32_t numSamples) {
+    for (int i = 0; i < numSamples; i++) {
+        destination[i] = source[i] * (1.0f / 32768.0f);


Curious: why not just "source[i] / 32768.0f"?

@philburk one for you

To avoid rounding in opposite directions because of the cast. But Andy has a better technique now. Will change.

mnaganov

Makes sense to me. Thanks!

philburk · 2017-11-29T16:01:07Z

include/oboe/LatencyTuner.h

@@ -83,7 +83,7 @@ class LatencyTuner {
    // arbitrary number of calls to wait before bumping up the latency
    static constexpr int32_t kIdleCount = 8;

-    Stream               &mStream;
+    AudioStream               &mStream;


philburk · 2017-11-29T16:11:54Z

src/opensles/StreamBuffered.cpp

-        mInternalCallback = new AudioStreamBufferedCallback(this);
-        mStreamCallback = mInternalCallback;
-        LOGD("StreamBuffered(): mInternalCallback = %p", mInternalCallback);
+        mStreamCallback = std::make_shared<AudioStreamBufferedCallback>(this);


This makes me nervous. Can this object be deleted properly if it holds a reference to itself?

Is "this" actually an AudioStreamBufferedCallback?

"this" is the argument for AudioStreamBufferedCallback's constructor. Thus, the constructed callback will hold a raw pointer to the stream, while the stream will be holding a refcounted pointer to the callback.

this is an AudioStreamBuffered. The call is equivalent to doing new AudioStreamBufferedCallback(this) except that instead of a raw pointer the result is a shared_ptr which will ensure deletion when the refcount drops to zero.

Cool. Thanks for the explanation.

krocard · 2017-11-29T22:34:26Z

FullGuide.md

@@ -28,11 +28,11 @@ The audio device attached to a stream determines whether the stream is for input

 A stream has a sharing mode:

-*   `OBOE_SHARING_MODE_EXCLUSIVE` (available on API 26+) means the stream has exclusive access to its audio device; the device cannot be used by any other audio stream. If the audio device is already in use, it might not be possible for the stream to have exclusive access. Exclusive streams provide the lowest possible latency, but they are also more likely to get disconnected. You should close exclusive streams as soon as you no longer need them, so that other apps can access the device.
-*   `OBOE_SHARING_MODE_SHARED` allows Oboe to mix audio. Oboe mixes all the shared streams assigned to the same device.
+*   `SharingMode::Exclusive` (available on API 26+) means the stream has exclusive access to its audio device; the device cannot be used by any other audio stream. If the audio device is already in use, it might not be possible for the stream to have exclusive access. Exclusive streams provide the lowest possible latency, but they are also more likely to get disconnected. You should close exclusive streams as soon as you no longer need them, so that other apps can access the device.


audio device

Are we defining the oboe meaning of audio device somewhere ? Because audio device as define by the policy and audio flinger means the actual hardware (speaker, mic...). But exclusive mode allows exclusive access to a front end of this hardware. So

the device cannot be used by any other audio stream

might be confusing for the user as it does not mean the hardware can not be concurrently played to. For example on pixel, the dsp will mix the aaudio exclusive port with the audioflinger low latency port resulting in concurrent playback on the same hardware.

Ignore this comment if this is subtility of the "exclusive" mode is explain in some other part of the doc.

I am very open to a better term than "audio device". Calling it a "front end of this hardware" is still not very clear. Maybe "audio resource"? A phone can have two "front ends" for a device. One is used by AudioTrack and one is available for MMAP.

Let's fix this doc issue in a later CL. We need to get this big refactoring done.

I used the concept of an audio device "endpoint" to explain this more clearly in #23.

krocard · 2017-11-29T23:00:05Z

include/oboe/Definitions.h

+        Stop = AAUDIO_CALLBACK_RESULT_STOP,
+    };
+
+    enum class Result : aaudio_result_t {


We are relying on aaudio_result_t beeing int32_t in multiple places. Maybe we should add in some .cpp static_assert(is_same<int32_t, aaudio_result_t>)

Thanks. Added checks for all AAudio primitive data types.

krocard · 2017-11-29T23:08:27Z

include/oboe/AudioStreamBase.h

+#include "oboe/AudioStreamCallback.h"
+#include "oboe/Definitions.h"
+
+namespace oboe {


You may or may not want to add an inline version namespace.
http://en.cppreference.com/w/cpp/language/namespace#Inline_namespaces
For non template library like Oboe, the advantage of inline version namespace is that symbols of different version will have a different name. Thus preventing a user from linking to the wrong version (aka less user error).
It also allows to have 2 oboe version in the same process.
Though I am not sure we want to support this.

The intention was to have an App statically link to Oboe.
I suppose there could be a problem if an app tried to use with a library that had it's own calls to an old version of Oboe. But that seems unlikely. Most audio libraries just pass PCM in and out and leave the audio I/O to the main app..

If Oboe is static linked, inline version namespace have a lot less attractivity. Do not bother with it.

krocard · 2017-11-29T23:50:35Z

src/common/Utilities.cpp

+        OBOE_CASE_ENUM(Result::ErrorNoService);
+        OBOE_CASE_ENUM(Result::ErrorInvalidRate);
+        default:
+            return "Unrecognized result";


It would be nice to return the unrecognized value. Eg: "unrecognized result (-50)" to help the developer determined the problem (wrong casting, value is positive (not an error)...). Though, it would mean changing the api...

I've added a TODO for this as it's a non-breaking change and I couldn't figure out exactly how this could be achieved easily without a lot of messing about with stringstream->string->c_str

This IS a breaking change. You would have to return a string buffer that the client owned (eg: std::string). instead of returning a static char *

krocard · 2017-11-29T23:54:22Z

src/common/Utilities.cpp

+#include "oboe/Definitions.h"
+#include "oboe/Utilities.h"
+
+#define OBOE_CASE_ENUM(name) case name: return #name


I usually like having both the human readable value and the number. How about

#define STR(x) #x #define OBOE_CASE_ENUM(name) case name: return #name " (" STR(name) ")"

I tried this and it just outputs the type name e.g. "Result::OK" twice in a row. I think I need to static_cast name to an int32_t then concat with the return string - could be tough to implement in a macro.

Right, for Kevin's approach to work, "name" must be a preprocessor define, so it can be expanded into a value during macro expansion phase, like described here: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/cpp/Stringification.html. Preprocessor knows nothing about the values of enums.

But actually, the tricky part that involves the use of preprocessor is getting the program symbol name, that is, the name of the enum constant. The value of a enum constant can be obtained at compile- or run-time.

So, for example, in your enum to string function you could use a stringstream, where you first put the value of the enum (unconditionally), and then use the stringification trick in order to add the symbol name of the enum.

Right, I though the name was an AAudio constant. There are solutions to convert the enum value to a compile time string, but it would add too much complexity. Hopefully in C++20, reflection will make all that much easier.
The alternative would be to construct the string at build time which would make it much easier:

template <class Enum, Enum enum> const char* enumToStr(const char* name) { const static str = name } #define OBOE_CASE_ENUM(name) case name: return std::string(#name) + " (" + std::to_string(name) + ')';

But that changes the API, as a std::string has to be returned.

krocard · 2017-11-30T00:36:31Z

src/common/Utilities.cpp

+    }
+}
+
+const char *convertSharingModeToText(SharingMode mode) {


Because the input type is already in the signature, you may or may not want to remove it from the function name:

const char *convertToText(AudioFormat format); const char *convertToText(PerformanceMode mode); const char *convertToText(SharingMode mode);

but for one it is a style preference secondly some people prefer to be explicit.
Of course there is also the template route that allows explicit and implicit input type specification:

template <class From> const char * convertToText(From); template <> const char* convertToText<AudioFormat>(AudioFormat format); // ... // Which can be used explicitly: auto format = AudioFormat::I16; cout << convertToText<AudioFormat>(format); // or implicitly cout << convertToText(format);

But this is not the current style of the API, plus it is a backward incompatible change that might not be worth it.

This CL breaks the API anyway. We warned people it would break. And we are modernizing. So now is the time if we want to do this kind of stuff. Up to you Don.

The implicit use of convertToText is really nice. Have implemented.

philburk

Looks great! Thanks Don.

philburk

OK to finish adding the int value in a later CL.

dturner added 2 commits November 27, 2017 18:45

Use oboe namespace

3bf32ae

Documentation tidy up

2786fb5

dturner requested a review from philburk November 27, 2017 21:40

dturner commented Nov 27, 2017

View reviewed changes

philburk requested changes Nov 27, 2017

View reviewed changes

mnaganov reviewed Nov 27, 2017

View reviewed changes

mnaganov approved these changes Nov 28, 2017

View reviewed changes

dturner added 3 commits November 28, 2017 18:10

More documentation fixes

89c9076

Add default case to convert methods

e5bc9d9

Use std::shared_ptr for callback object

da0edda

mnaganov approved these changes Nov 28, 2017

View reviewed changes

dturner added 2 commits November 29, 2017 15:49

Rename Stream to AudioStream

379e8e5

Minor edits to address comments

93fad7d

philburk reviewed Nov 29, 2017

View reviewed changes

member vairable alignment

338eac6

krocard reviewed Nov 30, 2017

View reviewed changes

philburk approved these changes Nov 30, 2017

View reviewed changes

dturner added 2 commits December 1, 2017 13:51

Revert to using raw pointer for setCallback

d4b7f08

Adding template method convertToText

5ea2530

philburk approved these changes Dec 1, 2017

View reviewed changes

Add TODO for convertToText method to return int value in string

605c25c

philburk approved these changes Dec 1, 2017

View reviewed changes

dturner merged commit 2e10dec into master Dec 1, 2017

This was referenced Dec 1, 2017

Support latest version of Oboe googlearchive/android-audio-high-performance#90

Merged

When overriding onAudioReady the return type should be oboe_data_callback_result_t #14

Closed

Convert methods could return a std::string and numeric value #24

Closed

philburk deleted the namespace branch May 22, 2018 14:04

myj420 mentioned this pull request Feb 25, 2021

when call oboe::AudioStreamBuilder openStream function,the system will be stuck #1202

Open

extreamsd mentioned this pull request Jan 30, 2022

Crash in aaudio::AudioStreamLegacy::getBestTimestamp() #1489

Open

Hyyzt mentioned this pull request Aug 18, 2022

the drumthumper have a crash #1608

Closed


		Define an `OboeStreamCallback` class to receive callbacks whenever the stream requires new data.
		Define an `StreamCallback` class to receive callbacks whenever the stream requires new data.

		@@ -171,7 +174,7 @@ class OboeStreamBuilder : public OboeStreamBase {
		* @param performanceMode for example, OBOE_PERFORMANCE_MODE_LOW_LATENCY

		@@ -185,7 +188,7 @@ class OboeStreamBuilder : public OboeStreamBase {
		* @param deviceId device identifier or OBOE_DEVICE_UNSPECIFIED


		constexpr int32_t kUnspecified = 0;

		constexpr int64_t kNanosPerMicrosecond = 1000;

New namespace, constexprs and scoped enums #18

New namespace, constexprs and scoped enums #18

Conversation

dturner commented Nov 27, 2017 • edited

Summary

Detailed changelog

1) oboe namespace used for all classes and functions

2) Constants defined by macros are replaced by constexpr and scoped enums

3) Oboe_convert*Text methods replaced with single convertToText method. **

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

philburk left a 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

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

mnaganov left a 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 Nov 29, 2017 • edited

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

krocard Dec 1, 2017 • edited

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

krocard Nov 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philburk left a comment

Choose a reason for hiding this comment

dturner commented Nov 27, 2017 •

edited

1) `oboe` namespace used for all classes and functions

2) Constants defined by macros are replaced by `constexpr` and scoped enums

3) `Oboe_convert*Text` methods replaced with single `convertToText` method. **

dturner Nov 29, 2017 •

edited

krocard Dec 1, 2017 •

edited

krocard Nov 30, 2017 •

edited