Refactor code related to I/O handles to reduce chance for leaks

The AudioRecord input handle code was refactored earlier
to fix a potential handle leak, and to simplify the code:
    > Change-Id: I124dce344b1d11c2dd66ca5e2c9aec0c52c230e2

This changelist refactors AudioTrack similarly,
and adds further cleanup of both AudioTrack and AudioRecord.

We attempt to implement the rules for referencing counting I/O handles,
but there is still the possibility of a handle leak if the client process
dies after allocating the handle reference but before releasing it.
That issue is being tracked separately.

Details:
 - AudioSystem::getOutput() is now called within createTrack_l
 - restoreTrack_l was missing offload info
   now it has the info available,
   but is not yet being called for offloaded tracks
 - AudioTrack::getOutput() is now const
 - Remove getOutput_l()

Change-Id: I44a0a623d24fc5847bcac0939c276400568adbca

4 files changed, 72 insertions, 53 deletions

diff --git a/include/media/AudioRecord.h b/include/media/AudioRecord.h
index 45134c4..3d839fc 100644
--- a/include/media/AudioRecord.h
+++ b/include/media/AudioRecord.h
@@ -486,12 +486,11 @@ private:
     int                     mSessionId;
     transfer_type           mTransfer;
 
-    audio_io_handle_t       mInput;             // returned by AudioSystem::getInput()
-
-    // Next 3 fields may be changed if IAudioRecord is re-created, but always != 0
+    // Next 4 fields may be changed if IAudioRecord is re-created, but always != 0
     sp<IAudioRecord>        mAudioRecord;
     sp<IMemory>             mCblkMemory;
     audio_track_cblk_t*     mCblk;              // re-load after mLock.unlock()
+    audio_io_handle_t       mInput;             // returned by AudioSystem::getInput()
 
     int                     mPreviousPriority;  // before start()
     SchedPolicy             mPreviousSchedulingGroup;
diff --git a/include/media/AudioTrack.h b/include/media/AudioTrack.h
index 644e55c..cb67321 100644
--- a/include/media/AudioTrack.h
+++ b/include/media/AudioTrack.h
@@ -454,7 +454,7 @@ public:
      * Returned value:
      *  handle on audio hardware output
      */
-            audio_io_handle_t    getOutput();
+            audio_io_handle_t    getOutput() const;
 
     /* Returns the unique session ID associated with this track.
      *
@@ -640,14 +640,12 @@ protected:
                                  size_t frameCount,
                                  audio_output_flags_t flags,
                                  const sp<IMemory>& sharedBuffer,
-                                 audio_io_handle_t output,
                                  size_t epoch);
 
             // can only be called when mState != STATE_ACTIVE
             void flush_l();
 
             void setLoop_l(uint32_t loopStart, uint32_t loopEnd, int loopCount);
-            audio_io_handle_t getOutput_l();
 
             // FIXME enum is faster than strcmp() for parameter 'from'
             status_t restoreTrack_l(const char *from);
@@ -655,10 +653,11 @@ protected:
             bool     isOffloaded_l() const
                 { return (mFlags & AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD) != 0; }
 
-    // Next 3 fields may be changed if IAudioTrack is re-created, but always != 0
+    // Next 4 fields may be changed if IAudioTrack is re-created, but always != 0
     sp<IAudioTrack>         mAudioTrack;
     sp<IMemory>             mCblkMemory;
     audio_track_cblk_t*     mCblk;                  // re-load after mLock.unlock()
+    audio_io_handle_t       mOutput;                // returned by AudioSystem::getOutput()
 
     sp<AudioTrackThread>    mAudioTrackThread;
 
@@ -763,7 +762,6 @@ private:
 
     sp<DeathNotifier>       mDeathNotifier;
     uint32_t                mSequence;              // incremented for each new IAudioTrack attempt
-    audio_io_handle_t       mOutput;                // cached output io handle
     int                     mClientUid;
 };
 
diff --git a/media/libmedia/AudioRecord.cpp b/media/libmedia/AudioRecord.cpp
index 0673079..a999e7e 100644
--- a/media/libmedia/AudioRecord.cpp
+++ b/media/libmedia/AudioRecord.cpp
@@ -244,7 +244,7 @@ status_t AudioRecord::set(
 
     // create the IAudioRecord
     status = openRecord_l(0 /*epoch*/);
-    if (status) {
+    if (status != NO_ERROR) {
         return status;
     }
 
@@ -463,6 +463,9 @@ status_t AudioRecord::openRecord_l(size_t epoch)
         ALOGE("Could not get audio input for record source %d", mInputSource);
         return BAD_VALUE;
     }
