Add MuteSolo checks for audio player and fix bugs

The specification says "This [SLMuteSoloItf] interface cannot be
exposed on a player whose audio format is mono." This has a lot of
implications ...

Deny an explicit interface request for SLMuteSoloItf at audio player
creation time if the player is known pre-realize to have mono channel
count (e.g. PCM buffer queue).

Deny a GetInterface on MuteSolo if channel count is known to be mono,
but allow it if the channel count is still unknown.

When DynamicSource (or its replacement) is implemented, it will be
possible for the application to have done a GetInterface successfully,
and then to change the source such that the MuteSolo interface is
no longer valid.  In that case, return SL_RESULT_FEATURE_UNSUPPORTED
for most MuteSolo operations, if called when channel count is unknown
or is mono.  We _do_ allow MuteSolo::GetChannelCount to succeed if
channel count is known to be mono.

Enhance the URI-based mute solo test to check some of the above,
and the interactive buffer queue test to get and test the channel count.

Add curly braces to if statements.

Other bug fixes:
 - Do not rely on peek locks; they don't work on SMP.
 - Channel count and sample rate were not being set correctly
   for buffer queue data source audio players.
 - CreateAudioPlayer and AudioRecorder were both referring to
   application pointers more than once, with the possibility of
   reading inconsistent data.

Change-Id: Ie0109cbddc0aff8a56c0b53d989fb2be823e627b

7 files changed, 126 insertions, 38 deletions

diff --git a/opensles/libopensles/CAudioPlayer.c b/opensles/libopensles/CAudioPlayer.c
index d27f15b5..c70f204d 100644
--- a/opensles/libopensles/CAudioPlayer.c
+++ b/opensles/libopensles/CAudioPlayer.c
@@ -26,9 +26,6 @@ SLresult CAudioPlayer_Realize(void *self, SLboolean async)
     CAudioPlayer *this = (CAudioPlayer *) self;
     SLresult result = SL_RESULT_SUCCESS;
 
-    // initialize cached data, to be overwritten by platform-specific initialization
-    this->mNumChannels = 0;
-
 #ifdef ANDROID
     result = android_audioPlayer_realize(this, async);
 #endif
@@ -37,6 +34,10 @@ SLresult CAudioPlayer_Realize(void *self, SLboolean async)
     result = SndFile_Realize(this);
 #endif
 
+    // At this point the channel count and sample rate might still be unknown,
+    // depending on the data source and the platform implementation.
+    // If they are unknown here, then they will be determined during prefetch.
+
     return result;
 }
 
