Call AudioTrack start, stop, and pause when needed

These AudioTrack operations are relatively expensive Binder calls (and
will soon be even more expensive to fix a deadlock at AudioTrack::start),
and they were being called excessively.  Now AudioTrack start, stop,
and pause are only called when there is an actual play state change.

Details:
 - distinguish ATTR_TRANSPORT vs. ATTR_PLAY_STATE for audio players
 - android_audioPlayer_setPlayState is only called when the play state changes
 - handler_AudioPlayer_transport is only called for transport changes other than play state
 - android_audioPlayer_setPlayState is always called with mutex locked
 - since media player currently only handles play state, handler_MediaPlayer_transport
   is still called for both ATTR_TRANSPORT and ATTR_PLAY_STATE
 - since the handlers for ATTR_BQ_ENQUEUE and ATTR_ABQ_ENQUEUE are only called if in
   state PLAYING, changed an "if" to an "assert"

Change-Id: Iee2968fd98d215885b7105053bb1604f962ea337

diff --git a/wilhelm/src/android/AudioPlayer_to_android.cpp b/wilhelm/src/android/AudioPlayer_to_android.cpp
index 4fceff7..e6dbfc4 100644 (file)
--- a/wilhelm/src/android/AudioPlayer_to_android.cpp
+++ b/wilhelm/src/android/AudioPlayer_to_android.cpp
@@ -1725,12 +1725,14 @@ SLresult android_audioPlayer_metadata_getValue(CAudioPlayer *ap,
 }
 
 //-----------------------------------------------------------------------------