+    {
+    // Now that we have a reference to an I/O handle and have not yet handed it off to AudioFlinger,
+    // we must release it ourselves if anything goes wrong.
 
     size_t temp = mFrameCount;  // temp may be replaced by a revised value of frameCount,
                                 // but we will still need the original value also
@@ -480,9 +483,11 @@ status_t AudioRecord::openRecord_l(size_t epoch)
 
     if (record == 0 || status != NO_ERROR) {
         ALOGE("AudioFlinger could not create record track, status: %d", status);
-        AudioSystem::releaseInput(input);
-        return status;
+        goto release;
     }
+    // AudioFlinger now owns the reference to the I/O handle,
+    // so we are no longer responsible for releasing it.
+
     sp<IMemory> iMem = record->getCblk();
     if (iMem == 0) {
         ALOGE("Could not get control block");
@@ -497,6 +502,8 @@ status_t AudioRecord::openRecord_l(size_t epoch)
         mAudioRecord->asBinder()->unlinkToDeath(mDeathNotifier, this);
         mDeathNotifier.clear();
     }
+
+    // We retain a copy of the I/O handle, but don't own the reference
     mInput = input;
     mAudioRecord = record;
     mCblkMemory = iMem;
@@ -540,6 +547,14 @@ status_t AudioRecord::openRecord_l(size_t epoch)
     mAudioRecord->asBinder()->linkToDeath(mDeathNotifier, this);
 
     return NO_ERROR;
+    }
+
+release:
+    AudioSystem::releaseInput(input);
+    if (status == NO_ERROR) {
+        status = NO_INIT;
+    }
+    return status;
 }
 
 status_t AudioRecord::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp
index c8dc38b..8e91f12 100644
--- a/media/libmedia/AudioTrack.cpp
+++ b/media/libmedia/AudioTrack.cpp
@@ -245,8 +245,6 @@ status_t AudioTrack::set(
         return INVALID_OPERATION;
     }
 
-    mOutput = 0;
-
     // handle default values first.
     if (streamType == AUDIO_STREAM_DEFAULT) {
         streamType = AUDIO_STREAM_MUSIC;
@@ -319,17 +317,6 @@ status_t AudioTrack::set(
         mFrameSizeAF = sizeof(uint8_t);
     }
 
-    audio_io_handle_t output = AudioSystem::getOutput(
-                                    streamType,
-                                    sampleRate, format, channelMask,
-                                    flags,
-                                    offloadInfo);
-
-    if (output == 0) {
-        ALOGE("Could not get audio output for stream type %d", streamType);
-        return BAD_VALUE;
-    }
-
     // Make copy of input parameter offloadInfo so that in the future:
     //  (a) createTrack_l doesn't need it as an input parameter
     //  (b) we can support re-creation of offloaded tracks
@@ -369,7 +356,6 @@ status_t AudioTrack::set(
                                   frameCount,
                                   flags,
                                   sharedBuffer,
-                                  output,
                                   0 /*epoch*/);
 
     if (status != NO_ERROR) {
@@ -379,9 +365,15 @@ status_t AudioTrack::set(
             mAudioTrackThread.clear();
         }
         // Use of direct and offloaded output streams is ref counted by audio policy manager.
+#if 0   // FIXME This should no longer be needed
+        //Use of direct and offloaded output streams is ref counted by audio policy manager.
         // As getOutput was called above and resulted in an output stream to be opened,
         // we need to release it.
-        AudioSystem::releaseOutput(output);
+        if (mOutput != 0) {
+            AudioSystem::releaseOutput(mOutput);
+            mOutput = 0;
+        }
+#endif
         return status;
     }
 
@@ -397,7 +389,6 @@ status_t AudioTrack::set(
     mSequence = 1;
     mObservedSequence = mSequence;
     mInUnderrun = false;
-    mOutput = output;
 
     return NO_ERROR;
 }
@@ -821,23 +812,12 @@ status_t AudioTrack::reload()
     return NO_ERROR;
 }
 
-audio_io_handle_t AudioTrack::getOutput()
+audio_io_handle_t AudioTrack::getOutput() const
 {
     AutoMutex lock(mLock);
     return mOutput;
 }
 
