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

2 files changed, 67 insertions, 45 deletions

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;
     }