add input capture support to OpenSL wrapper #26

philburk · 2017-12-14T20:44:05Z

Refactored the OpenSL ES code.
Moved static functions into classes.
Split into an Input and Output stream.
Added RECORD support.
Added some thread safety to EngineOpenSLES.
Fixed some bugs and removed asserts.

Put Engine and OutputMixer into Singleton classes. Create separate input and output classes.

Also fix some bugs in OpenSL ES implementation and replace asserts with error returns.

Use mutex. Better singleton behavior. Separate files for each class.

mnaganov · 2017-12-14T22:28:37Z

src/opensles/AudioStreamOpenSLES.h

@@ -64,8 +61,23 @@ class AudioStreamOpenSLES : public AudioStreamBuffered {

    int32_t getFramesPerBurst() override;

+    static SLuint32 getDefaultByteOrder();
+
+    virtual int chanCountToChanMask(int chanCount) = 0;


Why this method needs to be declared as part of the interface? It doesn't seem to be used in any common code, only in particular implementations. Also, it can be made static.

I made it protected. It was already static. It is used by input and output streams to open a stream.

Sorry if I didn't explain my point clear enough. What I was trying to say is that virtual methods in the base class usually exist because there is either an external client or common code in the base class that uses them. For chanCountToChanMask, the usages only seem to be in the derived classes (in and out streams). So why not just define these methods in the derived classes?

Putting chanCountToChanMask doesn't help with code reuse, it only pollutes the interface definition.

mnaganov · 2017-12-14T22:31:16Z

src/opensles/AudioOutputStreamOpenSLES.cpp

+
+        case  2:
+            channelMask = SL_SPEAKER_FRONT_LEFT | SL_SPEAKER_FRONT_RIGHT;
+            break;


Why isn't this defined as SL_ANDROID_SPEAKER_STEREO? Then the masks above can use it as well.

mnaganov · 2017-12-14T22:31:45Z

src/opensles/AudioOutputStreamOpenSLES.cpp

+#define SL_ANDROID_SPEAKER_QUAD (SL_SPEAKER_FRONT_LEFT | SL_SPEAKER_FRONT_RIGHT \
+ | SL_SPEAKER_BACK_LEFT | SL_SPEAKER_BACK_RIGHT)
+
+#define SL_ANDROID_SPEAKER_5DOT1 (SL_SPEAKER_FRONT_LEFT | SL_SPEAKER_FRONT_RIGHT \


Can be defined as an extension to QUAD.

mnaganov · 2017-12-14T22:34:27Z

src/opensles/AudioInputStreamOpenSLES.cpp