-// must be called with mLock held
-audio_io_handle_t AudioTrack::getOutput_l()
-{
-    if (mOutput) {
-        return mOutput;
-    } else {
-        return AudioSystem::getOutput(mStreamType,
-                                      mSampleRate, mFormat, mChannelMask, mFlags);
-    }
-}
-
 status_t AudioTrack::attachAuxEffect(int effectId)
 {
     AutoMutex lock(mLock);
@@ -858,7 +838,6 @@ status_t AudioTrack::createTrack_l(
         size_t frameCount,
         audio_output_flags_t flags,
         const sp<IMemory>& sharedBuffer,
-        audio_io_handle_t output,
         size_t epoch)
 {
     status_t status;
@@ -868,27 +847,39 @@ status_t AudioTrack::createTrack_l(
         return NO_INIT;
     }
 
+    audio_io_handle_t output = AudioSystem::getOutput(mStreamType, mSampleRate, mFormat,
+            mChannelMask, mFlags, mOffloadInfo);
+    if (output == 0) {
+        ALOGE("Could not get audio output for stream type %d, sample rate %u, format %#x, "
+              "channel mask %#x, flags %#x",
+              mStreamType, mSampleRate, mFormat, mChannelMask, mFlags);
+        return BAD_VALUE;
+    }
+    {
+    // Now that we have a reference to an I/O handle and have not yet handed it off to AudioFlinger,
+    // we must release it ourselves if anything goes wrong.
+
     // Not all of these values are needed under all conditions, but it is easier to get them all
 
     uint32_t afLatency;
     status = AudioSystem::getLatency(output, streamType, &afLatency);
     if (status != NO_ERROR) {
         ALOGE("getLatency(%d) failed status %d", output, status);
-        return NO_INIT;
+        goto release;
     }
 
     size_t afFrameCount;
     status = AudioSystem::getFrameCount(output, streamType, &afFrameCount);
     if (status != NO_ERROR) {
         ALOGE("getFrameCount(output=%d, streamType=%d) status %d", output, streamType, status);
-        return NO_INIT;
+        goto release;
     }
 
     uint32_t afSampleRate;
     status = AudioSystem::getSamplingRate(output, streamType, &afSampleRate);
     if (status != NO_ERROR) {
         ALOGE("getSamplingRate(output=%d, streamType=%d) status %d", output, streamType, status);
-        return NO_INIT;
+        goto release;
     }
 
     // Client decides whether the track is TIMED (see below), but can only express a preference
@@ -940,7 +931,8 @@ status_t AudioTrack::createTrack_l(
         if (((size_t)sharedBuffer->pointer() & (alignment - 1)) != 0) {
             ALOGE("Invalid buffer alignment: address %p, channel count %u",
                     sharedBuffer->pointer(), mChannelCount);
-            return BAD_VALUE;
+            status = BAD_VALUE;
+            goto release;
         }
 
         // When initializing a shared buffer AudioTrack via constructors,
@@ -1020,8 +1012,11 @@ status_t AudioTrack::createTrack_l(
 
     if (track == 0) {
         ALOGE("AudioFlinger could not create track, status: %d", status);
-        return status;
+        goto release;
     }
+    // AudioFlinger now owns the reference to the I/O handle,
+    // so we are no longer responsible for releasing it.
+
     sp<IMemory> iMem = track->getCblk();
     if (iMem == 0) {
         ALOGE("Could not get control block");
@@ -1081,10 +1076,13 @@ status_t AudioTrack::createTrack_l(
             ALOGW("AUDIO_OUTPUT_FLAG_OFFLOAD denied by server");
             flags = (audio_output_flags_t) (flags & ~AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD);
             mFlags = flags;
-            return NO_INIT;
+            // FIXME This is a warning, not an error, so don't return error status
+            //return NO_INIT;
         }
     }
 
+    // We retain a copy of the I/O handle, but don't own the reference
+    mOutput = output;
     mRefreshRemaining = true;
 
     // Starting address of buffers in shared memory.  If there is a shared buffer, buffers
@@ -1127,6 +1125,14 @@ status_t AudioTrack::createTrack_l(
     mAudioTrack->asBinder()->linkToDeath(mDeathNotifier, this);
 
     return NO_ERROR;
+    }
+
+release:
+    AudioSystem::releaseOutput(output);
+    if (status == NO_ERROR) {
+        status = NO_INIT;
+    }
+    return status;
 }
 
 status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
@@ -1708,7 +1714,7 @@ status_t AudioTrack::restoreTrack_l(const char *from)
     status_t result;
 
     // refresh the audio configuration cache in this process to make sure we get new
-    // output parameters in getOutput_l() and createTrack_l()
+    // output parameters in createTrack_l()
     AudioSystem::clearAudioConfigCache();
 
     if (isOffloaded_l()) {
@@ -1716,10 +1722,6 @@ status_t AudioTrack::restoreTrack_l(const char *from)
         return DEAD_OBJECT;
     }
 
-    // force new output query from audio policy manager;
-    mOutput = 0;
-    audio_io_handle_t output = getOutput_l();
-
     // if the new IAudioTrack is created, createTrack_l() will modify the
     // following member variables: mAudioTrack, mCblkMemory and mCblk.
     // It will also delete the strong references on previous IAudioTrack and IMemory
@@ -1733,7 +1735,6 @@ status_t AudioTrack::restoreTrack_l(const char *from)
                            mReqFrameCount,  // so that frame count never goes down
                            mFlags,
                            mSharedBuffer,
-                           output,
                            position /*epoch*/);
 
     if (result == NO_ERROR) {
@@ -1763,9 +1764,15 @@ status_t AudioTrack::restoreTrack_l(const char *from)
     }
     if (result != NO_ERROR) {
         // Use of direct and offloaded output streams is ref counted by audio policy manager.
+#if 0   // FIXME This should no longer be needed
+        //Use of direct and offloaded output streams is ref counted by audio policy manager.
         // As getOutput was called above and resulted in an output stream to be opened,
         // we need to release it.
-        AudioSystem::releaseOutput(output);
+        if (mOutput != 0) {
+            AudioSystem::releaseOutput(mOutput);
+            mOutput = 0;
+        }
+#endif
         ALOGW("restoreTrack_l() failed status %d", result);
         mState = STATE_STOPPED;
     }