-void android_audioPlayer_setPlayState(CAudioPlayer *ap, bool lockAP) {
+// preconditions
+//  ap != NULL
+//  mutex is locked
+//  play state has changed
+void android_audioPlayer_setPlayState(CAudioPlayer *ap) {
 
-    if (lockAP) { object_lock_shared(&ap->mObject); }
     SLuint32 playState = ap->mPlay.mState;
     AndroidObjectState objState = ap->mAndroidObjState;
-    if (lockAP) { object_unlock_shared(&ap->mObject); }
 
     switch(ap->mAndroidObjType) {
     case AUDIOPLAYER_FROM_PCM_BUFFERQUEUE:

diff --git a/wilhelm/src/android/AudioPlayer_to_android.h b/wilhelm/src/android/AudioPlayer_to_android.h
index 008a79b..f4137b5 100644 (file)
--- a/wilhelm/src/android/AudioPlayer_to_android.h
+++ b/wilhelm/src/android/AudioPlayer_to_android.h
@@ -123,7 +123,7 @@ extern SLresult android_audioPlayer_metadata_getValue(CAudioPlayer *pAudioPlayer
 /**************************************************************************************************
  * Playback control and events
  ****************************/
-extern void android_audioPlayer_setPlayState(CAudioPlayer *pAudioPlayer, bool lockAP);
+extern void android_audioPlayer_setPlayState(CAudioPlayer *pAudioPlayer);
 
 extern void android_audioPlayer_useEventMask(CAudioPlayer *pAudioPlayer);
 

diff --git a/wilhelm/src/attr.h b/wilhelm/src/attr.h
index 305e715..28668ae 100644 (file)
--- a/wilhelm/src/attr.h
+++ b/wilhelm/src/attr.h
@@ -21,14 +21,14 @@
 #define ATTR_INDEX_GAIN        0 // volume: volume, stereo position, mute
                                  // mute solo: channel mute, channel solo
                                  // effect send: set direct level
-#define ATTR_INDEX_TRANSPORT   1 // play: play state, looping, callback events mask,
+#define ATTR_INDEX_TRANSPORT   1 // play: looping, callback events mask,
                                  //       marker position, position update period
                                  // recorder: duration limit, events mask,
                                  //           marker position, position update period
 #define ATTR_INDEX_POSITION    2 // requested position (a.k.a. seek position)
 #define ATTR_INDEX_BQ_ENQUEUE  3 // (buffer queue non-empty and in playing state) became true
 #define ATTR_INDEX_ABQ_ENQUEUE 4 // Android buffer queue became non-empty and in playing state
-#define ATTR_INDEX_UNUSED5     5 // reserved for future use
+#define ATTR_INDEX_PLAY_STATE  5 // play: play state
 #define ATTR_INDEX_UNUSED6     6 // reserved for future use
 #define ATTR_INDEX_UNUSED7     7 // reserved for future use
 #define ATTR_INDEX_MAX         8 // total number of bits used so far
@@ -41,6 +41,6 @@
 #define ATTR_POSITION    (1 << ATTR_INDEX_POSITION)
 #define ATTR_BQ_ENQUEUE  (1 << ATTR_INDEX_BQ_ENQUEUE)
 #define ATTR_ABQ_ENQUEUE (1 << ATTR_INDEX_ABQ_ENQUEUE)
-#define ATTR_UNUSED5     (1 << ATTR_INDEX_UNUSED5)
+#define ATTR_PLAY_STATE  (1 << ATTR_INDEX_PLAY_STATE)
 #define ATTR_UNUSED6     (1 << ATTR_INDEX_UNUSED6)
 #define ATTR_UNUSED7     (1 << ATTR_INDEX_UNUSED7)

diff --git a/wilhelm/src/handler_bodies.c b/wilhelm/src/handler_bodies.c
index 8bc9d62..9e435b8 100644 (file)
--- a/wilhelm/src/handler_bodies.c
+++ b/wilhelm/src/handler_bodies.c
@@ -81,14 +81,21 @@ unsigned handler_MidiPlayer_position(IObject *thiz)
 unsigned handler_AudioPlayer_transport(IObject *thiz)
 {
     CAudioPlayer *ap = (CAudioPlayer *) thiz;
-    // FIXME should only call when state changes
-    android_audioPlayer_setPlayState(ap, false /*lockAP*/);
-    // FIXME ditto, but for either eventflags or marker position
+    // FIXME should only call when either eventflags or marker position changes
     android_audioPlayer_useEventMask(ap);
     return ATTR_TRANSPORT;
 }
 
 
+// SL_OBJECTID_AUDIOPLAYER, ATTR_PLAY_STATE
+unsigned handler_AudioPlayer_play_state(IObject *thiz)
+{
+    CAudioPlayer *ap = (CAudioPlayer *) thiz;
+    android_audioPlayer_setPlayState(ap);
+    return ATTR_PLAY_STATE;
+}
+
+
 // SL_OBJECTID_AUDIORECORDER, ATTR_TRANSPORT
 unsigned handler_AudioRecorder_transport(IObject *thiz)
 {
@@ -98,7 +105,7 @@ unsigned handler_AudioRecorder_transport(IObject *thiz)
 }
 
 
-// XA_OBJECTID_MEDIAPLAYER, ATTR_TRANSPORT
+// XA_OBJECTID_MEDIAPLAYER, ATTR_TRANSPORT or ATTR_PLAY_STATE
 unsigned handler_MediaPlayer_transport(IObject *thiz)
 {
     CMediaPlayer *mp = (CMediaPlayer *) thiz;
@@ -106,7 +113,7 @@ unsigned handler_MediaPlayer_transport(IObject *thiz)
     if (avp != NULL) {
         android_Player_setPlayState(avp, mp->mPlay.mState, &(mp->mAndroidObjState));
     }
-    return ATTR_TRANSPORT;
+    return ATTR_TRANSPORT | ATTR_PLAY_STATE;
 }
 
 
@@ -115,9 +122,8 @@ unsigned handler_AudioPlayer_bq_enqueue(IObject *thiz)
 {
     // ( buffer queue count is non-empty and play state == PLAYING ) became true
     CAudioPlayer *ap = (CAudioPlayer *) thiz;
-    if (SL_PLAYSTATE_PLAYING == ap->mPlay.mState) {
-        android_audioPlayer_bufferQueue_onRefilled_l(ap);
-    }
+    assert(SL_PLAYSTATE_PLAYING == ap->mPlay.mState);
+    android_audioPlayer_bufferQueue_onRefilled_l(ap);
     return ATTR_BQ_ENQUEUE;
 }
 
@@ -127,9 +133,8 @@ unsigned handler_AudioPlayer_abq_enqueue(IObject *thiz)
 {
     // (Android buffer queue count is non-empty and play state == PLAYING ) became true
     CAudioPlayer *ap = (CAudioPlayer *) thiz;
-    if (SL_PLAYSTATE_PLAYING == ap->mPlay.mState) {
-        android_audioPlayer_androidBufferQueue_onRefilled_l(ap);
-    }
+    assert(SL_PLAYSTATE_PLAYING == ap->mPlay.mState);
+    android_audioPlayer_androidBufferQueue_onRefilled_l(ap);
     return ATTR_ABQ_ENQUEUE;
 }
 

diff --git a/wilhelm/src/handlers.c b/wilhelm/src/handlers.c
index 739d0b2..80d9a3f 100644 (file)
--- a/wilhelm/src/handlers.c
+++ b/wilhelm/src/handlers.c
@@ -32,7 +32,8 @@ const AttributeHandler handlerTable[1 + XA_OBJECTID_CAMERADEVICE +
         [ATTR_INDEX_GAIN]        = handler_MediaPlayer_gain,
         [ATTR_INDEX_TRANSPORT]   = handler_MediaPlayer_transport,
         [ATTR_INDEX_POSITION]    = handler_MediaPlayer_position,
-        [ATTR_INDEX_ABQ_ENQUEUE] = handler_MediaPlayer_abq_enqueue},
+        [ATTR_INDEX_ABQ_ENQUEUE] = handler_MediaPlayer_abq_enqueue,
+        [ATTR_INDEX_PLAY_STATE]  = handler_MediaPlayer_play_state},
 
 // SL IDs need a little arithmetic to make them contiguous with XA IDs
 #define _(id) ((id) - SL_OBJECTID_ENGINE + XA_OBJECTID_CAMERADEVICE + 1)
@@ -42,7 +43,8 @@ const AttributeHandler handlerTable[1 + XA_OBJECTID_CAMERADEVICE +
         [ATTR_INDEX_TRANSPORT]   = handler_AudioPlayer_transport,
         [ATTR_INDEX_POSITION]    = handler_AudioPlayer_position,
         [ATTR_INDEX_BQ_ENQUEUE]  = handler_AudioPlayer_bq_enqueue,
-        [ATTR_INDEX_ABQ_ENQUEUE] = handler_AudioPlayer_abq_enqueue},
+        [ATTR_INDEX_ABQ_ENQUEUE] = handler_AudioPlayer_abq_enqueue,
+        [ATTR_INDEX_PLAY_STATE]  = handler_AudioPlayer_play_state},
 
     [_(SL_OBJECTID_AUDIORECORDER)] = {
         [ATTR_INDEX_TRANSPORT]   = handler_AudioRecorder_transport},

diff --git a/wilhelm/src/handlers.h b/wilhelm/src/handlers.h
index 8cdb79b..4cf49ce 100644 (file)
--- a/wilhelm/src/handlers.h
+++ b/wilhelm/src/handlers.h
@@ -29,11 +29,13 @@ extern unsigned handler_MediaPlayer_gain(IObject *thiz);
 extern unsigned handler_MediaPlayer_transport(IObject *thiz);
 extern unsigned handler_MediaPlayer_position(IObject *thiz);
 extern unsigned handler_MediaPlayer_abq_enqueue(IObject *thiz);
+#define handler_MediaPlayer_play_state handler_MediaPlayer_transport
 extern unsigned handler_AudioPlayer_gain(IObject *thiz);
 extern unsigned handler_AudioPlayer_transport(IObject *thiz);
 extern unsigned handler_AudioPlayer_position(IObject *thiz);
 extern unsigned handler_AudioPlayer_bq_enqueue(IObject *thiz);
 extern unsigned handler_AudioPlayer_abq_enqueue(IObject *thiz);
+extern unsigned handler_AudioPlayer_play_state(IObject *thiz);
 extern unsigned handler_AudioRecorder_transport(IObject *thiz);
 extern unsigned handler_MidiPlayer_gain(IObject *thiz);
 extern unsigned handler_MidiPlayer_position(IObject *thiz);
@@ -43,10 +45,12 @@ extern unsigned handler_OutputMix_gain(IObject *thiz);
 #define handler_MediaPlayer_transport   NULL
 #define handler_MediaPlayer_position    NULL
 #define handler_MediaPlayer_abq_enqueue NULL
+#define handler_MediaPlayer_play_state  NULL
 #define handler_AudioPlayer_transport   NULL
 #define handler_AudioPlayer_position    NULL
 #define handler_AudioPlayer_bq_enqueue  NULL
 #define handler_AudioPlayer_abq_enqueue NULL
+#define handler_AudioPlayer_play_state  NULL
 #define handler_AudioRecorder_transport NULL
 #define handler_MidiPlayer_gain         NULL
 #define handler_MidiPlayer_position     NULL

diff --git a/wilhelm/src/itf/IPlay.c b/wilhelm/src/itf/IPlay.c
index 85de877..676113a 100644 (file)
--- a/wilhelm/src/itf/IPlay.c
+++ b/wilhelm/src/itf/IPlay.c
@@ -36,26 +36,29 @@ static SLresult IPlay_SetPlayState(SLPlayItf self, SLuint32 state)
             (CAudioPlayer *) thiz->mThis : NULL;
 #endif
         interface_lock_exclusive(thiz);
+        SLuint32 oldState = thiz->mState;
+        if (state != oldState) {
 #ifdef USE_OUTPUTMIXEXT
-        for (;; interface_cond_wait(thiz)) {
+          for (;; interface_cond_wait(thiz)) {
 
             // We are comparing the old state (left) vs. new state (right).
             // Note that the old state is 3 bits wide, but new state is only 2 bits wide.
             // That is why the old state is on the left and new state is on the right.
-            switch ((thiz->mState << 2) | state) {
+            switch ((oldState << 2) | state) {
 
             case (SL_PLAYSTATE_STOPPED  << 2) | SL_PLAYSTATE_STOPPED:
             case (SL_PLAYSTATE_PAUSED   << 2) | SL_PLAYSTATE_PAUSED:
             case (SL_PLAYSTATE_PLAYING  << 2) | SL_PLAYSTATE_PLAYING:
-               // no-op
+               // no-op, and unreachable due to earlier "if (state != oldState)"
                 break;
 
             case (SL_PLAYSTATE_STOPPED  << 2) | SL_PLAYSTATE_PLAYING:
             case (SL_PLAYSTATE_PAUSED   << 2) | SL_PLAYSTATE_PLAYING:
-                attr = ATTR_TRANSPORT;
+                attr = ATTR_PLAY_STATE;
                 // set enqueue attribute if queue is non-empty and state becomes PLAYING
                 if ((NULL != audioPlayer) && (audioPlayer->mBufferQueue.mFront !=
                     audioPlayer->mBufferQueue.mRear)) {
+                    // note that USE_OUTPUTMIXEXT does not support ATTR_ABQ_ENQUEUE
                     attr |= ATTR_BQ_ENQUEUE;
                 }
                 // fall through
@@ -90,12 +93,14 @@ static SLresult IPlay_SetPlayState(SLPlayItf self, SLuint32 state)
             }
 
             break;
-        }
+          }
 #else
-        // Here life looks easy for an Android, but there are other troubles in play land
-        thiz->mState = state;
-        attr = ATTR_TRANSPORT;
+          // Here life looks easy for an Android, but there are other troubles in play land
+          thiz->mState = state;
+          attr = ATTR_PLAY_STATE;
+          // no need to set ATTR_BQ_ENQUEUE or ATTR_ABQ_ENQUEUE
 #endif
+        }
         // SL_LOGD("set play state %d", state);
         interface_unlock_exclusive_attributes(thiz, attr);
         }