+
+    result = AudioStreamOpenSLES::registerBufferQueueCallback();
+    if (SL_RESULT_SUCCESS != result) {
+        LOGE("get bufferqueue interface:%p result:%s", mSimpleBufferQueueInterface, getSLErrStr(result));


Why isn't this error message being printed by registerBufferQueueCallback? You seem to have the same code in the output stream.

It is printed in registerBufferQueueCallback() so I removed it from the input and output streams.

mnaganov · 2017-12-14T22:36:52Z

src/opensles/EngineOpenSLES.cpp

+
+EngineOpenSLES *EngineOpenSLES::getInstance() {
+    static EngineOpenSLES sInstance;
+    return &sInstance;


It is preferred to return a reference (EngineOpenSLES&) rather than pointer for preventing clients from attempting to calling 'delete' on it or putting it into a smart pointer.

Also, references can't be null which makes caller's life easier.

mnaganov · 2017-12-14T22:44:27Z

src/opensles/OpenSLESUtilities.cpp

+            return SL_ANDROID_PCM_REPRESENTATION_SIGNED_INT;
+        case AudioFormat::Float:
+            return SL_ANDROID_PCM_REPRESENTATION_FLOAT;
+    }


mnaganov · 2017-12-14T22:44:52Z

src/opensles/OutputMixerOpenSLES.cpp

+
+OutputMixerOpenSL *OutputMixerOpenSL::getInstance() {
+    static OutputMixerOpenSL sInstance;
+    return &sInstance;


Same note about returning the reference.

mnaganov · 2017-12-14T22:47:03Z

src/opensles/OutputMixerOpenSLES.h

+
+private:
+    std::mutex            mLock;
+    std::atomic<int32_t>  mOpenCount{0};


Same note about atomicity.

mnaganov · 2017-12-14T22:52:34Z

src/opensles/EngineOpenSLES.h

+                                 SLDataSource *audioSource,
+                                 SLDataSink *audioSink);
+
+private:


The constructor and the destructor need to be made private, and copy and assignment constructors deleted. Otherwise, someone could make another instance of this singleton.

See example code here: http://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton

mnaganov · 2017-12-14T22:53:12Z

src/opensles/OutputMixerOpenSLES.h

+    SLresult createAudioPlayer(SLObjectItf *objectItf,
+                               SLDataSource *audioSource);
+
+private:


Same note about the need to hide and delete constructors.

dturner · 2017-12-15T17:31:18Z

src/opensles/AudioInputStreamOpenSLES.cpp

@@ -0,0 +1,219 @@
+/*


This file must be added to CMakeLists.txt so that clients can build it

Here's the relevant part of the CMakeLists.txt updated so this version builds and links correctly:

set (oboe_sources src/aaudio/AAudioLoader.cpp src/aaudio/AudioStreamAAudio.cpp src/common/LatencyTuner.cpp src/common/AudioStream.cpp src/common/AudioStreamBuilder.cpp src/common/Utilities.cpp src/fifo/FifoBuffer.cpp src/fifo/FifoController.cpp src/fifo/FifoControllerBase.cpp src/fifo/FifoControllerIndirect.cpp src/opensles/AudioInputStreamOpenSLES.cpp src/opensles/AudioOutputStreamOpenSLES.cpp src/opensles/AudioStreamBuffered.cpp src/opensles/AudioStreamOpenSLES.cpp src/opensles/EngineOpenSLES.cpp src/opensles/OpenSLESUtilities.cpp src/opensles/OutputMixerOpenSLES.cpp )

dturner · 2017-12-15T17:59:30Z

src/opensles/AudioOutputStreamOpenSLES.cpp

+}
+
+// These will wind up in <SLES/OpenSLES_Android.h>
+#define SL_ANDROID_SPEAKER_QUAD (SL_SPEAKER_FRONT_LEFT | SL_SPEAKER_FRONT_RIGHT \


Better practice to use constexpr here

dturner · 2017-12-15T18:00:59Z

src/opensles/AudioOutputStreamOpenSLES.cpp

+        return Result::ErrorInternal;
+    }
+
+    SLuint32 bitsPerSample = getBytesPerSample() * OBOE_BITS_PER_BYTE;


Use constexpr for this oboe constant and add it to the oboe namespace. The usual convention is to prefix constants with k so this would end up as oboe::kBitsPerByte

dturner · 2017-12-15T18:01:56Z

src/opensles/AudioStreamOpenSLES.h


 namespace oboe {

+#define OBOE_BITS_PER_BYTE            8      // common value TODO modernize


See previous comment

Use constexpr. Add new files to CMakeLists.txt

mnaganov

Please resolve the note about chanCountToChanMask before submitting.

dturner

Great work!! Really excited to see this major feature implemented.

Phil Burk added 5 commits December 13, 2017 18:36

oboe: OpenSL ES refactor engine

64c55f4

Put Engine and OutputMixer into Singleton classes. Create separate input and output classes.

oboe: move OpenSL ES output code to its own class

9a147a1

oboe: implement input stream

6d95291

Also fix some bugs in OpenSL ES implementation and replace asserts with error returns.

oboe: cleanup OpenSL ES input

420715b

Use mutex. Better singleton behavior. Separate files for each class.

oboe: OPenSL ES opens moved into stream open

d9703b0

philburk requested a review from dturner December 14, 2017 20:44

mnaganov reviewed Dec 14, 2017

View reviewed changes

dturner requested changes Dec 15, 2017

View reviewed changes

oboe: address review comments, better singleton

7040a9d

Use constexpr. Add new files to CMakeLists.txt

mnaganov approved these changes Dec 15, 2017

View reviewed changes

oboe: make chanCountToChanMask non-virtual

9b1c2ce

dturner approved these changes Dec 15, 2017

View reviewed changes

philburk merged commit a40baae into master Dec 15, 2017

dturner mentioned this pull request Jan 5, 2018

Add support for INPUT streams for OpenSL ES #2

Closed

philburk deleted the addinput branch February 9, 2018 01:01

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

add input capture support to OpenSL wrapper #26

add input capture support to OpenSL wrapper #26

philburk commented Dec 14, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

mnaganov Dec 14, 2017

philburk Dec 15, 2017

dturner Dec 15, 2017

dturner Dec 15, 2017

philburk Dec 15, 2017

dturner Dec 15, 2017

philburk Dec 15, 2017

dturner Dec 15, 2017

philburk Dec 15, 2017

dturner Dec 15, 2017

philburk Dec 15, 2017

mnaganov left a comment

dturner left a comment


		namespace oboe {

		#define OBOE_BITS_PER_BYTE 8 // common value TODO modernize

add input capture support to OpenSL wrapper #26

add input capture support to OpenSL wrapper #26

Conversation

philburk commented Dec 14, 2017

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

Choose a reason for hiding this comment

mnaganov left a comment

Choose a reason for hiding this comment

dturner left a comment

Choose a reason for hiding this comment