diff --git a/opensles/libopensles/IEngine.c b/opensles/libopensles/IEngine.c
index 865da4de..3352346b 100644
--- a/opensles/libopensles/IEngine.c
+++ b/opensles/libopensles/IEngine.c
@@ -123,7 +123,8 @@ static SLresult IEngine_CreateAudioPlayer(SLEngineItf self, SLObjectItf *pPlayer
                     // Initialize private fields not associated with an interface
                     this->mMuteMask = 0;
                     this->mSoloMask = 0;
-                    // const, will be set later by the containing AudioPlayer or MidiPlayer
+                    // Will be set soon for PCM buffer queues, or later by platform-specific code
+                    // during Realize or Prefetch
                     this->mNumChannels = 0;
                     this->mSampleRateMilliHz = 0;
 
@@ -136,13 +137,18 @@ static SLresult IEngine_CreateAudioPlayer(SLEngineItf self, SLObjectItf *pPlayer
                         break;
                     }
 
-                    result = checkSourceFormatVsInterfacesCompatibility(&this->mDataSource,
-                            numInterfaces, pInterfaceIds, pInterfaceRequired);
+                    result = checkDataSink(pAudioSnk, &this->mDataSink, SL_OBJECTID_AUDIOPLAYER);
                     if (SL_RESULT_SUCCESS != result) {
                         break;
                     }
 
-                    result = checkDataSink(pAudioSnk, &this->mDataSink, SL_OBJECTID_AUDIOPLAYER);
+                    // It would be unsafe to ever refer to the application pointers again
+                    pAudioSrc = NULL;
+                    pAudioSnk = NULL;
+
+                    // Check that the requested interfaces are compatible with the data source
+                    result = checkSourceFormatVsInterfacesCompatibility(&this->mDataSource,
+                            numInterfaces, pInterfaceIds, pInterfaceRequired);
                     if (SL_RESULT_SUCCESS != result) {
                         break;
                     }
@@ -150,13 +156,16 @@ static SLresult IEngine_CreateAudioPlayer(SLEngineItf self, SLObjectItf *pPlayer
                     // copy the buffer queue count from source locator to the buffer queue interface
                     // we have already range-checked the value down to a smaller width
 
-                    switch (*(SLuint32 *) pAudioSrc->pLocator) {
+                    switch (this->mDataSource.mLocator.mLocatorType) {
                     case SL_DATALOCATOR_BUFFERQUEUE:
 #ifdef ANDROID
                     case SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE:
 #endif
-                        this->mBufferQueue.mNumBuffers = (SLuint16) ((SLDataLocator_BufferQueue *)
-                            pAudioSrc->pLocator)->numBuffers;
+                        this->mBufferQueue.mNumBuffers =
+                                (SLuint16) this->mDataSource.mLocator.mBufferQueue.numBuffers;
+                        assert(SL_DATAFORMAT_PCM == this->mDataSource.mFormat.mFormatType);
+                        this->mNumChannels = this->mDataSource.mFormat.mPCM.numChannels;
+                        this->mSampleRateMilliHz = this->mDataSource.mFormat.mPCM.samplesPerSec;
                         break;
                     default:
                         this->mBufferQueue.mNumBuffers = 0;
@@ -268,9 +277,18 @@ static SLresult IEngine_CreateAudioRecorder(SLEngineItf self, SLObjectItf *pReco
             } else {
 
                 do {
-                    // const, will be set later by the containing AudioRecorder
+
+                    // Initialize fields not associated with any interface
+
+                    // These fields are set to real values by
+                    // android_audioRecorder_checkSourceSinkSupport.  Note that the data sink is
+                    // always PCM buffer queue, so we know the channel count and sample rate early.
                     this->mNumChannels = 0;
                     this->mSampleRateMilliHz = 0;
+#ifdef ANDROID
+                    this->mAudioRecord = NULL;
+                    this->mRecordSource = android::AUDIO_SOURCE_DEFAULT;
+#endif
 
                     // Check the source and sink parameters, and make a local copy of all parameters
                     result = checkDataSource(pAudioSrc, &this->mDataSource);
@@ -282,6 +300,10 @@ static SLresult IEngine_CreateAudioRecorder(SLEngineItf self, SLObjectItf *pReco
                         break;
                     }
 
+                    // It would be unsafe to ever refer to the application pointers again
+                    pAudioSrc = NULL;
+                    pAudioSnk = NULL;
+
                     // check the audio source and sink parameters against platform support
 #ifdef ANDROID
                     result = android_audioRecorder_checkSourceSinkSupport(this);
@@ -293,10 +315,10 @@ static SLresult IEngine_CreateAudioRecorder(SLEngineItf self, SLObjectItf *pReco
 
 #ifdef ANDROID
                     // FIXME move to dedicated function
-                    SLuint32 locatorType = *(SLuint32 *) pAudioSnk->pLocator;
+                    SLuint32 locatorType = this->mDataSink.mLocator.mLocatorType;
                     if (locatorType == SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE) {
-                        this->mBufferQueue.mNumBuffers = ((SLDataLocator_BufferQueue *)
-                                pAudioSnk->pLocator)->numBuffers;
+                        this->mBufferQueue.mNumBuffers =
+                            this->mDataSink.mLocator.mBufferQueue.numBuffers;
                         // inline allocation of circular Buffer Queue mArray, up to a typical max
                         if (BUFFER_HEADER_TYPICAL >= this->mBufferQueue.mNumBuffers) {
                             this->mBufferQueue.mArray = this->mBufferQueue.mTypical;
diff --git a/opensles/libopensles/IMuteSolo.c b/opensles/libopensles/IMuteSolo.c
index 5a6f4641..2811a715 100644
--- a/opensles/libopensles/IMuteSolo.c
+++ b/opensles/libopensles/IMuteSolo.c
@@ -29,16 +29,22 @@ static SLresult IMuteSolo_SetChannelMute(SLMuteSoloItf self, SLuint8 chan, SLboo
         result = SL_RESULT_FEATURE_UNSUPPORTED;
     } else {
         CAudioPlayer *ap = (CAudioPlayer *) thisObject;
-        if (ap->mNumChannels <= chan) {
+        interface_lock_exclusive(this);
+        SLuint8 numChannels = ap->mNumChannels;
+        if (1 >= numChannels) {
+            interface_unlock_exclusive(this);
+            result = SL_RESULT_FEATURE_UNSUPPORTED;
+        } else if (numChannels <= chan) {
+            interface_unlock_exclusive(this);
             result = SL_RESULT_PARAMETER_INVALID;
         } else {
             SLuint8 mask = 1 << chan;
-            interface_lock_exclusive(this);
             SLuint8 oldMuteMask = ap->mMuteMask;
-            if (mute)
+            if (mute) {
                 ap->mMuteMask |= mask;
-            else
+            } else {
                 ap->mMuteMask &= ~mask;
+            }
             interface_unlock_exclusive_attributes(this, oldMuteMask != ap->mMuteMask ? ATTR_GAIN :
                 ATTR_NONE);
             result = SL_RESULT_SUCCESS;
@@ -62,15 +68,22 @@ static SLresult IMuteSolo_GetChannelMute(SLMuteSoloItf self, SLuint8 chan, SLboo
             result = SL_RESULT_FEATURE_UNSUPPORTED;
         } else {
             CAudioPlayer *ap = (CAudioPlayer *) thisObject;
-            if (ap->mNumChannels <= chan) {
+            SLboolean mute;
+            interface_lock_shared(this);
+            SLuint8 numChannels = ap->mNumChannels;
+            if (1 >= numChannels) {
+                mute = SL_BOOLEAN_FALSE;
+                result = SL_RESULT_FEATURE_UNSUPPORTED;
+            } else if (numChannels <= chan) {
+                mute = SL_BOOLEAN_FALSE;
                 result = SL_RESULT_PARAMETER_INVALID;
             } else {
-                interface_lock_peek(this);
                 SLuint8 mask = ap->mMuteMask;
-                interface_unlock_peek(this);
-                *pMute = (mask >> chan) & 1;
+                mute = (SLboolean) ((mask >> chan) & 1);
                 result = SL_RESULT_SUCCESS;
             }
+            interface_unlock_shared(this);
+            *pMute = mute;
         }
     }
 
@@ -88,16 +101,22 @@ static SLresult IMuteSolo_SetChannelSolo(SLMuteSoloItf self, SLuint8 chan, SLboo
         result = SL_RESULT_FEATURE_UNSUPPORTED;
     } else {
         CAudioPlayer *ap = (CAudioPlayer *) thisObject;
-        if (ap->mNumChannels <= chan)
+        interface_lock_exclusive(this);
+        SLuint8 numChannels = ap->mNumChannels;
+        if (1 >= numChannels) {
+            interface_unlock_exclusive(this);
+            result = SL_RESULT_FEATURE_UNSUPPORTED;
+        } else if (numChannels <= chan) {
+            interface_unlock_exclusive(this);
             result = SL_RESULT_PARAMETER_INVALID;
-        else {
+        } else {
             SLuint8 mask = 1 << chan;
-            interface_lock_exclusive(this);
             SLuint8 oldSoloMask = ap->mSoloMask;
-            if (solo)
+            if (solo) {
                 ap->mSoloMask |= mask;
-            else
+            } else {
                 ap->mSoloMask &= ~mask;
+            }
             interface_unlock_exclusive_attributes(this, oldSoloMask != ap->mSoloMask ? ATTR_GAIN :
                 ATTR_NONE);
             result = SL_RESULT_SUCCESS;
@@ -121,15 +140,22 @@ static SLresult IMuteSolo_GetChannelSolo(SLMuteSoloItf self, SLuint8 chan, SLboo
             result = SL_RESULT_FEATURE_UNSUPPORTED;
         } else {
             CAudioPlayer *ap = (CAudioPlayer *) thisObject;
-            if (ap->mNumChannels <= chan) {
+            SLboolean solo;
+            interface_lock_shared(this);
+            SLuint8 numChannels = ap->mNumChannels;
+            if (1 >= numChannels) {
+                solo = SL_BOOLEAN_FALSE;
+                result = SL_RESULT_FEATURE_UNSUPPORTED;
+            } else if (numChannels <= chan) {
+                solo = SL_BOOLEAN_FALSE;
                 result = SL_RESULT_PARAMETER_INVALID;
             } else {
-                interface_lock_peek(this);
                 SLuint8 mask = ap->mSoloMask;
-                interface_unlock_peek(this);
-                *pSolo = (mask >> chan) & 1;
+                solo = (SLboolean) ((mask >> chan) & 1);
                 result = SL_RESULT_SUCCESS;
             }
+            interface_unlock_shared(this);
+            *pSolo = solo;
         }
     }
 
@@ -150,9 +176,11 @@ static SLresult IMuteSolo_GetNumChannels(SLMuteSoloItf self, SLuint8 *pNumChanne
             result = SL_RESULT_FEATURE_UNSUPPORTED;
         } else {
             CAudioPlayer *ap = (CAudioPlayer *) thisObject;
-            // no lock needed as mNumChannels is const
-            *pNumChannels = ap->mNumChannels;
-            result = SL_RESULT_SUCCESS;
+            object_lock_shared(thisObject);
+            SLuint8 numChannels = ap->mNumChannels;
+            object_unlock_shared(thisObject);
+            *pNumChannels = numChannels;
+            result = 0 < numChannels ? SL_RESULT_SUCCESS : SL_RESULT_PRECONDITIONS_VIOLATED;
         }
     }
 
diff --git a/opensles/libopensles/IObject.c b/opensles/libopensles/IObject.c
index e7c47b31..e06e1f5c 100644
--- a/opensles/libopensles/IObject.c
+++ b/opensles/libopensles/IObject.c
@@ -335,6 +335,11 @@ static SLresult IObject_GetInterface(SLObjectItf self, const SLInterfaceID iid,
                     // Can't get interface on a suspended/suspending/resuming object
                     // unless this is an explicit pre-realize interface
                     result = SL_RESULT_PRECONDITIONS_VIOLATED;
+                } else if ((MPH_MUTESOLO == MPH) && (SL_OBJECTID_AUDIOPLAYER == class__->mObjectID)
+                        && (1 == ((CAudioPlayer *) this)->mNumChannels)) {
+                    // Can't get the MuteSolo interface of an audio player if the channel count is
+                    // mono, but _can_ get the MuteSolo interface if the channel count is unknown
+                    result = SL_RESULT_FEATURE_UNSUPPORTED;
                 } else {
                     switch (this->mInterfaceStates[index]) {
                     case INTERFACE_EXPOSED:
diff --git a/opensles/libopensles/sles.c b/opensles/libopensles/sles.c
index 4c038be5..686fe605 100644
--- a/opensles/libopensles/sles.c
+++ b/opensles/libopensles/sles.c
@@ -666,6 +666,12 @@ SLresult checkSourceFormatVsInterfacesCompatibility(const DataLocatorFormat *pDa
                 SL_LOGE("can't request SL_IID_SEEK with a buffer queue data source");
                 return SL_RESULT_FEATURE_UNSUPPORTED;
             }
+            if (pInterfaceRequired[i] && (SL_IID_MUTESOLO == pInterfaceIds[i]) &&
+                    (SL_DATAFORMAT_PCM == pDataLocatorFormat->mFormat.mFormatType) &&
+                    (1 == pDataLocatorFormat->mFormat.mPCM.numChannels)) {
+                SL_LOGE("can't request SL_IID_MUTESOLO with a mono buffer queue data source");
+                return SL_RESULT_FEATURE_UNSUPPORTED;
+            }
         }
         break;
     default:
@@ -992,6 +998,7 @@ extern void
 IObject *construct(const ClassTable *class__, unsigned exposedMask, SLEngineItf engine)
 {
     IObject *this;
+    // Do not change this to malloc; we depend on the object being memset to zero
     this = (IObject *) calloc(1, class__->mSize);
     if (NULL != this) {
         unsigned lossOfControlMask = 0;
diff --git a/opensles/tests/listening/slesTest_playMuteSolo.cpp b/opensles/tests/listening/slesTest_playMuteSolo.cpp
index 7ef101ba..d142cf4b 100644
--- a/opensles/tests/listening/slesTest_playMuteSolo.cpp
+++ b/opensles/tests/listening/slesTest_playMuteSolo.cpp
@@ -212,6 +212,13 @@ void TestPlayUri( SLObjectItf sl, const char* path)
     result = (*player)->GetInterface(player, SL_IID_MUTESOLO, (void*)&muteSoloItf);
     ExitOnError(result);
 
+    // Attempt to get the channel count before it is necessarily known.
+    // This may fail depending on the platform.
+    SLuint8 numChannels = 123;
+    result = (*muteSoloItf)->GetNumChannels(muteSoloItf, &numChannels);
+    printf("GetNumChannels after Realize but before pre-fetch: result=%lu, numChannels=%u\n",
+        result, numChannels);
+
     /* Initialize a context for use by the callback */
     Context             context;
     context.playItf = playItf;
@@ -243,7 +250,7 @@ void TestPlayUri( SLObjectItf sl, const char* path)
     }
 
     /* Query the number of channels */
-    SLuint8 numChannels = 0;
+    numChannels = 123;
     result = (*muteSoloItf)->GetNumChannels(muteSoloItf, &numChannels);
     ExitOnError(result);
     fprintf(stdout, "Content has %d channel(s)\n", numChannels);
diff --git a/opensles/tests/sandbox/intbufq.c b/opensles/tests/sandbox/intbufq.c
index f8a22b45..99289301 100644
--- a/opensles/tests/sandbox/intbufq.c
+++ b/opensles/tests/sandbox/intbufq.c
@@ -20,6 +20,7 @@
 #define USE_ANDROID_SIMPLE_BUFFER_QUEUE     // change to #undef for compatibility testing
 #endif
 
+#include <assert.h>
 #include <math.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -59,6 +60,9 @@ frame_t square[SQUARE_FRAMES];
 #define SAWTOOTH_FRAMES (44100*5)
 frame_t sawtooth[SAWTOOTH_FRAMES];
 
+#define HALF_FRAMES (44100*5)
+frame_t half[HALF_FRAMES];
+
 BufferQueueItf expectedCaller = NULL;
 void *expectedContext = NULL;
 
@@ -115,10 +119,10 @@ int main(int argc, char **argv)
     audiosnk.pLocator = &locator_outputmix;
     audiosnk.pFormat = NULL;
     SLObjectItf playerObject;
-    SLInterfaceID ids[1] = {IID_BUFFERQUEUE};
-    SLboolean flags[1] = {SL_BOOLEAN_TRUE};
+    SLInterfaceID ids[2] = {IID_BUFFERQUEUE, SL_IID_MUTESOLO};
+    SLboolean flags[2] = {SL_BOOLEAN_TRUE, SL_BOOLEAN_TRUE};
     result = (*engineEngine)->CreateAudioPlayer(engineEngine, &playerObject, &audiosrc, &audiosnk,
-            1, ids, flags);
+            2, ids, flags);
     checkResult(result);
     result = (*playerObject)->Realize(playerObject, SL_BOOLEAN_FALSE);
     checkResult(result);
@@ -128,6 +132,12 @@ int main(int argc, char **argv)
     BufferQueueItf playerBufferqueue;
     result = (*playerObject)->GetInterface(playerObject, IID_BUFFERQUEUE, &playerBufferqueue);
     checkResult(result);
+    SLMuteSoloItf playerMuteSolo;
+    result = (*playerObject)->GetInterface(playerObject, SL_IID_MUTESOLO, &playerMuteSolo);
+    checkResult(result);
+    SLuint8 numChannels = 123;
+    result = (*playerMuteSolo)->GetNumChannels(playerMuteSolo, &numChannels);
+    assert(2 == numChannels);
     SLuint32 state;
     state = SL_PLAYSTATE_PLAYING;
     result = (*playerPlay)->SetPlayState(playerPlay, state);
@@ -149,6 +159,10 @@ int main(int argc, char **argv)
         sawtooth[i].left = ((((int) (i % (unsigned) (sr / hz))) - 50) / 100.0) * 60000.0 - 30000.0;
         sawtooth[i].right = sawtooth[i].left;
     }
+    for (i = 0; i < HALF_FRAMES; ++i) {
+        half[i].left = sine[i].left;
+        half[i].right = sawtooth[i].right / 2;
+    }
 
     set_conio_terminal_mode();
     int in_count = 0;
@@ -177,6 +191,10 @@ int main(int argc, char **argv)
                 buffer = square;
                 size = sizeof(square);
                 goto enqueue;
+            case 'h':
+                buffer = half;
+                size = sizeof(half);
+                goto enqueue;
             case 'r':
                 if (in_count) {
                     expectedCaller = playerBufferqueue;